Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/arduino: Using Arduino libraries #12180

Merged
merged 3 commits into from
Nov 22, 2019
Merged

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Sep 9, 2019

Contribution description

This PR allows the use of an Arduino library in RIOT applications without the need for an Arduino sketch. For this purpose it introduces the pseudomodule arduino_lib. This pseudomodule enables implicitly module arduino but avoids that a sketch is required or generated and compiled. Thus, it becomes possible just to compile and use a package or a directory with some source files from an Arduino library in RIOT applications without having an Arduino sketch. The application has to enable the arduino_lib module instead of the arduino module.

The test app tests/arduino_lib demonstrates how to import and use Arduino libraries as packages using a very simple Arduino library. For this purpose, the PR contains the package talking_leds for on-board LED flashing sequences.

In addition, this PR introduces the Arduino standard header file Arduino.h and the data types boolean and byte to improvethe compatibility of sys/arduino with Arduino libraries and applications.

Testing procedure

Compile and flash tests/sys_arduino_lib:

make -C tests/sys_arduino_lib BOARD=... flash

The integrated LED should flash in a loop twice short and twice long.

Issues/PRs references

@gschorcht gschorcht added Area: arduino API Area: Arduino wrapper API Area: sys Area: System Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. labels Sep 9, 2019
@gschorcht gschorcht changed the title sys/arduino: use Arduino libraries sys/arduino: Using Arduino libraries Sep 9, 2019
@gschorcht gschorcht force-pushed the sys/arduino/lib branch 2 times, most recently from d8b8c64 to b186c23 Compare September 9, 2019 00:31
@gschorcht
Copy link
Contributor Author

gschorcht commented Sep 9, 2019

@cladmi @haukepetersen What do think about this approach. Maybe, there could be a solution in future for pulling Ardunio libraries into the build directory of an application instead of having a package for each of the thousands Arduino libraries. But for the moment, the proposed approach helps to use the pool of Arduino drivers.

BTW, I have used this approach in PR #10363 together with PR #10592 to use a pretty complex Ardunio driver for the VL53L1X ToF ranging sensor which worked like a magic with any changes in the Arduino code.

@kaspar030
Copy link
Contributor

wow, this seems so useful!

tests/arduino_lib/main.cpp Outdated Show resolved Hide resolved
tests/arduino_lib/main.cpp Outdated Show resolved Hide resolved
@cladmi
Copy link
Contributor

cladmi commented Sep 12, 2019

I do not like the ifeq (,$(filter arduino_lib,$(USEMODULE))) because I think one may still want to use the arduino libraries with a sketch written application.

I would prefer to introduce another module to differentiate between "arduino" that gives the headers and pulls the required features and modules, and a module to says use "arduino_sketches".

I can do a change for this, I just do not know what would be preferred between:

  • making the common part be called arduino and changing the current usage/api to say "if you use the sketch, you must say USEMODULE += arduino_sketches".
  • Keep the current api of "say USEMODULE += arduino to use the sketches" and the common be called arduino_common or arduino_api something

That really depends on the usage.

http://doc.riot-os.org/group__sys__arduino.html
http://doc.riot-os.org/group__sys__arduino__api.html

@gschorcht
Copy link
Contributor Author

@cladmi It works in exactly that way with the ifeq (,$(filter arduino_lib,$(USEMODULE))). With both modules arduino and arduino_lib, sys/arduino is compiled and sys/include/arduino is added to the include path. That is, in both cases, one can compile Arduino libraries. But in case of module arduino, an Arduino sketch is reuqired in addition. That is, the only difference between both modules is that a sketch must exist for module arduino.

If you prefere, we can use arduino_api instead of arduino_lib to make clear that the Ardunio API is used in that case.

@gschorcht
Copy link
Contributor Author

@cladmi It would be good if one module is realized on top of the other module to have only one place where we define include pathes, compiler flags, a.s.o., for the Arduino stuff.

@cladmi
Copy link
Contributor

cladmi commented Sep 12, 2019

Ohh my bad, I thought there were 3 things, when there are 2.
I will look again more precisely tomorrow.

With your naming, somehow the Makefile.dep should do the opposite, that arduino depends on arduino_lib (the common one) that requires the features and the include.
And then adapt sys/Makefile.include to include it on arduino_lib.
So no need for the ifeq but would be, as usual ifneq(,sketches_requested).

Somehow I think it just makes sense to say "use arduino_sketches" when you want the sketches generated and arduino when you want the library.

@haukepetersen as you added this, what do you think about it?

@cladmi
Copy link
Contributor

cladmi commented Sep 12, 2019

I am stupid, we could only build the 'arduino_sketches' module if there are '.sketches' files.

@keestux
Copy link
Contributor

keestux commented Sep 15, 2019

What could be the reason that the test program hangs after 727 loop iterations?

@gschorcht
Copy link
Contributor Author

gschorcht commented Sep 16, 2019

@keestux I have no idea. Maybe it is problem of the TalkingLED library and the handling of the delay. What platform did you use to test? I have tested it with an ESP32 board, and haven't seen this problem.

@keestux
Copy link
Contributor

keestux commented Sep 17, 2019

Ah, found the reason for the hang. millis() has a wraparound roughly after 4294967 milliseconds, UINT32_MAX / 1000. Unrelated to this PR.

@keestux
Copy link
Contributor

keestux commented Sep 19, 2019

@gschorcht Did you run the test program for longer than 4295 seconds? I assume it would hang on ESP32 as well.

@gschorcht
Copy link
Contributor Author

@kee

Ah, found the reason for the hang. millis() has a wraparound roughly after 4294967 milliseconds, UINT32_MAX / 1000. Unrelated to this PR.

@keestux Thanks for figuring out the problem and the fix.

@keestux keestux self-requested a review October 7, 2019 20:10
@keestux
Copy link
Contributor

keestux commented Oct 7, 2019

Shall we go ahead with this one? It looks OK to me.

@keestux keestux mentioned this pull request Oct 8, 2019
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keestux pointed out this PR in #12386 so I had a quick look (without testing).

I like the proposed approach of this PR, it makes the reuse of Arduino libraries in existing RIOT applications code very simple and compatible.

For simplicity, the arduino_lib module could be pull-in automatically by the package.

I also found some typos and the test application README still has to be written. See my comments below.

Otherwise, it looks good.

sys/arduino/Makefile.include Outdated Show resolved Hide resolved
pkg/talking_leds/doc.txt Outdated Show resolved Hide resolved
tests/arduino_lib/Makefile Outdated Show resolved Hide resolved
tests/arduino_lib/README.md Outdated Show resolved Hide resolved
sys/arduino/include/arduino.hpp Outdated Show resolved Hide resolved
@gschorcht
Copy link
Contributor Author

@aabadie Thanks for reviewing, I will provide some changes later today.

Shall we go ahead with this one? It looks OK to me.

@keestux I didn't continue working on this PR for the moment since @cladmi suggested in #12180 (comment) and #12180 (comment) a slightly different approach but asked @haukepetersen for his opinion first.

I will have a look later today.

@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Oct 23, 2019
@maribu
Copy link
Member

maribu commented Oct 23, 2019

Didn't look at the code, but I used it to test #10592 and it worked like a charm

@gschorcht gschorcht removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. labels Oct 28, 2019
@gschorcht
Copy link
Contributor Author

Even though @cladmi proposed to think about a change of the make system so that the sketch is only compiled if it exist, this PR is compatible with current master and does the job it should do. Maybe, we should make it productive.

@gschorcht
Copy link
Contributor Author

gschorcht commented Nov 7, 2019

Finally, I was able to change sys/arduino/Makefile.include in the manner suggested by @cladmi. It now first sets the SKETCHES variable by looking for existing sketch files. Only if there sketch filesm the other variables are defined that lead to the compilation of the sketch. In all other cases, only the include path for Arduino headers is defined.

This has the advantage that we can use the existing dummy module arduino to compile an Arduino sketch as well as an Arduino library. The new dummy module arduino_lib could be removed again.

@aabadie All your change requests were addressed.
@maribu Could you test it again? The test app tests/sys_arduino_lib should work. Thanks.

@maribu
Copy link
Member

maribu commented Nov 7, 2019

@maribu Could you test it again? The test app tests/sys_arduino_lib should work. Thanks.

Done. The "TalkingLED" Arduino Lib builds on the Arduino Uno and the LED blinks as described in main.cpp.

@gschorcht
Copy link
Contributor Author

@maribu Could you test it again? The test app tests/sys_arduino_lib should work. Thanks.

Done. The "TalkingLED" Arduino Lib builds on the Arduino Uno and the LED blinks as described in main.cpp.

Thanks.

boolean and byte are data types defined in Arduino.h and very often used in Arduino code.
@gschorcht gschorcht added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 21, 2019
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice now. The integration with the build system is very light.

I have no more comment and tested this PR on arduino-zero: it works like a charm.

Please squash!

To make it possible to use an Arduino library, a new pseudomodule arduino_lib is introduced. This pseudomodule enables implicitly module arduino but avoids that a sketch is required or generated and compiled. Thus, it is possible to compile and use a package or directory with some source files from an Arduino library in RIOT applications.
This test application defines a packages which imports a very simple Arduino library that is used by test application to demonstrate how an Arduino library can be imported as package and used by an application.
@gschorcht
Copy link
Contributor Author

Murdock is now happy.

@aabadie aabadie merged commit c7cd3d8 into RIOT-OS:master Nov 22, 2019
@gschorcht
Copy link
Contributor Author

Thank you for your support with all these PRs.

@gschorcht gschorcht deleted the sys/arduino/lib branch November 22, 2019 15:24
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
@jdavid jdavid mentioned this pull request Oct 15, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: arduino API Area: Arduino wrapper API Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 3-testing The PR was tested according to the maintainer guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants