esp-idf-hal
esp-idf-hal copied to clipboard
esp-rs
Fix issue #99 (try 2): De-assert SPI CS on last transmit
The root cause of #99 seems to be that the behavior of spi_device_polling_transmit and spi_device_transmit for zero-byte transmissions differs between ESP32 hardware versions and appears to be undefined. This pull request removes the zero-byte call to spi_device_polling_transmit and instead assures the final call to spi_device_polling_transmit with non-zero rx/tx buffer do not contain the SPI_TRANS_CS_KEEP_ACTIVE flag.
(Made a new PR off my fix-99 branch rather than main branch.)
I've been looking at the same issue using SPI on the S3. I think the issue with this is that the CS line needs to stay asserted between multiple write / read within a transaction for certain devices. With this change CS changes state at the end of each action which would break that (although this works for my own use case, and the S3 implementation is basically broken without some sort of fix.)
I was going to look at directly setting the register containing SPI_TRANS_CS_KEEP_ACTIVE in the finish method but didn't get around to it. Not sure if it would work.
This change should not change the behavior of the CS pin compared to the current code so long as the bus is accessed through SpiDeviceMaster read/write/transfer. But I guess it is true that in the current version one can do custom transactions with the CS pin asserted and that is no longer possible in this change.
Unless there is a way to de-assert CS directly without initiating a transaction, I’m not sure of a good solution. Seems the underlying API is missing this feature, and the HAL API doesn’t let us expose extra arguments like keep_cs_active…
Should we just ask upstream to clarify that zero-byte transfers shouldn’t clock the bus and fix the implementation for S3/C3 so this is the case? (And maybe make this explicit for other cases where it is the current behavior perhaps by accident?)
Agree that having an explicit method to shut down a transaction including chip select would be ideal.
The changes you've currently made to the implementation of SpiBus can be moved to SpiDevice implementation instead.
So SpiDevice::transaction will still have the bug but SpiDevice::write, SpiDevice::read, etc. will be fixed.
A more advance fix for SpiBus would be to buffer writes, which the SpiBus trait allows. If the last command a user makes in a transaction is a SpiBus::write, then we can perform this write after the transaction function ends and set keep_cs_active to false. It only fixes one edge case though but it might be a popular one. 🙂
@Dominaezzz @Vollbrecht Is this PR still necessary after the SPI driver rework?
yeah as stated #99 is only fixed for the driver that uses SpiSoftCsDeviceDriver as it doesnt need to reset SPI_TRANS_CS_KEEP_ACTIVE flag to make the cs pins work. so if your application have a problem with garbage data getting clk'd out on the bus one need to use the SpiSoftCsDeviceDriver.
