Breakage caused by PinStatus and PinMode types

per1234 commented 5 years ago

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:

drf5n commented 1 year ago

I like that the PinStatus enum names LOW = 0 and HIGH = 1 as used in the text of :

https://docs.arduino.cc/built-in-examples/basics/DigitalReadSerial

from

https://github.com/arduino/docs-content/blob/main/content/built-in-examples/01.basics/DigitalReadSerial/DigitalReadSerial.md

Now that your setup has been completed, move into the main loop of your code. When your button is pressed, 5 volts will freely flow through your circuit, and when it is not pressed, the input pin will be connected to ground through the 10k ohm resistor. This is a digital input, meaning that the switch can only be in either an on state (seen by your Arduino as a "1", or HIGH) or an off state (seen by your Arduino as a "0", or LOW), with nothing in between. ... Now, when you open your Serial Monitor in the Arduino Software (IDE), you will see a stream of "0"s if your switch is open, or "1"s if your switch is closed.

and the

     thirdSensor = map(digitalRead(2), 0, 1, 0, 255);

line from https://docs.arduino.cc/built-in-examples/communication/SerialCallResponse

It doesn't seem fair to call depending on LOW=0/HIGH=1 as bad code if the reference documentation has been pushing it for years

On the other hand, isn't this PinStatus enum inconsistent with the either-or text from https://www.arduino.cc/reference/en/language/functions/digital-io/digitalread/ and https://www.arduino.cc/reference/en/language/functions/digital-io/digitalwrite/

And doesn't this break expectations for this code:

https://github.com/arduino/ArduinoCore-megaavr/blob/ffab9cb2e4bca7647f11d6e25727788aec597a03/cores/arduino/wiring_digital.c#L148-L161

If you call digitalWrite(13,FALLING); should it set the pin or clear the pin?

SpenceKonde commented 1 year ago

IMO this whole concept of pin enums was something that would be great if we were starting from scratch low end ARMs where we could afford to piss away some flash like that. But we''er on AVRs, there's a long established tradition of the 0/1 being legal for digitalWrite, and it's easy to come up with plausible use cases and common ideoms where it falls over.

That's why I 86'ed it from my cores. (the whole Arduino API was sort of a disaster IMO - at this point it's been pretty much gutted on my cores as people have complained (with good reason) about the bloat it introduced (virtual functions are the devil's creation).

WestfW commented 1 year ago

If we were starting from scratch, "pins" should probably have been objects.

aentinger commented 1 year ago

virtual functions are the devil's creation

Why are virtual functions the devils creation? Please inform me about the performance cost you think they'll incur ;)

Note: I'll grant you that they do incur a cost, but not as great as many people apparently think.

bperrybap commented 1 year ago

One issue with virtual functions is that they will get linked in regardless if they are used. The compiler & linker options that are used to remove unused functions don't work for virtual functions and while there was a way (hack) to patch the vtable to allow the linker to remove unused fucntions, that no longer works in the more recent gcc toolsets.

ianfixes commented 1 year ago

I'm only just running into this issue in Arduino CI; I found this thread by running a google search for the actual definition of digitalWrite, and so far this discussion is very discouraging.

Arduino CI enables unit testing an Arduino library by mocking the entire set of Arduino built-ins, such that they can be fed scripted inputs and have their outputs analyzed. See the definitions of HIGH and LOW and digitalRead/digitalWrite respectively. (I'm not sure how correct these are, but they have been working for quite some time as various commenters in this thread can personally attest to.)

To make code more testable in a library that I am writing, I prefer to follow the dependency injection pattern. This simplifies the problem of testing, since I can precisely simulate the inputs I want my library to handle by writing a mocked version of digitalRead (rather than manually energizing wires attached to real hardware and looking at the serial port monitor). All I need to do to support Dependency Injection is declare the precise type signature of the digitalRead function in the library, and require the sketch to pass a reference to digitalRead to the library as part of setup().

But what is the correct type signature of this function???

For this to have 2 different signatures for 2 different boards spells disaster for any library that wants to work for multiple boards -- it looks like I have to pick one signature or the other, and using preprocessor directives to sort this out seems like a very unsustainable way to go.

How can I encapsulate the possibly changing signature of this function in (1) an Arduino library that I write and (2) my mocks for testing Arduino libraries as part of Arduino CI?

ianfixes commented 1 year ago

Addendum, even overloading isn't an obvious solution here, because in some cases PinStatus won't even be defined.

bperrybap commented 1 year ago

For this to have 2 different signatures for 2 different boards spells disaster for any library that wants to work for multiple boards -- it looks like I have to pick one signature or the other, and using preprocessor directives to sort this out seems like a very unsustainable way to go.

How can I encapsulate the possibly changing signature of this function in (1) an Arduino library that I write and (2) my mocks for testing Arduino libraries as part of Arduino CI?

Number 1 rule: never abuse the API and use the APIs the way they are documented. (using a specific type for HIGH and LOW is abusing the API) You have to treat HIGH and LOW as an opaque unknown type.

never assume that their type is anything and could change.
never attempt to assign HIGH or LOW to a variable and never use a variable as the value parameter to digitalWrite()

You do things like when you call functions like digitalWrite() always use HIGH or LOW for the value parameter, not a variable.

For digitalRead() always compare the return value to LOW or HIGH and then do things based on that evaluation. i.e. if LOW set a variable to some value, if HIGH then set it to some other value. When using your variable assigned based on return value of digitalRead() to set a pin using digitalWrite(), look at the variable and then call dititalWrite() with LOW or HIGH

My libraries do this and it didn't have any issues with the different types for HIGH and LOW. It works on all boards within all cores and the types for HIGH and LOW can change and it won't affect my code compiling and working.

If you are trying to do something special or out of the ordinary for testing / emulation purposes, again you cannot ever assume any type for the symbols HIGH and LOW given the way the API is documented and defined.

If you for some reason absolutely need their type you can get to them by using decltype() i.e decltype(HIGH) or decltype(LOW)

So I would think that between a clever combination of use of macros to remap function names and the use decltype() you could get to anything you want / need for your simulation / emulation purposes.

stuff like

decltype(HIGH) my_digitalRead(int pin);
void my_digitalWrite(int pin, decltype(HIGH));
decltype(HIGH) value; // this variable can be used with digitalRead() and digitalWrite()

You could even create a typedef for the decltype(HIGH) that would hide this away

Michael-Brodsky commented 1 year ago

Untyped object-like macros, IMHO this is half the problem with the API.

On Tue, May 2, 2023 at 8:59 AM Ian @.***> wrote:

I'm only just running into this issue in Arduino CI https://github.com/Arduino-CI/arduino_ci; I found this thread by running a google search for the actual definition of digitalWrite, and so far this discussion is very discouraging.

Arduino CI enables unit testing an Arduino library by mocking the entire set of Arduino built-ins, such that they can be fed scripted inputs and have their outputs analyzed. See the definitions of HIGH and LOW https://github.com/Arduino-CI/arduino_ci/blob/6f326f8070e619802ff6efe07bc3340313806d1f/cpp/arduino/ArduinoDefines.h#L5-L6 and digitalRead/digitalWrite https://github.com/Arduino-CI/arduino_ci/blob/6f326f8070e619802ff6efe07bc3340313806d1f/cpp/arduino/Godmode.h#L166-L167 respectively.

To make code more testable, I prefer to follow the dependency injection https://allencch.wordpress.com/2017/12/05/c-unit-test-and-dependency-injection/ pattern. This simplifies the problem of testing, since I can simply write a mocked version of digitalRead and use it to precisely simulate the inputs I want my library to handle. All I need to do is declare the precise type of the digitalRead function. But what is the correct type signature of this function???

For this to have 2 different signatures for 2 different boards spells disaster for any library that wants to work for multiple boards -- it looks like I have to pick one signature or the other, and using preprocessor directives to sort this out seems like a very unsustainable way to go.

How can I encapsulate the possibly changing signature of this function in (1) an Arduino library that I write and (2) my mocks for testing Arduino libraries as part of Arduino CI?

— Reply to this email directly, view it on GitHub https://github.com/arduino/ArduinoCore-API/issues/25#issuecomment-1531632716, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANSCUI5C3ZFFPIRA74LY3VLXEEOL5ANCNFSM4GM5J24Q . You are receiving this because you were mentioned.Message ID: @.***>

SpenceKonde commented 1 year ago

Thanks for the tip on decltype()

@Michael-Brodsky hits the nail on the head - #define was horribly overused throughout the API. And where things were typed, int's were overused and uint8_t's underused.... But now it is too late to fix most of this stuff - there's too much code in the wild, and we long ago reached the point where the API was set in stone effectively. We can't go changing it without destroying compatibility short of straight-up magic, where one could shout "abracadabra!"* while brandishing an illuminated, arduino powered wand, and have all the code in existence changed to match a new API

*I've been told that this was originally an invocation of the demon-god of numbers and math; I don't know enough about such mythology to know if that's true or not, but that's the demon upon whose power you'd need to call to retroactively correct an API. Based on the absence of anyone working such magic, I conclude that the demon must demand too high a price for such alterations of reality (I certainly would if I were such a demon - it looks like a lot of work, even with unholy supernatural powers you'd expect of that sort of entity). It could also be that demons like that don't exist, and we'd need a full on time machine, and physics has pretty much constrained those out of the realm of possibility (well, for backward time travel. Forwards time travel is easy. I just used a time machine to travel forwards a half hour writing this crap)

Michael-Brodsky commented 1 year ago

😂🤣🤣😂 love it. If anyone’s interested, I’ve cobbled together a good chunk of the C++ std lib for Arduino. Have been using it for several years, mostly out of habit (once you go C++ you never go back 😅). It’s C++11 compliant (allegedly) and I think I’ve defined a lot of constants (maybe). My biggest beef has been interoperability w/ the API, specifically with untyped constants. If anyone’s interested I can send a link (no laughing).

On Tue, May 2, 2023 at 12:59 PM Spence Konde (aka Dr. Azzy) < @.***> wrote:

Thanks for the tip on decltype()

@Michael-Brodsky https://github.com/Michael-Brodsky hits the nail on the head - #define was horribly overused throughout the API. And where things were typed, int's were overused and uint8_t's underused.... But now it is too late to fix most of this stuff - there's too much code in the wild, and we long ago reached the point where the API was set in stone effectively. We can't go changing it without destroying compatibility short of straight-up magic, where one could shout "abracadabra!"* while brandishing an illuminated, arduino powered wand, and have all the code in existence changed to match a new API

*I've been told that this was originally an invocation of the demon-god of numbers and math; I don't know enough about such mythology to know if that's true or not, but that's the demon upon whose power you'd need to call to retroactively correct an API. Based on the absence of anyone working such magic, I conclude that the demon must demand too high a price for such alterations of reality (I certainly would if I were such a demon - it looks like a lot of work, even with unholy supernatural powers you'd expect of that sort of entity). It could also be that demons like that don't exist, and we'd need a full on time machine, and physics has pretty much constrained those out of the realm of possibility (well, for backward time travel. Forwards time travel is easy. I just used a time machine to travel forwards a half hour writing this crap)

— Reply to this email directly, view it on GitHub https://github.com/arduino/ArduinoCore-API/issues/25#issuecomment-1531993426, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANSCUI2D5HUIZOUSG2YS7TTXEFKPTANCNFSM4GM5J24Q . You are receiving this because you were mentioned.Message ID: @.***>

bperrybap commented 1 year ago

While the Arduino core APIs in general have quite a few issues, IMO, it goes back to the APIs being originally defined by the early Arduino.cc dev team which from my observations appeared to have little real world development and coding experience particularly in C++, but the real breakage in cases like this is due to so many people abusing the APIs by making assumptions that just are not in the API documentation, And while many of the abuses and assumptions about the API are reasonable, they are not defined in the API so they still abuse the API to create non portable code. i.e. users essentially wrote non portable code - at least according to the limited documentation.

Not defending Arduino.cc's choices, just saying the current state of things has been caused by a combination of issues on both sides. (Arduino.cc APIs, documentation, their example coding choices and Arduino users coding choices)

About the only thing Arduino.cc could do now is to either accept the most common abuses and document the APIs as actually working that way (which would be my choice) or have an IDE option to allow building in a "strict" type mode for the newer types or a compatibility mode to allow the code that abuses the API to still continue to function. or come up with a different new & better set of APIs where you have the freedom to make them better based on the 15+ years of Arduino user experiences.

Michael-Brodsky commented 1 year ago

What about incrementally working up to an ISO compliant version of the API, while still maintaining backwards compatibility? That’s one of the reasons I spent so much time implementing my std lib, that and necessity. When all else fails, I just use wrappers that meet the ISO standards. Just an idea.

On Tue, May 2, 2023 at 1:31 PM bperrybap @.***> wrote:

While the Arduino core APIs in general have quite a few issues, IMO, it goes back to the APIs being originally defined by the early Arduino.cc dev team which from my observations appeared to have little real world development and coding experience particularly in C++, but the real breakage in cases like this is due to so many people abusing the APIs by making assumptions that just are not in the API documentation, And while many of the abuses and assumptions about the API are reasonable, they are not defined in the API so they still abuse the API to create non portable code. i.e. users essentially wrote non portable code - at least according to the limited documentation.

Not defending Arduino.cc's choices, just saying the current state of things has been caused by a combination of issues on both sides. (Arduino.cc APIs, documentation, their example coding choices and Arduino users coding choices)

About the only thing Arduino.cc could do now is to either accept the most common abuses and document the APIs as actually working that way (which would be my choice) or have an IDE option to allow building in a "strict" type mode for the newer types or a compatibility mode to allow the code that abuses the API to still continue to function. or come up with a different new & better set of APIs where you have the freedom to make them better based on the 15+ years of Arduino user experiences.

— Reply to this email directly, view it on GitHub https://github.com/arduino/ArduinoCore-API/issues/25#issuecomment-1532033809, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANSCUI4OKX2GYK4XMSP72OTXEFOIVANCNFSM4GM5J24Q . You are receiving this because you were mentioned.Message ID: @.***>

ianfixes commented 1 year ago

It does seem like there is room to define an abstract class of "board support", which each individual board/architecture could implement in their own way (with appropriate methods to introspect on what is and isn't supported by particular hardware). That would provide a stable API moving forward (something I could #include and use for dependency injection & mocks), but still work (by way of #define statements) for backwards compatibility.

Number 1 rule: never abuse the API and use the APIs the way they are documented.

I didn't write Arduino CI out of an appreciation for the Arduino API as it exists, I wrote it to show the value of unit testability and Continuous Integration on open source libraries and to move that conversation forward.

I care very little about the specifics of how we ended up in the status quo. The question is whether the Arduino maintainers and volunteer library writers would find value in a more precise API (I'm hearing: "yes, there is value"), and if so, what we can do to move in that direction. If ease of maintenance for volunteer efforts isn't the goal, I'm open to hearing that argument as well.

PaulStoffregen commented 1 year ago

If ease of maintenance for volunteer efforts isn't the goal, I'm open to hearing that argument as well.

Arduino's incredible success has been attributed to design for ease of learning and lowering barriers for new users who may have little or no programming experience. Traditionally, removing novice user barriers has been of paramount importance. In the many years Arduino had a developer's mail list, over and over heated discussions involved programmers arguing Arduino should more closely follow certain programming standards. Time and time again, the Arduino developers would explain Arduino's not-so-standard design choices to simplify learning, usually commenting that numerous other far more powerful IDEs exist on the market for professional programmers who want standards compliance, and that people who choose Arduino IDE do so because it is an alternative where very different design choices were made to prioritize ease of learning.

Maybe Arduino's goals have shifted in recent years? I really don't know. They certainly are now selling "Pro" products. I don't speak for Arduino, and since the pandemic I've not been to any conferences where I've had an opportunity to speak with Arduino people in person.

But regarding what is and isn't the goal, your (perhaps rhetorical) question, traditionally Arduino's overriding design goal has been simplicity for novice users, especially removing barriers beginners and non-programmers experience while using it to accomplish their projects.

Seeing all this focus on ISO C++ standard compliance, done in a manner which causes compile errors with so many programs already published in the Arduino ecosystem, really feels like Arduino priorities & values may have shifted from the formula that paved Arduino's popularity and success in a market filled with far more advanced and standards compliant programming tools.

ianfixes commented 1 year ago

Arduino's overriding design goal has been simplicity for novice users, especially removing barriers beginners and non-programmers experience while using it to accomplish their projects

I think we are in perfect agreement here, and would point to the 5833 libraries in the library manager as evidence of how many people are working to make the novice user experience more accessible. Would you agree that the novice user is the one who ultimately benefits from improvements to the quality and breadth of libraries (in that they can avoid writing all of that code from scratch, for any given algorithm, data structure, or attached circuit)?

I'd also point out that there is an active community of developers; the forums suggest 372 forum topics per week are discussed. Would you agree that the ability for advanced users to demonstrate bugs through unit tests -- directly copy/pastable into a novice's project -- is at least as useful as an advanced user trying to explain the (mis)behavior of a code snippet in English?

TL;DR

I agree completely that removing barriers for novice users is the goal, and I don't mean to suggest any features that would detract from that goal. What I am suggesting is that at the individual level we lack continuity between the novice, intermediate, and advanced skill sets, and at the group level we lack some mechanisms that could greatly improve collaboration.

runger1101001 commented 1 year ago

Would someone have a hint on how to write a generally compatible declaration like:

PinStatus polarity = RISING;
attachInterrupt(digitalPinToInterrupt(pin), callback, polarity);

but above fails on the frameworks that don't define PinStatus :-(

int polarity = RISING;
attachInterrupt(digitalPinToInterrupt(pin), callback, polarity);

but above fails on the frameworks that do define PinStatus :-(

I'm finding it really troublesome to maintain compatibility based on checking a bunch of #if defined(TARGET_RP2040) etc as the frameworks change, and in some cases it seems to depend on the board used (e.g. Adafruit vs. Arduino) :-(

bperrybap commented 1 year ago

I would assume something like: decltype(RISING) polarity = RISING;

runger1101001 commented 1 year ago

Thank you very much @bperrybap , that was exactly what I needed!

arduino / ArduinoCore-API

Breakage caused by PinStatus and PinMode types #25

But what is the correct type signature of this function???

TL;DR