cpu/native: Allow Access to Hardware SPI Bus on Linux

[fhessel]

Contribution description

Motivation: For my thesis, I need to be able to run the same code on a testbed consisting of microcontrollers as well as SoC platforms like the Raspberry Pi (not on bare metal but as process in Linux userspace, so I'll use the native cpu in RIOT).

Problem: The native cpu/board cannot make use of the underlying hardware, even if the CPU does support peripherals like SPI.

Suggested Solution: I wrote a wrapper around the Linux userspace SPI API that allows to map the devices from /dev/spidevB.D to SPI devices in RIOT. Basically, these device files represent SPI busses, where the first number (B) represents the bus itself, and the second number (D) represents the connected device / hardware CS line. If the RIOT application is compiled for Linux with the PERIPH_SPI feature being required, it will provide the --spi command line parameter, which can be used to dynamically map those device files to the application. For example, calling:

./my-application --spi=0:0:/dev/spidev0.0 --spi=0:1:/dev/spidev0.1

... will allow to use SPI_DEV(0) in RIOT with either SPI_HWCS(0) or SPI_HWCS(1). Using arbitrary lines for CS isn't possible until native also has access to GPIOs (related PRs #1737 or #7530 seem to be stuck, and #8048 is more or less raspi-only and also possibly outdated).

In addition to my use case, I think that SPI access under native might as well become handy for development and debugging on device drivers.

Testing procedure

Board: Native CPU/Board on a Linux-based SoC with hardware SPI. I used a Raspberry Pi, where the pinout for Rev. 2 and 3 would be: more details

Function	Pin (physical)	Device File
MOSI	19	both
MISO	21	both
SCLK	23	both
CE0	24	/dev/spidev0.0
CE1	26	/dev/spidev0.1

This means we have one physical bus with two hardware-based CS lines.

Loopback test

The easiest way would be a loopback test with the periph_spi test, as it does not require any additional hardware. For the Raspberry Pi, one would connect MISO and MOSI with each other. Then, in the periph_spi directory:

BOARD=native make all
./bin/native/tests_periph_spi.elf --spi=0:0:/dev/spidev0.0  --spi=0:1:/dev/spidev0.1

And in the application's shell:

init 0 0 2 -1 0
send test

... or init 0 0 2 -1 1 for the seconds CS line.

This should output something like this:

> send test
send test
Sent bytes
   0    1    2    3 
  0x74 0x65 0x73 0x74
    t    e    s    t 

Received bytes
   0    1    2    3 
  0x74 0x65 0x73 0x74
    t    e    s    t 

Further Testing

Any further tests, especially with real SPI hardware, are a bit complicated as most drivers also require GPIO functionality in addition to SPI (for CS, RST or IRQ lines), which is not supported by now. I did most of my tests with a LoRa GPS Hat which provides a SX1276-compatible transceiver. However, the sx127x driver also requires GPIO, so I ported some of my former code to RIOT. The code can be found here, if that helps testing in any way.

I did the best I could to find errors in the code, but if you have any suggestions how I could verify the implementation more formally, a hint would be appreciated.

Other Build Environments

I also used a VM with FreeBSD to verify that the non-Linux build does not break (unless you require PERIPH_SPI as feature). However, due to the lack of hardware, I couldn't verify it for macOS right now.

Issues/PRs references

Besides the aforementioned PRs regarding GPIOs, I did not find any Issues/PRs related to SPI on native.

[kaspar030]

@fhessel thank you for your PR and welcome to the RIOT community! 😉

The PR already looks quite nice. I'll try to test it ASAP.

[kaspar030]

Found some minor style issues.

[fhessel]

@kaspar030 Thank you for the feedback! If I can anyhow support testing, just let me know. I addressed the styling issue already.

Regarding the failing CI build, I assume that the failing tests are now run for the first time for the native CPU (all of them are for drivers which require PERIPH_SPI as feature, which is now available). As far as I could see, the reason for the failure is mostly using char as type for byte constants or assigning macros which expand to 1-byte expressions to chars, with chars being signed on native. This leads to the overflow in implicit constant conversion seen in the CI log.

The only other issue that I could see was a binary constant in tests_driver_adt7310 which creates an error with this toolchain as well.

As that's not directly related to the suggested change, maybe you could suggest what's the best way to address this.

[fhessel]

@kaspar030 Is there there anything else I can or should do to push the PR forward?

The style issues should be resolved, and for the Murdock failure, I again had a look at the output, with this being my interpretation of it:

tests/driver_adt7310
The test has only a problem with this line in adt7310.c using the gnu toolchain (first failure) or llvm (second failure):

#define ADT7310_EXPECTED_MANUF_ID (0b11000000)

which could easily be changed to using its hexadecimal representation: (0xC0).

tests/driver_pcd8544
The problem here is this array used by pcd8544_riot(...) to draw a sample image on the display, which is passed to pcd_write_image(const pcd8544_t*, const char*) and eventually is casted to uint8_t in the internal _write(...) function. So I assume that the _riot char array is intended to be used as an array of unsigned bytes anyway, but that would also implicate changes to the API of that driver, as the char type is used there as well. (The character table is already using uint8_t).

tests/driver_sdcard_spi
Again, these failures occur because the result of a macro expansion of e.g. SD_CARD_DUMMY_BYTE or SD_INVALID_R1_RESPONSE is passed to a function that expects a char parameter. For values bigger than 0x7F, this generates an error because of a potential overflow.

So I'd suggest:

• Changing the binary to a hexadecimal constant for ADT7310_EXPECTED_MANUF_ID (this should not cause any trouble at all)
• Changing the types of the parameters of the drivers in question from char to uint8_t, which should also conform the Types chapter of the Coding Convetions. If the code could be compiled without error for other platforms by now, I assume that they already use an unsigned char type and this should not change anything.

If these changes are also reasonable from your perspective, I think there are to ways to go to get the build running:

1. Adding those changes to this PR
2. Creating a new separate PR for each driver and when they can be and are merged, rebasing this PR on top of that.

For the sake of clarity, I'd assume the second option being the best, as it would also allow an individual review by a maintainer for each driver.

Would one of those ways be suitable?

[kaspar030]

Would one of those ways be suitable?

Option two would be the way to go! Thanks for dealing with these unrelated issures...

[fhessel]

@kaspar030 As the PRs for the drivers are now merged into master: How should I proceed?

Should I rebase my branch on the current master and force-push it to verify with Murdock? And should I already squash the requested formatting changes into the original commit?

[fhessel]

@kaspar030 Is there anything else I can do to bring this PR forward, or is there just no capacity for review at the moment?

[kaspar030]

Should I rebase my branch on the current master and force-push it to verify with Murdock? And should I already squash the requested formatting changes into the original commit?

yes please!

is there just no capacity for review at the moment?

reviewing time is always scarce, sorry...

[fhessel]

Thanks for the feedback!

It looks like the llvm toolchain has another issue with the adt3710. I'll try to fix it like the other issues and then rebase the PR again. I might need some time however, as I'm less familiar with llvm than with gcc.

[benpicco]

Sorry for the long period of radio silence, I hope you are still with us!

The only failing unit test is tests/driver_adt7310 where llvm rightfully complains about the use of abs() on a float.

Just piggy-back a commit that casts the argument to int and CI should be happy.

--- a/tests/driver_adt7310/main.c
+++ b/tests/driver_adt7310/main.c
@@ -80,7 +80,7 @@ int test_adt7310_sample_print(adt7310_t *dev)
     fractional = modff(celsius_float, &integral);
 
     printf("0x%04" PRIx16 " %7" PRId32 " mC %4d.%07u C)\n", raw, millicelsius,
-        (int)integral, abs(fractional * 10000000.f));
+        (int)integral, abs((int) (fractional * 10000000.f)));
     return 0;

[fhessel]

I myself didn't get to setting up the llvm toolchain, so sorry for that too. However, I'd be really nice to get this through now.

I think the most meaningful next step would be a rebase, as this PR has been stale for some time now, and to fix that llvm error. Do you think just piggy-backing the driver_adt7310 fix is the right choice here, as all the other required changes have also been done in separate PRs? Otherwise I'd just file such a PR, wait until it made its way to master and then do the rebase.

[benpicco]

You don't need to setup a llvm toolchain just for that.
The fixup is small, just add it as a separate commit here, it's just one line changed (a cast).

[fhessel]

Regarding your suggested change to just cast to int here – is it valid to assume that int is always 32 bit? Otherwise casting would cause undefined behavior: fractional is within the [0.0, 1.0) interval, so multiplying it by 10000000.f would clearly exceed the boundaries of a 16 bit int. Wouldn't casting to long and using labs() be a more general solution then (note also the change from %07u to %07lu)?

printf("0x%04" PRIx16 " %7" PRId32 " mC %4d.%07lu C)\n", raw, millicelsius,
  (int)integral, labs((long) (fractional * 10000000.f)));

This might not be a problem with the native CPU, but maybe it is for some other boards?

[benpicco]

Good catch! Using labs() should solve the problem indeed.

[fhessel]

It seems like Murdock is finally happy with this PR.

@benpicco – I added you suggestions in 002368b after rebasing the PR to the current master.

@kaspar030 – The requested changes to code style should also be implemented, if I'm not mistaken.

What's the best way to proceed from here? As the formal checks have passed, only testing the new functionality on actual hardware would remain, if you'd like to try that before merging.

If I should squash 002368b on top of 6726279, just let me know. I didn't want to do that while there are some pending review comments.

[benpicco]

Nice!
I just tried it on a raspberry pi in loopback mode and it's working well!

The code looks good too, I just have some minor stylistic suggestions for improvement.

@kaspar030 what do you think?

[fhessel]

I added all the suggested changes in beea452, spi_transfer_bytes looks a lot cleaner now, thanks!

[fhessel]

Reviewing the docs myself again, I found another typo/mistake where I confused NUM_SPI and SPI_NUMOF – fixed in 3fd6bf5.

[benpicco]

I think the comments by @kaspar030 have been addressed.
I'll wait till the end of soft feature freeze this week, but this should be good to merge.

Feel free to squash already!

[fhessel]

Squashed and rebased onto current master.

[miri64]

Sorry for coming in so late to the review, but can we make this a native-specific module (compare netdev_tap)? This way, in case we ever want to implement emulated SPI, we don't have to dance around the wrapped SPI.

[miri64]

(spidev_linux would already be a great module name ;-))

[fhessel]

I separated the SPI code in 6187ae8 as an optional module spidev_linux.

However, there are some things for which I'm not sure if there is a better solution:

• Makefile.features now contains an entry that adds periph_spi only if the spidev_linux module has previously been added to the project. Otherwise you'd get the warning that the build may fail.
• The configuration for SPI is still in periph_cpu.h and not in a separate spidev_linux_params.h like it's the case for e.g. candev_linux. I found no suitable way of putting the definitions elsewhere, as periph/spi.h was included too early everytime, and the PERIPH_SPI_NEEDS_ have to be defined before.

Maybe you could especially have a look at these points, as you know the build system much better than I do.

Other than that, without adding USEMODULE += spidev_linux to the Makefile, native should now again be building without SPI. When testing with the tests/periph_spi agian, don't forget to add this line there.

[benpicco]

I'd say periph_spi_linux would be a better module name, since periph_* is already a pseudo module. So you can keep it in periph/ and don't need an extra Makefile.

Keep the

ifeq ($(OS),Linux)
  FEATURES_PROVIDED += periph_spi
endif

But add a entry to cpu/native/Makefile.dep (does not exist yet, but will be included automatically):

ifneq (,$(filter periph_spi,$(USEMODULE)))
  USEMODULE += periph_spi_linux
endif

[fhessel]

@benpicco – I now use the pesudomodule like you suggested, but I kept the spidev part in its name to be able to match the other header filenames (like candev_linux.h). If that's not okay, just let me know.

I tested the commit with examples/hello_world and tests/periph_spi on a Raspberry Pi and also in a VM with FreeBSD to have a non-Linux build environment. The module is only used in the periph_spi application, which ran successfully on the Pi and failed to build on FreeBSD with the error message that SPI is only supported on Linux.

@miri64 – Does this sound reasonable to you, too? If native should get an emulated SPI in the future, only the Makefiles have to be adjusted to control which implementation is used.

comments have been addressed.

[benpicco]

I just tested it again and it still works.
If there are no objections, I'd say please squash!

[miri64]

I have just some style comments left.

[fhessel]

@miri64 – Thanks for the feedback, I implemented your suggestions in 9f63855.

[miri64]

Thanks! As @benpicco already asked: you may squash :-).

[benpicco]

Code looks good, tested working on a raspberry pi.