Fix TMC6200 driver: spi->end() should be spi->endTransaction()#99
Open
94xhn wants to merge 1 commit into
Open
Conversation
readRegister() and writeRegister() both pair spi->beginTransaction() with spi->end() instead of spi->endTransaction(). end() tears down the whole SPI peripheral (spi_deinit on STM32), while endTransaction() just releases the transaction lock. Every other driver in this repo (drv8316, drv832x, drv8376) correctly uses endTransaction() here. On the STM32 Arduino core the effect is worse than just needing a fresh begin() before reusing the bus: SPIClass::beginTransaction() skips re-initializing the hardware whenever the SPISettings object is unchanged from the last call, so the very next TMC6200 register access (e.g. the read-modify-write in setDriverState()) ends up transferring over a peripheral that was just de-initialized by the previous call's end(), with no error raised. Confirmed this by tracing the actual Arduino_Core_STM32 SPI.cpp logic and reproducing the sequence in a small standalone harness. Reported in simplefoc#66, maintainer agreed with the fix but it was never applied.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both
readRegister()andwriteRegister()in TMC6200.cpp pairspi->beginTransaction(settings)withspi->end()instead ofspi->endTransaction().end()fully de-initializes the SPI peripheral (on the STM32 core it callsspi_deinit()), whileendTransaction()is just supposed to release the transaction lock so the bus can be shared. Every other driver in this repo (drv8316, drv832x, drv8376) gets this right and usesendTransaction().On the STM32 Arduino core this is worse than "just needs a fresh begin() before the bus is used again":
SPIClass::beginTransaction()only re-initializes the hardware when theSPISettingspassed in differ from the ones cached from the previous call. TMC6200Driver reuses the samesettingsmember for every call, so as soon asreadRegister()tears the peripheral down withend(), the very nextbeginTransaction()(e.g. the write half of the read-modify-write insetDriverState()) sees identical settings and silently skips re-init, and the write goes out over a de-initialized peripheral with no error raised.I confirmed this by pulling the actual
SPIClass::configSpi/begin/beginTransaction/end/endTransactionimplementation from Arduino_Core_STM32 and reproducing the exact call sequence (readRegister()thenwriteRegister(), assetDriverState()does) in a small standalone harness outside the Arduino toolchain - the peripheral ends up de-initialized after the very first register write following a read.This was reported in #66 back in October, and the maintainer agreed with the fix ("I will fix it for the next release") but it looks like it never got applied. It's a straight two-line swap, no behavior change intended anywhere else.