DIT112-V20 / group-09

Apache License 2.0
4 stars 1 forks source link

Current implementation would straight up crash on Arduino API calls at dynamic init #34

Closed AeroStun closed 4 years ago

AeroStun commented 4 years ago

(Issue opened here since the discussion will have a direct impact on whether we implement a fix or not) @platisd I can't help but notice that in your examples for smartcar_shield at this location you declare a global variable which gets initialized at dynamic init (https://eel.is/c++draft/basic.start#dynamic), which results in this other location to be run, itself causing this last location to invoke the Arduino IO API function pinMode.

Now, does the Arduino framework guarantee that IO API functions will work as expected when invoked at dynamic init? If not, is it simply always the case despite no guarantees from the framework? The C++ standard only mandates expected behavior for its iostreams (which aren't required on standalone implementations like often found on embedded anyway).

The main question boils down to: while implementation-defined regarding the C++ standard, is the support for hardware IO API calls at dynamic initialization widespread and relied-upon enough that it would be reasonable and more or less expected of us to go out of our way to support that scenario?

Thanks in advance for your time and your input; your experience with embedded is precious

[While we're at it, since I dug a bit on how you manage your runtime global in smartcar_shield, I ought to inform you that if someone were to write for a single program multiple C++ translation units which all include <Smartcar.h> to get access to your library headers, they would get multiple definitions of arduinoRuntime therefore breaking ODR and therefore failing to link; you didn't document this anywhere, so I assume it's a bug.]

platisd commented 4 years ago

Now, does the Arduino framework guarantee that IO API functions will work as expected when invoked at dynamic init? If not, is it simply always the case despite no guarantees from the framework?

It's the latter (in bold). At least for the AVR and Espressif platforms that we support. It is done so to eliminate sequential coupling of the Odometer classes APIs. This was introduced in the last library revision. To use the Odometers before, one had to call an Odometer::begin function in setup, which I disliked because aside of the sequential coupling it was also inconsistent with how the other classes worked.

I ought to inform you that if someone were to write for a single program multiple C++ translation units which all include to get access to your library headers, they would get multiple definitions of arduinoRuntime therefore breaking ODR and therefore failing to link; you didn't document this anywhere, so I assume it's a bug.

Well, it's not really a bug since writing multiple translation units including Smartcar.h is not a possible/normal/easy use case for Arduino. That's why it is not documented. I didn't even consider it a scenario! :smile: However, if someone would migrate this to another platform, then they should create and use a different header file, e.g. Smartcar_yourplatform.h. This is implied here:

  • The entrypoint for the Smartcar platform on boards that are compatible with
    • the Arduino API.

Perhaps it should be more explicit about what should happen on different platforms or add the requirement to "the Arduino IDE way of doing things" too. Anyhow, if that'd be the case and someone was working on such migration, i'd also consider moving the smartcarlib::pins into a separate header file that's to be included by the "main" header file(s). I'll probably do that anyway at some point (perhaps along the change I'm going to mention below).

Other than that, this "hack" we're doing with the ArduinoRuntime so it can be injected "automagically" is... well, a hack and a code-smell. It is there ultimately for convenience and I am considering removing it and have the users manually inject a Runtime instance to their classes, as it's being done in the unit tests for example. I think that since there are so many examples anyway and everyone's copying them to get started, it should not be a problem.

AeroStun commented 4 years ago

Okay; thanks for the insightful info! Then I'll make so it works as best as we can make it (for your odometers objects it'll definitely work)

platisd commented 4 years ago

Coincidentally, I was just reading Arduino's official "how to make a library" article and they also do this, i.e. call pinMode before setup() in the Morse::Morse(int pin) constructor.

AeroStun commented 4 years ago

\<rant> I would love if Arduino actually made a list of requirements for Arduino cores; right now it seems like implementers are supposed to guess what users can rightfully assume to work across all Arduino-compatible systems. \<\/rant>