Arduino-CI / arduino_ci

Unit testing and Continuous Integration (CI) for Arduino libraries, from a Ruby gem
Apache License 2.0
110 stars 34 forks source link

Dependency problem on AVR specific #include. #251

Open RobTillaart opened 3 years ago

RobTillaart commented 3 years ago

Hi,

am struggling with a library - https://github.com/RobTillaart/DS18B20_INT

The library needs the "OneWire: so I adapted .arduino-ci.yml

compile:
  # Choosing to run compilation tests on 2 different Arduino platforms
  platforms:
    - uno
    - leonardo
    - due
    - zero
  # Declaring Dependent Arduino Libraries (to be installed via the Arduino Library Manager)
  libraries:
    - "OneWire"

unittest:
  # These dependent libraries will be installed
  libraries:
    - "OneWire"
    - "util/crc16"   <<<<<<<<<<<<<<<<<<< does not work.

The example of the lib now compiles correctly, however the unit test fails on util/crc16.h  which is used by the OneWire lib. The path (on my PC) is   ...\Arduino\hardware\tools\avr\avr\include\util\crc16.h  but I do not know how to include it in the unit test.

It might be a feature that need to be solved in the future.

For now I will disable the unit test.

ianfixes commented 3 years ago

Can you post the output from the unit test run? The libraries that you list in the config file refer to the names that the library manager would use, not paths on disk. If utl/crc16.h isn't being made available to the unit tests, that's the real bug here.

RobTillaart commented 3 years ago

LOG_DS28CM00_LIB.zip

This log is from the DS28CM00 library [this is an ID chip] - https://github.com/RobTillaart/DS28CM00/tree/arduino-ci

The problem 3 is expected and need to be fixed. I will merge [2] and [3] and use conditional compilation. When I get ESP32 to work it will be tested too. (another story).

ianfixes commented 3 years ago

@per1234 does the use of utli/crc16.h or rom/crc.h fall under either "undocumented API" or "platform-specific feature"?

I'm tempted to close this as "works as intended", with limiting the set of test platforms & adding preprocessor directives being the proper remedy. But I want to double check that assumption.

per1234 commented 3 years ago

"undocumented API"

util/crc16.h

it's definitely documented: https://www.nongnu.org/avr-libc/user-manual/group__util__crc.html

rom/crc.h

I'm not sure. I see there is doxygen documentation in the source code: https://github.com/espressif/esp-idf/blob/v3.3.4/components/esp32/include/rom/crc.h but I didn't find that documentation published anywhere. I really don't have much experience with the ESP32.

"platform-specific feature"

It looks like both of these are platform-specific.

ianfixes commented 3 years ago

Hmm. I'll probably deprioritize this then, sorry Rob.

My reasoning here is that it seems fruitless to try and replicate all functions that might exist in any platform library (vs existing in the "core" set of Arduino library functions). I had enough trouble with the AVR side of things, and don't want to go further down that path unless/until I can find a smarter way to do it.

I can revisit this if there is movement on https://github.com/arduino/arduino-cli/issues/1090 or https://github.com/arduino/arduino-cli/issues/1092

RobTillaart commented 3 years ago

Hmm. I'll probably deprioritize this then, sorry Rob.

OK, but as the OneWire library is used for several devices / libraries, all these will be affected. So please do not close.

If I find a workaround I will let you know.

ianfixes commented 3 years ago

I won't close it. It's definitely something that needs to be addressed and the good news is that there are a few ways to address it. I just have to figure out which of the possible workarounds is the appropriate one and/or whether the approach in OneWire is (or isn't) considered a best practice.

ianfixes commented 3 years ago

@per1234 regarding "undocumented API", would you say that something like portModeRegister(P) is undocumented? I see it listed here: https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/Arduino.h

But what assurance do I have that this macro will always be provided by the core? My brief google search for anything in the online arduino reference documentation was unsuccessful.

per1234 commented 3 years ago

This is definitely a problem. It's difficult to say whether some of these things are undocumented because nobody ever got around to documenting them (or even creating a place to document them, since this is not something you would want to expose a beginner to), or if it was only ever intended for internal use by the core library.

There is a request for a more comprehensive documentation of the platform interface here: https://github.com/arduino/arduino-cli/issues/985 There is also an interesting related discussion about breakage the transition to using ArduinoCore-API in ArduinoCore-samd caused to some code that relied on the undocumented interfaces: https://github.com/arduino/ArduinoCore-samd/issues/587

RobTillaart commented 3 years ago

It's difficult to say whether some of these things are undocumented because nobody ever got around to documenting them

Arduino was intended to learn people to do fun stuff, this level of coding was explicitly the level they wanted to hide, not only for beginners but for everyone.

For the libraries / unit test, I can live perfectly with some things that won't work in a unit test. Should not be too much of course, but when developing I often depend on testing with real hardware (sensor etc). The unit tests are most valuable for PR's fro 3rd parties to check they didn't break too much.

For me the compilation of examples are the top priority, as these are the starting points for the users of the libs.