Yes, the wiced_spi_* APIs are not thread safe. Locking/Unlocking is required if multiple threads are accessing to the same SPI peripheral.
Yes, currently the same speed, mode and data size devices could share the same SPI peripheral.
Thanks for confirming, I have added locking/unlocking as well as per bus context to allow different device configuration. It seems to work well for me. Let me know if you would like to share that back.
Why is the same speed, mode and data size shared? It looks like the wiced_spi_device_t structure is passed into the init and the deinit (which looks more like a reinit to me). Since you have to specify a different GPIO pin to select by, why can't the speed/mode/size be specified on each init/reinit when you select the desired slave?
SPI bus is initialized once, during spi_init. Previous settings are not retained. Currently, if you init port with settings A, then again for another device with setting B, settings A will be lost. Worst, a transfer for device A, after device B has been initialized will be performed with most recent settings.
Current SPI bus driver is pretty much broken for multi-client use. A concept of bus context needs to be introduced and bus re-configure for each client when necessary between transactions as it is not the case currently.
Also, a lock / port needs to be added so multi-threaded system can access ports safely.
So is this a problem with the SDK or the driver talking to the SDK?
On the side of the SDK, you could locally have a state that indicates which SPI slave you are talking to, you can easily switch states on the calling side by calling init/deinit. As long as you switch at request boundaries, which would be ensured by your locking code, the slave should not see the switch and the reconfigure should work fine.
I believe the is should be fixed on the MCU platform code, it is quite easy to achieve, what is the point of having SPI device config passed down to the SPI platform APIs if they are not used to reconfigure the bus prior to transfer ?
I am not disagreeing with you. The simplest interface that could have been given to you would be one that just requested a transfer and passed a wiced_spi_device_t , which it does, and have that interface perform the init/deinit as is required. That would be clean and would solve this part of your request.
I don't know what the API designer(s) had in mind. It is possible they separated the init/deinit from the transfer to allow the hardware to be turned off to save power and pass the structure pointer just as a handle. It is hard to tell without asking them.
The deinit does actually do anything, I think SPI wasn't quite thought through as a bus, with multiple client in mind. It was clearly design to handle the serial flash that comes with the ref design.
Since I/O are pretty limited on the module itself, I use a SPI GPIO expander, a SPI display, etc... and the driver in its current state does not quite cut it.
I open that thread before taking on the task of modifying the SDK code to my need, just to check that I was not missing anything.
Maybe my use case is singular, but maybe not ;-)
The deinit in its current state, looks like you could pass a different wiced_spi_device_t structure and it would reinit, which was part of my first comment.
The API designer thought it out enough to pass a GPIO and didn't make the assumption that there was a hard wired slave select line. So they did move in the right direction. Making the change to drop init/deinit and just perform the transfer (configuring as required based on the passed wiced_spi_device_t) seems like it is a simple request and one that should be quite common, and not just a singular case. Does anyone know who designed this interface?
You are indeed right about deinit calling init, I was looking at the MCU code itself.
Maybe they were expecting caller to take care of handling the lock, calling init before every single transfer... That would probably work, but it is not intuitive, not efficient either if you ask me.
The locking is best handled by the caller. This could be handled by a semaphore or disabling interrupts, as is required. The latter could be a performance hit and should not be inflicted on all for the cases that the request is being performed at an ISR level, I would assume this to be a minority.
If the MCU code kept the state of the current connection and compared against that before performing the inefficient configuration setup, then you would only take the hit when you change the device you are talking to. In most cases, you are going to have bursts of activity on one device at a time and this would solve that issue.
I am not sure I agree by the lock handled by caller, but we can agree to disagree on that :-)
Regarding current port state, this is what I did, comparison between caller device settings and current set settings comes down to a few memcpm() on a few bytes, this is quite painless. If setting is different, port is reconfigured.
I will agree on the disagreeing :-)
You chose the right answer and we just need to get this pushed into the MCU code and it will be handled for all. I will ask around to see who designed that and if it can be changed.
Thanks for the feedback !