@maribu This PR is related to PR #10592 which

• implements the TwoWire Interface
• introduces Arduino.h for compatibility reasons
• tried to change the make system so that not only sketches can be compiled but also Arduino libraries.
• and of course defines again the millis() function 😄

My PR is waiting since January for merge. Maybe we can help each other by reviewing both the PRs.

@keestux, @gschorcht : I'd like to push this PR forward. Anything left to do on my side?

One question left. Would it make sense to add FEATURES_REQUIRED += periph_spi or alternatively to embed the code in #if MODULE_PERIPH_SPI ... #endif?

One question left. Would it make sense to add FEATURES_REQUIRED += periph_spi or alternatively to embed the code in #if MODULE_PERIPH_SPI ... #endif?

Better: FEATURES_OPTIONAL += periph_spi and the ifdef. This way boards without spi can still use other Arduino functions. Look at the ADC stuff.

One question left. Would it make sense to add FEATURES_REQUIRED += periph_spi or alternatively to embed the code in #if MODULE_PERIPH_SPI ... #endif?

Yes. Very much.

I think it is not possible to just do FEATURES_REQUIRED += periph_spi, as SPI support is not configurable in the Arduino world. If we would make Arduino depending on this, boards without SPI could not use any Arduino compatibility layer at all.

I would go the #if MODULE_PERIPH_SPI ... #endif route and #error in the SPI header if the module is not loaded.

Using FEATURES_OPTIONAL += periph_spi as @aabadie suggested would also make the Arduino API load SPI support by default, if existing.

I just added the #error message. As the build will there fail anyway if SPI.h is included without SPI support, I think the #ifdef ... #endif is not needed around the code.

Yes of course, FEATURES_OPTIONAL is much better. I guess, this is something we have also to do in #10592. I will do it once this PR is merged.

Testing procedure

A student of mine had to use the MFRC522 RFID card reader, for which RIOT did not yet provide a driver. I hacked together this SPI API to allow using the Arduino driver, which however still needed some modifications to work (most notably: RIOT does not support storing const char * in the flash for non-von-Neumann architecture boards). This could be reproduced to test the driver, or any other Arudino library using SPI could be used.

Would it possible to find a small Arduino library that uses SPI, for example a sensor driver, but has not be hacked to get it working in RIOT? Until now, we couldn't find an agreement in PR #12180 on how to deal with Arduino libraries which would make an import into RIOT much easier.

I 'm interested to test the SPI interface with a small Arduino lib together with PR #12180. That might become the test for this PR.

I 'm interested to test the SPI interface with a small Arduino lib together with PR #12180. That might become the test for this PR.

From my point of view, a good candidate for testing is a sensor for which we already have a driver, for example LIS3DH. I have this sensor and Adafruit provides a driver library including examples at GitHub. Since these kind of sensors always have I2C and SPI interface, the library supports both.

I will and put together PR #12118, #10592 and #12180 and create a package tomorrow that can serve as test for these 3 PRs. Unfortunatly, it will not be possible to test them separately.

@maribu Could you address the issue in static tests (Travis) in the meantime?

I will and put together PR #12118, #10592 and #12180 and create a package tomorrow that can serve as test for these 3 PRs. Unfortunatly, it will not be possible to test them separately.

BTW, I could already compile https://github.com/adafruit/Adafruit_L3GD20 successfully in that way.

@maribu What about the functions

setBitOrder()
setClockDivider()
setDataMode()

Although they are declared as deprecated, they are used by a number of existing Arduino libraries.

I know, setClockDivider() would require support by the platform. But at least setBitOrder and setDataMode could be implemented pretty easy based on your implementation. They are just modifying the settings. What do you think?

I have not yet implemented both bitorders. (I have never seen hardware seen using non-defauly bitorder.) I'm happy to add it similar to the configuration: Just asserting it to be the default bitorder and otherwise ignoring it. If someone reports to actually own and use a device with different bitorder, I'll do my best to add that feautre properly. But as far as I understand spi_acquire() would have to be extended first. (But I bet that adding the stub enforcing the default bit order will be sufficient for 99.999% of the cases.)

setClockDivider seems to be a archaic way of setting the SPI clock judging by its name without having checked the doc. I could check which value would translate to which SPI clock and simply use the closet SPI clock supported in RIOT. That way it would be hardware independent.

I'm currently away from keyboard and don't what to check the API doc of setDataMode via smartphone. But I will do so soon and think that there will also be a reasonable way to provide that API.

@maribu I get the following linking error on AVR platform when I try to compile an Arduino library:

/data/riotbuild/tests/arduino_lis3dh/bin/arduino-mega2560/arduino_lis3dh/Adafruit_LIS3DH.o: In function `Adafruit_LIS3DH::~Adafruit_LIS3DH()':

Adding -lstdc++ solves this problem for other platforms. Unfortunately, there is no libstdc++ for AVR. You have a lot experience with Arduino. Do you have any idea on how to solve this problem for AVR?

@maribu What about the functions

setBitOrder()
setClockDivider()
setDataMode()

Although they are declared as deprecated, they are used by a number of existing Arduino libraries.

Now that I have looked into the details: It is not possible to integrate these into the current approach of the SPI interface. E.g. the Arudino SPI interface should be using by running

SPI.begin()
SPI.beginTransaction(spi_settings);
// send and receive data via SPI
SPI.endTransaction(spi_settings);
// and optionally:
SPI.end();

It could however also be using like this:

SPI.begin()
// send and receive data via SPI
// and optionally
SPI.end();

In the latter case the user is in charge to make sure that concurrent contexts do not access the SPI bus at the same time - which means two devices on the SPI bus can only be used, if both drivers use the transaction flavor of accessing the bus. Also, as SPI.end() is fully optional and often never called at all, power management is a mess.

Therefore, the current implementation just forces that beginTransaction() and endTransaction() are used and maps them to spi_acquire() and spi_release() to solve all issues with the Arduino interface. However, the deprecated call given will (hopefully) never be used together with beginTranscation(), as beginTranscation() takes the SPI config as argument. (It would be quite insane to change the config in the middle of an transaction - therefore I think it is hopefully not used together with beginTransaction().)

So providing the deprecated API is currently not possible with this implementation.

I could however change the approach like this:

• beginTransaction():
  1. Lock an rmutex
  2. set a flag to indicate an ongoing transaction
  3. call spi_acquire().
• transfer() (and friends):
  1. Lock the rmutex
  2. If the flag to indicate an ongoing transaction is not set, call spi_acquire()
  3. Transfer the data
  4. If the flag to indicate the ongoing transaction is not set, call spi_release()
  5. Unlock the rmutex
• endTransaction():
  1. Clear the ongoing transaction flag
  2. Call spi_release()
  3. Unlock the rmutex

It would be more overhead, but fully compatible. I assume you would prefer this approach?

OK. rmutex currently cannot be used from C++, because C++ is not compatible with C11 atomics. One would need a way to define the API interface to C++ without every using C11 atomics in the header, if included from C++. I have to think about this for a while...

I think it's better to leave the API as it is. Everything else could become too tricky. That's not worth it for old-style libraries.

Hmm, I have completed the implementation by now. Only the issue with rmutex needs to be solved - but that should be addressed regardless of the issue at hand, as C++ developers might have use for that.

One problem I run into when I try to use https://github.com/adafruit/Adafruit_LIS3DH as test library is that the statement in the library

SPIinterface->beginTransaction(SPISettings(2000000, MSBFIRST, SPI_MODE0));

doesn't work correctly on ESP32. Although I believe that it is unrelated to this PR, it is pretty weird.

In the SPISettings constructor, clock is set correctly to SPI_CLK_1MHZ or 2. But when spi_acquire is called in beginTransaction method

void SPIClass::beginTransaction(SPISettings settings)
{
    /* Call spi_acquire first to prevent data races */
    int retval = spi_acquire(spi_dev, SPI_CS_UNDEF,
                             settings.mode, settings.clock);
   ...
}

settings.clock is very often 7 or any arbitrary value, e.g., 1073106944, but never 2. This would mean that the copy constructor doesn't work correctly. Providing an explicit copy constructor didn't help.

Any idea?

Unfortunatly, I have no other platform, for which I could test it:

• Arduino Mega2560 does not support module cpp
• Arduino Due does not support periph_i2C
• ESP8266 does not support module cpp
• Nucleo F411RE does not support arduino module

The only platform that I have which support all modules is ESP32 😟

OK. Now the legacy SPI interface should work as well, with the same conceptional bugs and hazards the real Arduino API has. (That is: When not surround transactions with beginTransaction() and endTranscation(), every other driver might get access to the SPI bus in the meantime and break the communication.)

OK. Now the legacy SPI interface should work as well, with the same conceptional bugs and hazards the real Arduino API has.

The test with PR #12180 and #12518 still works.

@maribu Please squash.

@maribu Please squash.

Done

@maribu We should also remove Implement SPI interface class from ToDos section in sys/arduino/doc.txt. Add support for the 'Wire Library' (I2C) should also be removed.

Do you think that it would be possible to remove both items with your PR maybe as separate commits? Otherwise, I would have to provide a PR for only removing this one item.

Do you think that it would be possible to remove both items with your PR maybe as separate commits?

Done

Do you think that it would be possible to remove both items with your PR maybe as separate commits?

Done

Thanks.

Please fix the errors reported by Travis and squash afterwards.

@maribu Thanks for providing the Arduino SPI library.

Thanks for the review!