Breakage caused by PinStatus and PinMode types

[per1234]

This project changes LOW, HIGH, INPUT, INPUT_PULLUP, and OUTPUT from macros to enums:

ArduinoCore-API/api/Common.h

Lines 10 to 23 in e1eb8de

	typedef enum {
	LOW = 0,
	HIGH = 1,
	CHANGE = 2,
	FALLING = 3,
	RISING = 4,
	} PinStatus;
	typedef enum {
	INPUT = 0x0,
	OUTPUT = 0x1,
	INPUT_PULLUP = 0x2,
	INPUT_PULLDOWN = 0x3,
	} PinMode;

I'm concerned that this will cause breakage of a significant amount of existing code.

An example is the popular Keypad library. Compilation of both the original library and the version in Library Manager fails once this change is made.

In file included from E:\electronics\arduino\libraries\Keypad-master\examples\HelloKeypad\HelloKeypad.ino:10:0:

E:\electronics\arduino\libraries\Keypad-master\src/Keypad.h: In member function 'virtual void Keypad::pin_write(byte, boolean)':

E:\electronics\arduino\libraries\Keypad-master\src/Keypad.h:81:81: error: cannot convert 'boolean {aka bool}' to 'PinStatus' for argument '2' to 'void digitalWrite(pin_size_t, PinStatus)'

  virtual void pin_write(byte pinNum, boolean level) { digitalWrite(pinNum, level); }

This commonly used code will also now fail:

digitalWrite(13, !digitalRead(13));  // toggle pin 13

toggle:2:36: error: cannot convert 'bool' to 'PinStatus' for argument '2' to 'void digitalWrite(pin_size_t, PinStatus)'

   digitalWrite(13, !digitalRead(13));  // toggle pin 13

I understand that the root cause of these errors is bad code and that any code which followed best practices will have no problems with this change. However, I fear there is a lot of bad code in widespread use that currently works fine. In the case of the Keypad library, it is unlikely it can even be fixed since Chris--A has gone AWOL. I'm sure there are other such abandoned projects.

I do like the spirit of this change (though lumping CHANGE, FALLING, and RISING into PinStatus is questionable). I'm open to being convinced that it's worth the breakage and, if so, I'm willing to help ease the transition by providing user support and submitting PRs to fix broken code. I just think this warrants some consideration before ArduinoCore-API goes into more widespread use.

Some previous discussion on the topic:

• http://forum.arduino.cc/index.php?topic=455579 (from here onward)
• https://forum.arduino.cc/index.php?topic=584322
• http://forum.arduino.cc/index.php?topic=602250
• https://forum.arduino.cc/index.php?topic=621429
• https://forum.arduino.cc/index.php?topic=627883.msg4319254#msg4319254
• https://forum.arduino.cc/index.php?topic=659624
• digitalWrite has a wrong signature ArduinoCore-megaavr#68

[PaulStoffregen]

I can confirm more code in the wild which does an ifdef check on INPUT_PULLUP (and falls back to pinMode & digitalWrite), because INPUT_PULLUP was added to Arduino later than the others.

[matthijskooijman]

I can confirm more code in the wild which does an ifdef check on INPUT_PULLUP (and falls back to pinMode & digitalWrite), because INPUT_PULLUP was added to Arduino later than the others.

One way to fix that, without sacrificing the enum, is to add #define INPUT_PULLUP INPUT_PULLUP. I think we've used this trick for other macros converted to constants in the AVR core before (or at least discussed it).

As for the convert-from-boolean point, I guess one could introduce a class type to wrap these constants and add the appropriate conversion operators on them, though that might become a bit more complex than one would like. It does keep the advantage of separate types for each of these. It would also allow splitting PinStatus and SignalFlank (or whatever), and still allow using LOW and HIGH for both by adding implicit conversions.

[Coding-Badly]

The digitalRead can be handled with an operator...

bool operator ! (const PinStatus &v)
{
  return v == HIGH ? LOW : HIGH;
}

[Coding-Badly]

The digitalWrite can handled with an overload...

void digitalWrite( unsigned pin, PinStatus value )
{
...
}

void digitalWrite( unsigned pin, bool value )
{
  digitalWrite( pin, value ? HIGH : LOW );
}

Ideally, the unsigned / bool function is placed in a header file and marked to be inlined.

[Floessie]

Maybe a stupid question: Why does digitalWrite() take a PinStatus at all? CHANGE, FALLING, and RISING make no sense, or do they? As PinStatus is a class-less enum, providing only a void digitalWrite(unsigned pin, bool value) would solve the problem IMHO.

[WestfW]

I agree with Floessie - having digitalRead/digitalWrite use pinStatus() is no better than the code that expects/passes booleans. There is no reason to overload the enum with functionality from two separate sets of functionality...

[per1234]

Today another report came in of a popular library broken by this:
https://bitbucket.org/fmalpartida/new-liquidcrystal

Minimal demonstration sketch:

#include <LiquidCrystal_I2C.h>
void setup() {}
void loop() {}

E:\arduino\libraries\Newliquidcrystal_1.3.5\LiquidCrystal.cpp: In member function 'virtual void LiquidCrystal::send(uint8_t, uint8_t)':

E:\arduino\libraries\Newliquidcrystal_1.3.5\LiquidCrystal.cpp:125:48: error: cannot convert 'bool' to 'PinStatus' for argument '2' to 'void digitalWrite(pin_size_t, PinStatus)'

    digitalWrite( _rs_pin, ( mode == LCD_DATA ) );

                                                ^

E:\arduino\libraries\Newliquidcrystal_1.3.5\LiquidCrystal.cpp: In member function 'void LiquidCrystal::writeNbits(uint8_t, uint8_t)':

E:\arduino\libraries\Newliquidcrystal_1.3.5\LiquidCrystal.cpp:305:48: warning: invalid conversion from 'int' to 'PinStatus' [-fpermissive]

       digitalWrite(_data_pins[i], (value >> i) & 0x01);

                                                ^

In file included from C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\megaavr\1.6.210\cores\arduino/api/ArduinoAPI.h:52:0,

                 from C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\megaavr\1.6.210\cores\arduino/Arduino.h:23,

                 from E:\arduino\libraries\Newliquidcrystal_1.3.5\LiquidCrystal.cpp:36:

C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\megaavr\1.6.210\cores\arduino/api/Common.h:104:6: note:   initializing argument 2 of 'void digitalWrite(pin_size_t, PinStatus)'

 void digitalWrite(pin_size_t pinNumber, PinStatus status);

      ^

Reported at:
http://forum.arduino.cc/index.php?topic=595987

[facchinm]

Could something like this work for everyone? (it is from the namespace_arduino branch, hence the namespace stuff 🙂

ArduinoCore-API/api/Compat.h

Lines 1 to 16 in 55cbc08

	#ifndef __COMPAT_H__
	#define __COMPAT_H__
	namespace arduino {
	inline void pinMode(pin_size_t pinNumber, int mode) {
	pinMode(pinNumber, (PinMode)mode);
	};
	inline void digitalWrite(pin_size_t pinNumber, int status) {
	digitalWrite(pinNumber, (PinStatus)status);
	};
	}
	#endif

[per1234]

@facchinm this is probably a dumb question, but it's not clear to me exactly how this would be implemented. I get the idea of overloading the functions. But pinMode and digitalWrite are C, which doesn't support function overloading in this manner.

I was trying to modify the Arduino megaAVR Boards core code to test this solution against the known issues reported so far in the thread. I tried just replacing the api folder with the one from the namespace_arduino branch of ArduinoCore-API, but then I get the same issues as well as some new unrelated errors.

[facchinm]

@per1234 it's not a dumb question at all!
In the namespace branch all implementations are C++ only, and only one of them is declared extern "C" so it can be used from any C file.
In this case, the (non compact) implementation gets exposed, while the other one is only available in C++ files.
Maybe it's not the most elegant approach but it should solve 99% of the problems (if the nonstandard code lives in the sketch or in a c++ library)

[per1234]

Thanks for the explanation! I agree that the amount of C code in existence affected by this issue is probably very small. I hadn't considered that the solution would be limited to C++/.ino.

[MCUdude]

I'll have to agree with @WestfW and @Floessie on this one.

I understand that @facchinm tries to provide a solution that will keep the new implementation, but what do we need this new implementation in the first place? It only leaves ut with a workaround that's not compatible with C libraries, not any benefits as far as I can see.

[JChristensen]

I'd be interested to know what problem is solved by introducing the PinStatus type. If this is about type safety, then I think there is such a thing as getting carried away. For example, it seems perfectly natural to pass a bool as the second argument to digitalWrite(). In that case, introducing PinStatus is not only confusing but seems like reinventing the wheel, as the language already provides a completely adequate type. If in fact this change is about type safety, then I'd have to wonder how big the problem was in the first place.

Perhaps worse, the current situation is inconsistent between various boards, with megaAVR architecture demanding a PinStatus type (e.g. for digitalWrite), and regular AVR being more permissive. In fact, PinStatus is not defined for the AVR architecture.