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: Added Wire (I2C) interface #10592

Merged
merged 2 commits into from
Oct 24, 2019

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Dec 11, 2018

Contribution description

This PR provides an implementation of the Arduino Wire library.

Testing procedure

See PR #12518

Issues/PRs references

The changes in PR #7654 might require changes of the handling of the pseudomodule arduino_lib.

@gschorcht gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers Area: arduino API Area: Arduino wrapper API labels Dec 11, 2018
@smlng smlng self-assigned this Dec 12, 2018
@smlng
Copy link
Member

smlng commented Dec 12, 2018

Cool! I'll try to have a closer look and test this soon(ish).

@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
@gschorcht gschorcht force-pushed the sys_arduino_ext branch 2 times, most recently from 5d1dbbf to 9ae0c73 Compare January 2, 2019 13:46
smlng
smlng previously requested changes Jan 22, 2019
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

some minor stuff, otherwise looks good ... needs testing though

@gschorcht
Copy link
Contributor Author

Let's pickup the game again. I had an email conversation with someone who was asking whether it is possible to use Arduino driver libraries for I2C sensors in RIOT. Having the Wire interface library would be a step in this direction.

In addition to the wire library, we need a common approach to automatically import Arduino libraries as packages. I'll think about such an approach as soon as I have some time left.

@gschorcht
Copy link
Contributor Author

@cladmi When I was trying to rebase this, I ran into conflicts since the make system has been changed with PR #7654. With this PR, I tried to define a pseudomodule arduino_lib so that it became possible not only to compile a complete sketch but also some Arduino libraries. Do have any idea, how I could do the same with the new make system as defined in PR #7654?

@cladmi
Copy link
Contributor

cladmi commented Sep 5, 2019

Is the following the behavior your want?

If somebody wants the sketch, it does not use your lib.
If somebody wants your lib, it does not use the sketch.
Both sketch and lib depend on a "common" arduino part.

If this is the case, I think the current arduino module should be split between the sketch and the common that you also use.

So either rename it to arduino_sketches, and when an application that wants it, should say USEMODULE += arduino_sketches, and put the common as arduino.
Or it could be arduino == sketches, arduino_lib your thing, arduino_common common things required by both (if needed).

That depends on what is wanted to give as an interface to users.

Currently there was an automatic arduino is implemented by arduino_sketches that was done in the sys/arduino/Makefile.include and this goes against your need as it should be defined in Makefile.dep if keeping the current naming.

Depending on the naming wanted, I can do the change.

@gschorcht
Copy link
Contributor Author

@cladmi Thanks for explaination. I will restructure the PR a bit. I will leave this PR for the implemenation of Wire (I2C) implentation and will prepare a new PR for the make system.

@gschorcht gschorcht changed the title sys/arduino: extension to Arduino module sys/arduino: Added Wire (I2C) interface Sep 5, 2019
@gschorcht
Copy link
Contributor Author

@cladmin The situation was the following:

You have a directory with an Arduino application, for example examples/hello-world-arduino. The Makefile enables the arduino module

USEMODULE += arduino

In this case, the make system expects a file with extension .sketch from which the file _sketch.cpp is generated and compiled afterwards.

However, when you have an Arduino library, the directory usually contains only a .h and a .cpp file, for example:

pkg/vl53l1x_arduino/VL53L1X.cpp
pkg/vl53l1x_arduino/VL53L1X.h

The compilation of such a directory requires same compiler configuration as for an Arduino application using sys/arduino library.

So the idea was just to declare a pseudomodule arduino_lib in addition to the arduino psdeudomodule. The makefile would then enable

USEMODULE += arduino_lib

which enables module arduino implicitly. The make system wouldn't try to generate _sketch.cpp from a .sketch file.

Therefore I used the following construct in former sys/arduino/Makefile.include

- $(shell $(RIOTTOOLS)/arduino/pre_build.sh $(SRCDIR) $(APPDIR) $(SKETCHES))
+ # run the Arduino pre-build script if Arduino module is not used as library
+ ifeq (, $(filter arduino_lib, $(USEMODULE)))
+     $(shell $(RIOTTOOLS)/arduino/pre_build.sh $(SRCDIR) $(APPDIR) $(SKETCHES))
+ endif

and in sys/Makefile.dep

+ ifneq (, $(filter arduino_lib, $(USEMODULE)))
+     USEMODULE += arduino
+ endif  

It would be really cool if it would be possible to import Arduino libraries as packages and to compile them since there are thousands of drivers available for Arduino IDE.

@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 Sep 5, 2019
@maribu
Copy link
Member

maribu commented Sep 5, 2019

@gschorcht: You didn't have reacted to some open comments of @smlng yet. Otherwise, I'd test and (hopefully) ACK.

@gschorcht
Copy link
Contributor Author

@smlng Could you approve this PR? All your change request were addressed, either changed or you gave thumbs up for the comment.

@smlng smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 22, 2019
@smlng smlng dismissed their stale review October 22, 2019 09:40

comments addressed, testing needed

@smlng
Copy link
Member

smlng commented Oct 22, 2019

code-wise this looks good to me, can somebody tests this please?

@gschorcht
Copy link
Contributor Author

@maribu Could you test it? If you don't have LIS3DH at hand, just tell me what other I2C device you have and I can prepare a test.

@maribu
Copy link
Member

maribu commented Oct 22, 2019

Sorry, not yet. I do not have a LIS3DH. I have BME280, BME680, SHT30, and VL53L0X at hands.

@gschorcht
Copy link
Contributor Author

gschorcht commented Oct 23, 2019

Sorry, not yet. I do not have a LIS3DH. I have BME280, BME680, SHT30, and VL53L0X at hands.

I have attached a test application with an Arduino library for SHT31D, which also works for SHT30. I tested it with arduino-mega2560. The library is of poor quality but uses very intense I2C. Temperature and humidity are casted to int for output because of the lack of floating point outputs.

For the test you have to merge this PR and PR #12180 into a test branch in which you extract also the attached archive.

tests_arduino_sht31d.zip

@maribu
Copy link
Member

maribu commented Oct 23, 2019

Tested on an Arduino Nano:

Output of the terminal with incorrect I2C address:

2019-10-23 20:02:08,224 # main(): This is RIOT! (Version: 2020.01-devel-197-g1de8b-g-ard)
2019-10-23 20:02:08,258 # ClosedCube SHT3X-D Periodic Mode Example
2019-10-23 20:02:08,291 # supports SHT30-D, SHT31-D and SHT35-D
2019-10-23 20:02:08,292 # Serial #0
2019-10-23 20:02:08,454 # [ERROR] Cannot start periodic mode
2019-10-23 20:02:08,462 # Periodic Mode: [ERROR] Code #-40
2019-10-23 20:02:08,745 # Periodic Mode: [ERROR] Code #-40
2019-10-23 20:02:09,029 # Periodic Mode: [ERROR] Code #-40
...

Capture with logic analyzer (still incorrect I2C address):

Picture of I2C Capture

Output of the terminal with I2C address matching device:

2019-10-23 20:09:31,983 # main(): This is RIOT! (Version: 2020.01-devel-197-g1de8b-g-ardJ�(���Չ��SHT3X-D Periodic Mode Example
2019-10-23 20:09:32,017 # supports SHT30-D, SHT31-D and SHT35-D
2019-10-23 20:09:32,019 # Serial #28566
2019-10-23 20:09:32,150 # Periodic Mode: T=23C, RH=63%
2019-10-23 20:09:32,430 # Periodic Mode: T=23C, RH=63%
2019-10-23 20:09:32,710 # Periodic Mode: T=23C, RH=63%

So: Errors seem to be correctly reported to the libraries (first try with incorrect address) and with correct address it worked as advertised.

@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

@smlng: I'd say squash, let Murdock run once more and than it could be merged from my point of view.

@smlng
Copy link
Member

smlng commented Oct 23, 2019

please squash

@gschorcht
Copy link
Contributor Author

Squashed

@maribu @smlng @haukepetersen Just for the sake of completeness. Wire is derived public from Stream. This PR only implements the methods that are defined in class Wire. Theoretically, any other public method of Stream could be used in Arduino code. These are:

find()
findUntil()
readBytes()
readBytesUntil()
readString()
readStringUntil()
parseInt()
parseFloat()
setTimeout()

But the only method from Stream I have ever seen for I2C access is readBytes which is therefore already implemented in this PR.

We should have this in mind and should implement further functions if there is any request for them. Furthermore, Serial is also derived from Stream. If there will be further requests for function in future, we should think about to implement a Stream class and to derive Wire and Serial from it.

@maribu
Copy link
Member

maribu commented Oct 24, 2019

@gschorcht: Thanks for pointing that out.

+1 for not increasing the problem space of this PR. If there really is need for also using the Stream methods for I2C, this should be a follow up PR in my opinion. But I personally would not implement that until someone stumbles upon a case it is actually needed. In my opinion it would be a shame to spend effort to add an API that in the end no one ever uses.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. If the pointer with the Stream API did not raise any new concern, I'd say merge!

@gschorcht
Copy link
Contributor Author

ACK. If the pointer with the Stream API did not raise any new concern, I'd say merge!

As I said, I have never seen problems with the Stream class that would block this PR. It was just a comment that it might become necessary in future for enhanced compatibility to think about that class and of course its derivatives.

@maribu
Copy link
Member

maribu commented Oct 24, 2019

OK, no one complaint. Let's merge!

@maribu maribu merged commit 1c624e4 into RIOT-OS:master Oct 24, 2019
@maribu
Copy link
Member

maribu commented Oct 24, 2019

@gschorcht: Thanks for this PR. A lot of students are familiar with Arduino. A feature rich compatibility layer could make it easier to get them on board 👍

@gschorcht
Copy link
Contributor Author

@maribu Thanks for your support. Lets's hope we get merged your SPI and the arduino_lib build soon.

@gschorcht gschorcht deleted the sys_arduino_ext branch October 24, 2019 16:49
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: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants