Breakage caused by PinStatus and PinMode types

[per1234]

This project changes LOW, HIGH, INPUT, INPUT_PULLUP, and OUTPUT from macros to enums: https://github.com/arduino/ArduinoCore-API/blob/e1eb8de126786b7701b211332dda3f09aa400f35/api/Common.h#L10-L23

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
• https://github.com/arduino/ArduinoCore-megaavr/issues/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 :slightly_smiling_face: https://github.com/arduino/ArduinoCore-API/blob/55cbc081bb5bcf1244743c3387b85ba56d0fe4b2/api/Compat.h#L1-L16

[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.

[Coding-Badly]

Jack! I hope you've been well!

Excellent question. I can imagine two possibilities.

1. bool is severely limited in the amount of information that it can carry. A pin status type can carry the same information and later, if necessary, be changed to carry additional information. It's a way of "future proofing" the interface.

2. Having a type specific to pin status allows overloading. It is possible for digitalWrite(pin, bool) to have different behaviour than digitalWrite(pin, PinStatus). (I can't think of a valid reason to do that but I also have not put much thought into it.)

[bxparks]

Question 1:

Looking at $ARDUINO_IDE/portable/packages/arduino/hardware/megaavr/1.8.1/cores/arduino/wiring_digital.c, I see that digitalWrite() now accepts 3 values of PinStatus (HIGH, LOW and CHANGE). It seems that CHANGE causes the pin to toggle, and CHANGE is defined to be an enum of 2.

However, on a normal AVR, CHANGE is defined to be 1 (see $ARDUINO_IDE/hardware/arduino/avr/cores/arduino/Arduino.h) so digitalWrite(pin, CHANGE) compiles, but has the same effect as digitalWrite(pin, HIGH).

Is this intended?

Question 2:

Is there a #define macro that identifies the megaAVR boards which use this new PinMode and PinStatus API, so that I can fix my library for the megaAVR?

[facchinm]

Hi @bxparks

1. probably implementing CHANGE as pin toggle was not the best idea ever; anyway, not using it doesn't hurt :slightly_smiling_face: What's your use case?
2. you can target any core implementing PinMode and PinStatus with #if (ARDUINO_API_VERSION >= 10000)

[bxparks]

Hi @facchinm,

I don't use digitalWrite(pin, CHANGE)... I was trying to deduce the valid range of PinStatus for the new digitalWrite() and digitalRead(). Previously both of them were documented to handle only 2 values: HIGH and LOW. Now digitalWrite() accepts 3 values. So I'm wondering, will digitalRead() ever return any other values of PinStatus?

The reason for asking is that my AceButton library stores a value of digitalRead() that represents "UNKNOWN", in other words, the state of the pin before any digitalRead() has been performed. But the documentation for digitalRead() does not define the value of HIGH or LOW, so I have a compile-time check like this:

#if HIGH != 1
  #error HIGH must be defined to be 1
#endif
#if LOW != 0
  #error LOW must be defined to be 0
#endif

Which enforces that my "UNKNOWN" value is different from either HIGH or LOW. Obviously for the megaAVR, these checks no longer work. And obviously, I cannot use PinStatus to store my digitalRead() result, because "UNKNOWN" cannot be represented by the enum.

Thanks for the pointer to ARDUINO_API_VERSION. Is it correct to assume that eventually even the normal AVRs will migrate to the new API, with the PinStatus and PinMode enums?

[facchinm]

The whole idea of HIGH and LOW is to hide the implementations details (HIGH could be 0 and LOW 0x12 in some random funky architecture, but your Arduino code will run seamlessly). Expecting them to be 0 and 1 (with no other value allowed) breaks this assumption, so maybe there's a better way to enforce the UNKNOWN field :wink:

[PaulStoffregen]

Overloading digitalWrite will break much of the Arduino ecosystem. A tremendous amount of code has been built up over the last decade equating digitalWrite & digitalRead values with the C/C++ convention of zero representing logical false to mean LOW and non-zero representing logical true to mean HIGH.

[facchinm]

@PaulStoffregen of course most (all?) the architectures are sane and HIGH is 1 while LOW is zero but since the APIs don't specify the value of those macros/enum/whatever (and never did) the code runs "by chance" :slightly_smiling_face:

[bxparks]

I guess I'm having a hard time seeing what advantages the PinStatus API offers that outweighs the cost breaking backwards compatibility. We've already seen that the one new feature that it does provide, the ability to toggle using digitalWrite(pin, CHANGE) cannot be used, since it breaks compatibility with all other platforms.

I also understand that writing:

pinStatus = (pinStatus != LOW)? LOW : HIGH;

is probably more clear to most people than

pinStatus ^= 0x1;

but it would be nice to have the option of writing the second form in places where it makes sense (e.g. deep inside a library where app developers are not expected to visit often). The XOR can save a few bytes of flash memory and CPU cycles, since the compiler cannot optimize the first into the second. (Interestingly, when I wrote the first version using ?: operator, I actually wrote it wrong initially and flipped the LOW and HIGH. That expression is not 100% easy to get correct. Hmm.)

With regards to UNKNOWN, yes it's always possible to use a second byte to store the bool isUnknown flag. But that's using 2 bytes to store 2 bits of information (LOW, HIGH, UNKNOWN), instead of just one byte. Now, if PinStatus contained a PinStatus::UNKNOWN whose value was guaranteed to be never returned by digitalRead(), then PinStatus would actually provide some value to me.

In any case, it looks like the new PinStatus API was introduced almost 2 years ago, so the ship has sailed? I will use

#if (ARDUINO_API_VERSION >= 10000)
...
#endif

to target different platforms appropriately.

[WestfW]

the APIs don't specify the value of those macros/enum/whatever (and never did) the code runs "by chance" wiring_shift.c in the arduino core ( https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/wiring_shift.c#L32 ) passes a boolean to digitalWrite(), and the Firmata library uses 0/non-zero: https://github.com/firmata/arduino/blob/master/Boards.h#L970

[PaulStoffregen]

That sort of code is pervasive throughout the Arduino ecosystem.

I fear these API changes could bring widespread misery.

[bxparks]

I tried to protect myself against those assumptions with my compile-time checks, e.g.:

#if HIGH != 1
  #error HIGH must be defined to be 1
#endif
#if LOW != 0
  #error LOW must be defined to be 0
#endif

But even that breaks under this new API. :-/

[WestfW]

That's presumably because HIGH and LOW are now part of an enum rather than mere constants.

You can probably do:

void unexpected_HIGH_or_LOW_value(void) __attribute__ (( error("") ));
// ...
void MyFunction (...) {
   if (HIGH != 1 || LOW != 0)
      unexpected_HIGH_or_LOW_value();
//    ...
}

Since HIGH and LOW are still compile-time constants, this won't normally produce any actual code, but they no longer need to be preprocessor-time constants. (There are a bunch of places where it might be useful to use this sort of thing in the Arduino code. But that's for another time/issue!)

[bxparks]

(This is getting slightly off topic, but hopefully it's useful to someone else facing a similar problem with compile-time checking of HIGH and LOW values)

@WestfW: Thanks for the pointer! I didn't know about the error attribute. I discovered 3 problems with your solution unfortunately:

1. The compiler complains about an unused MyFunction() if this check is the only code inside.
2. I have to create a new unexpected_HIGH_or_LOW_value() function for each compile-time check that I want to perform.
3. The error attribute is a g++ only extension, clang++ does not recognize it. (This is a problem for me because I try to write portable code, so that I can compile and run my Arduino unit tests on Linux (g++ or clang++) and MacOS (clang++)).

After reading the GCC guide on __attributte__ on the extern char [(condition) ? 1 : -1]; trick, and this blog on catching compile-time errors, I came up with the following:

#define CONCAT_(x, y) x##y
#define CONCAT(x,y) CONCAT_(x,y)
#define COMPILE_TIME_ASSERT(cond, msg) \
    extern char CONCAT(compile_time_assert, __LINE__)[(cond) ? 1 : -1];
COMPILE_TIME_ASSERT(HIGH == 1, "HIGH must be 1")
COMPILE_TIME_ASSERT(LOW == 0, "LOW must be 0")

Works on both a #define HIGH and an enum value of HIGH.

[Avamander]

Can this be fixed as soon as possible? It's unnecessarily incompatible with a large majority of other platforms, megaavr is just an annoying outlier here.

[Avamander]

@facchinm Can this god-awful change be reverted, there is really zero benefit to this breakage.

[JM-FRANCE]

Part of me would say "too bad for those who never understood that type coherence is an important concept"... but I would vote against getting that live too.

I've seen too much bad code that has been confusing a truth value (a bool) with a voltage status value (HIGH and LOW or even UNKNOWN) that I fear this change would have terrible impacts on newcomers trying to use random libraries and books that have been relying on the "expected" behavior.

And the benefit is very limited...

[MCUdude]

Since the devs aren't listening. I'm tired of this too, so I will get rid of this in the MegaCoreX repo. This core is 100% compatible with the Uno Wifi Rev2 and the Nano Every.

@SpenceKonde might do it for megaTinyCore as well.