drivers/periph/spi: added means to make SPI thread-safe

[haukepetersen]

extended the SPI driver's API with locking/unlocking functions and implemented them exemplary for the stm32f4.

These functions are needed for applications that share a SPI bus for more then one slave device, e.g. 2 sensors, while their drivers run in separate threads.

Usage:

...
spi_aquire(SPI_x);
gpio_set(CS_PIN);
spi_transfer(SPI_X, ...);
gpio_clear(CS_PIN);
spi_release(SPI_X);
...

The price to pay:

for 1 configured SPI interface:    84 byte ROM and  8 byte RAM
for 2 configured SPI interfaces:  112 byte ROM and 16 byte RAM
(compiling the tests/periph_spi application)

[jnohlgard]

I think the memory bytes is a small price to pay for the thread safety of our device drivers. Errors caused by these kinds of race conditions will be extremely difficult to reproduce or locate when they bite you, adding a simple NOP somewhere completely unrelated could potentially make such a problem hide again.

👍 for the API, it is similar to what is being used in ChibiOS (which has been around for a very long time), so the general idea has already been tested thoroughly.

I was thinking of whether to put spi_acquire in a common file for all ports, but I believe that it is better to keep all SPI related functions in one file. It also opens the possibility for some corner cases where you would want to customize the locking mechanism (I have something in mind for kinetis_common #2265 ).

[PeterKietzmann]

I also think that the price is ok for the benefit we get. Should we maybe adapt the test? Or extend it?

[haukepetersen]

@gebart I wouldn't put the locking in a central file, as there might be platforms where the locking can be handled more efficient without mutexes or similar. Also the additional implementation effort is not too high...

@PeterKietzmann I don't think we need to adapt the test, the Mutexes itself are well tested and I think the effort for a 'good' test are not worth it (as the locking implementation is quite straight forward...).

[PeterKietzmann]

Well, I would ACK this. Any other opinions?

[BytesGalore]

looks also good from my side

[thomaseichinger]

Maybe mutual exclusive access?

[jnohlgard]

..."mutually exclusive"...

[haukepetersen]

here is an update - the mutex initialization issue is fixed through static initialization - multiple calls to spi_init should now be save.

I decided against calling spi_acquire in the init function. If you application uses one SPI bus for more then one device (with differing configurations), I think it should be up to the user to protect the complete acquire->init->transfer->release sequence. This is possible as the mutex works already before init is called the first time...

[haukepetersen]

... and the new change even saved 24 bytes in ROM :-)

[thomaseichinger]

Looks good to me.

[PeterKietzmann]

I am fine with this soultion and willing to ACK. But also I would like to hear @gebart 's optinion on it.

[jnohlgard]

@haukepetersen While it is still most likely a user error, I think it would be beneficial for developers to have the init function lock the mutex while initializing, to avoid any race conditions between two drivers, or between an application main thread and some data consumer thread. I don't really see why we should not lock the mutex for all of spi_init?

[haukepetersen]

@gebart You are right, the overhead is limited and it could save some trouble. I was first reluctant against this idea since the major design goal was to keep the peripheral drivers as slim as possible - and every function call adds some more bytes to them... But I agree, the benefits for locking the init function are greater then the drawbacks of 2 function calls, so I will update this PR as soon as I have a minute.

[jnohlgard]

@haukepetersen I will update #2317 accordingly.

[jnohlgard]

@PeterKietzmann I think this is a good solution to the problem. The locking inside spi_init should be added before merging though.

[thomaseichinger]

As the mutexes (mutices?) are statically initialised I thought usage would be

spi_acquire(dev);
spi_init_master(dev, ...);
spi_release(dev);

Wouldn't this result in a more consistent usage?

[jnohlgard]

@thomaseichinger It would make the API usage more consistent, yes. I am updating #2317 again.

[haukepetersen]

I am completely for @thomaseichinger's solution! I think it is for (i) more consitent and it (ii) leaves it to the user to omit the locking for setups where I can sure that I am only using one SPI device for example...

squashed and rebased - ready for merge?

[jnohlgard]

ACK

[haukepetersen]

ok, somehow Travis is complaining that the PR needs to be squashed, though the label was removed... Do we have a bug in our Travis script?

[PeterKietzmann]

Travis is happy

[haukepetersen]

finally. go then!