-
-
Notifications
You must be signed in to change notification settings - Fork 429
Marlin SPI Review
-
Marlin SPI Review
- Marlin Core
- TFDI eve touch ui (~OK)
- Marlin/src/HAL/STM32_F4_F7/STM32F7/TMC2660.cpp (OK)
- TFT (OK??)
- W25QXX (~OK)
- Stepper.cpp -> digipot_init (NOK NOK)
- Marlin/src/feature/dac/dac_dac084s085.cpp (NOK)
- LPC1768
- DUE
- STM32
- STM32_F4_F7
- TEENSY31_32
- TEENSY35_36
- TEENSY40_41
- Libs
- HAL status
- Conclusions
- How to Fix
Marlin born with a few devices using SPI. But now, we have a lot of concurrent devices sharing SPI.
For one SPI device work, it needs prepare a spi port with ALL this parameters, before each spi usage:
- SPI PORT
- Speed
- Bit Order
- Data Mode
A lot of current Marlin code rely upon HAL to configure the SPI. But it leads to a problem: the current HAL code only handles ONE SPI PORT at time. So, the client code must ensure that the SPI is properly configured before the use and the corret SPI PORT is selected.
This review try to search for every spi usage to check if it's doing all the SPI setup before the use.
uses spiXXXX functions available in HAL
call spiInit before use the spi
!!! rely on the spiInit to configure ALL spi paramenters for SD !!!
uses spiXXXX functions available in HAL
call spiBegin+spiInit ONLY ONCE in the init of the code
!!! don't call any begin or init to send/recv data !!!
use global SPI object. Have its own SPISettings instance and use beginTransaction. (good)
!!! rely on global SPI object to select the correct SPI PORT !!!
uses spiXXXX functions available in HAL
call spiBegin+spiInit before use the spi
!!! rely on the spiInit to configure ALL spi paramenters !!!
rely on the spiInit to configure the correct spi paramenters (media player)
use global SPI object. Have its own SPISettings instance and use beginTransaction. (good)
!!! rely on global SPI object to select the correct SPI PORT !!!
Uses its own SPIClass instance
After the refactoring, all TFT and TOUCH io will be using its own SPIClass instance
Use global SPI object. Have its own init method that setup ALL spi parameters.
!!! it change the global SPI object and may conflit with others using that same object !!!
Use global SPI object.
Don't have its own configs.
Call SPI.begin only once in the init!
Don't call begin before spi usage.
!!! Rely totally in the CURRENT global SPI instance !!!
use spiXXX functions.
Don't call spiInit, only spiBegin
Call SPI.begin only once in the init!
Don't call begin before spi usage.
!!! don't call any begin or init to use SPI !!!
uses spiXXXX functions available in HAL
call spiBegin+spiInit ONLY ONCE in the init of the LCD setup
!!! don't call any begin or init to send data to LCD !!!
uses spiXXXX functions available in HAL
call spiBegin+spiInit ONLY ONCE in the init of the LCD setup
!!! don't call any begin or init to send data to LCD !!!
spiBegin calls spiInit in DUE
uses spiXXXX functions available in HAL
call spiBegin+spiInit ONLY ONCE in the init of the LCD setup
!!! don't call any begin or init to send data to LCD !!!
- spiRec/spiRead/spiSend/etc call beginTransaction for every byte
!!! client code must call begin/end accordingly !!!
- spiRec/spiRead/spiSend/etc call beginTransaction for every byte
!!! client code must call begin/end accordingly !!!
- spiRec/spiRead/spiSend/etc call beginTransaction for every byte
!!! client code must call begin/end accordingly !!!
- spiRec/spiRead/spiSend/etc call beginTransaction for every byte
!!! client code must call begin/end accordingly !!!
- spiRec/spiRead/spiSend/etc call beginTransaction for every byte
!!! client code must call begin/end accordingly !!!
use global SPI object. Have its own SPISettings instance and use beginTransaction. (good)
Call beginTransaction for every byte in some cases. It could call beginTransaction in the SELECT command.
!!! rely on global SPI object to select the correct SPI PORT !!!
use global SPI object. Have its own SPISettings instance and use beginTransaction. (good)
!!! rely on global SPI object to select the correct SPI PORT !!!
As a lot of code rely upon spiInit function, here is the status of that function in each HAL.
!!! Note: the current spiXXXX functions ONLY works with ONE SPI PORT !!!
spiInit function configure ALL spi parameters to the HAL default.
!!! problem: spiXXXX functions alter the SPI global object
The most commom problem is rely in spiInit and the global SPI instance. It may break all the codes that rely in this methods, if we have two peripheral using diferent SPI PORTs, both changing the global spi config.
There're 3 or 4 places that are very problematics. But the main issue is about the current selected SPI PORT.
It's working now because seems all most common marlin setups uses the same SPI PORT for the active peripheral. And it may explain the random complains about SD or other spi peripheral not working in some users setups.
For example: SD using SPI 1 and TMC (or U8Glib) using SPI 2 will not work, because the global SPI instance will point only to the SD spi, as the TMC/U8GLib don't change the SPI PORT when using it...
In resume, we have 3 types of problems:
1 - Wrong/missing/incomplete/unneeded begin/init calls (high problematic)
2 - Almost no handling of different SPI PORT (medium)
3 - We have 2 APIs to use SPI: SPIClass (SPI global object) and spiXXXX functions. It lead to a very confusing code and errors. (low)
Having in mind that we cannot just change everything once and completely break Marlin, I suggest the following action plan:
Objective 1: Remove all the spiXXXX functions in favor of the SPIClass.
Objective 2: Every fix for begin/init calls should be done using SPIClass
Objective 3: Enhance the interface of the SPIClass
Objective 4: Implement SPIClass for all HALs (we can start using the spiXXX funcions for that)
Objective 5: Remove the global SPI instance, only enabling it for lib compatibilty.