Dev spi asynch l0l1 #3288
Dev spi asynch l0l1 #3288
Conversation
This is a temporary update waiting for the next official release
This is a temporary update waiting for the next official release
Disable IRQ when transfer is finished. Also clear pending IRQ after they have been disabled.
This commit contains a few optimizations to get a better performance in SPI Asynch mode
|
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
|
@marhil01 Is Oulu ci still broken? If not can we kick this off again ? |
|
@adbridge who is in action to review this PR ? |
|
@LMESTM I'll take a look this morning |
| #if (USE_SPI_CRC != 0U) | ||
| if(hspi->Init.CRCCalculation == SPI_CRCCALCULATION_ENABLE) | ||
| { | ||
| hspi->RxISR = SPI_RxISR_8BITCRC; |
There was a problem hiding this comment.
Extra space here after =
| static void SPI_RxISR_16BITCRC(struct __SPI_HandleTypeDef *hspi) | ||
| { | ||
| __IO uint16_t tmpreg = 0U; | ||
|
|
There was a problem hiding this comment.
Can't tmpreg just be set to hspi->Instance->DR immediately here? Or do you need to write 0 first ? If so, a comment saying why would be good here.
|
|
||
| #if (USE_SPI_CRC != 0U) | ||
| /* Enable CRC Transmission */ | ||
| if((hspi->RxXferCount == 1U) && (hspi->Init.CRCCalculation == SPI_CRCCALCULATION_ENABLE)) |
There was a problem hiding this comment.
Why are you checking a XferCount of 1 ? Can we comment it ?
There was a problem hiding this comment.
RxXferCount == 1 means that the last byte to receive will come next so this is the time to activate HW SPI_CR1_CRCNEXT. Reference Manual says
Communication starts and continues normally until the last data frame has to be sent or
received in the SPIx_DR register. Then CRCNEXT bit has to be set in the SPIx_CR1
register to indicate that the CRC frame transaction will follow after the transaction of the
currently processed data frame. The CRCNEXT bit must be set before the end of the last
data frame transaction.
| if(hspi->RxXferCount==0) | ||
| #if (USE_SPI_CRC != 0U) | ||
| /* Enable CRC Transmission */ | ||
| if((hspi->RxXferCount == 1U) && (hspi->Init.CRCCalculation == SPI_CRCCALCULATION_ENABLE)) |
There was a problem hiding this comment.
Why are you checking a XferCount of 1 ? Can we comment it ?
There was a problem hiding this comment.
same as previous comment, in a 16 bits this case.
|
|
||
| if(hspi->RxXferCount == 0U) | ||
| { | ||
| #if (USE_SPI_CRC != 0U) |
There was a problem hiding this comment.
Any particular reason for using a value for USE_SPI_CRC instead of just:
#ifdef USE_SPI_CRC ?
There was a problem hiding this comment.
No, no particular reason that I know of.
This is how this is defined in STM32 hal.
| SPI_RxCloseIRQHandler(hspi); | ||
| if(Timeout != HAL_MAX_DELAY) | ||
| { | ||
| if((Timeout == 0U) || ((HAL_GetTick()-Tickstart) >= Timeout)) |
There was a problem hiding this comment.
Is HAL_GetTick() guaranteed to always be > Tickstart ? Can wrapping occur? What would be the impact ?
There was a problem hiding this comment.
I'll have to check with code author - is worth a check
| /* Disable TXE, RXNE and ERR interrupts for the interrupt process */ | ||
| __HAL_SPI_DISABLE_IT(hspi, (SPI_IT_TXE | SPI_IT_RXNE | SPI_IT_ERR)); | ||
|
|
||
| if((hspi->Init.Mode == SPI_MODE_MASTER)&&((hspi->Init.Direction == SPI_DIRECTION_1LINE)||(hspi->Init.Direction == SPI_DIRECTION_2LINES_RXONLY))) |
There was a problem hiding this comment.
This might be easier to read if it were split across 2 lines?
| return HAL_TIMEOUT; | ||
| } | ||
| return HAL_OK; | ||
| } |
There was a problem hiding this comment.
Is this function really necessary? could the functionality of SPI_CheckFlag_BSY() and SPI_WaitFlagStateUntilTimeout() not be combined as the only difference is the SET_BIT ?
| static void SPI_CloseRxTx_ISR(SPI_HandleTypeDef *hspi) | ||
| { | ||
| uint32_t tickstart = 0U; | ||
| __IO uint32_t count = SPI_DEFAULT_TIMEOUT * (SystemCoreClock / 24 / 1000); |
There was a problem hiding this comment.
Could you document what these values mean ? 24 and 1000 ?
| if(hspi->State == HAL_SPI_STATE_BUSY_RX) | ||
| { | ||
| hspi->State = HAL_SPI_STATE_READY; | ||
| HAL_SPI_RxCpltCallback(hspi); |
There was a problem hiding this comment.
Some indentation issues here?
| else | ||
| { | ||
| hspi->State = HAL_SPI_STATE_READY; | ||
| HAL_SPI_TxRxCpltCallback(hspi); |
There was a problem hiding this comment.
Some indentation issues here?
| static void SPI_CloseTx_ISR(SPI_HandleTypeDef *hspi) | ||
| { | ||
| uint32_t tickstart = 0U; | ||
| __IO uint32_t count = SPI_DEFAULT_TIMEOUT * (SystemCoreClock / 24 / 1000); |
There was a problem hiding this comment.
Please comment on what the values 24 and 1000 mean
| __IO uint32_t count = SPI_DEFAULT_TIMEOUT * (SystemCoreClock / 24 / 1000); | ||
|
|
||
| /* Init tickstart for timeout management*/ | ||
| tickstart = HAL_GetTick(); |
There was a problem hiding this comment.
Similar comment to previous one about whether tickstart could just be initialised with HAL_GetTick() ?
| do | ||
| { | ||
| if(count-- == 0) | ||
| { |
There was a problem hiding this comment.
Is there any chance that count could enter this loop set to 0 ?
There was a problem hiding this comment.
This might happen in rare cases if communication is broken, or HW clock is unintentionally stopped ... so the loop avoids getting stuck infinitely.
| /** | ||
| * @} | ||
| */ | ||
|
|
There was a problem hiding this comment.
Why is this comment block here ?
| /* Set the function for IT treatment */ | ||
| if(hspi->Init.DataSize > SPI_DATASIZE_8BIT ) | ||
| { | ||
| hspi->TxISR = SPI_TxISR_16BIT; |
There was a problem hiding this comment.
Indentation here is inconsistent with the rest of the function which uses 2 spaces
| } | ||
| else | ||
| { | ||
| hspi->TxISR = SPI_TxISR_8BIT; |
There was a problem hiding this comment.
Indentation here is inconsistent with the rest of the function which uses 2 spaces
| /* Set the function for IT treatment */ | ||
| if(hspi->Init.DataSize > SPI_DATASIZE_8BIT ) | ||
| { | ||
| hspi->RxISR = SPI_RxISR_16BIT; |
There was a problem hiding this comment.
Indentation here is inconsistent with the rest of the function which uses 2 spaces
| } | ||
| else | ||
| { | ||
| hspi->RxISR = SPI_RxISR_8BIT; |
There was a problem hiding this comment.
Indentation here is inconsistent with the rest of the function which uses 2 spaces
| /* Set the function for IT treatment */ | ||
| if(hspi->Init.DataSize > SPI_DATASIZE_8BIT ) | ||
| { | ||
| hspi->RxISR = SPI_2linesRxISR_16BIT; |
There was a problem hiding this comment.
Indentation here is inconsistent with the rest of the function/file which uses 2 spaces
| static void SPI_2linesRxISR_8BITCRC(struct __SPI_HandleTypeDef *hspi) | ||
| { | ||
| __IO uint8_t tmpreg = 0U; | ||
|
|
There was a problem hiding this comment.
Can tmpreg not be initialised directly ?
| __IO uint16_t tmpreg = 0U; | ||
|
|
||
| /* Read data register to flush CRC */ | ||
| tmpreg = hspi->Instance->DR; |
There was a problem hiding this comment.
Can tmpreg not be initialised directly ?
| __IO uint8_t tmpreg = 0U; | ||
|
|
||
| /* Read data register to flush CRC */ | ||
| tmpreg = *((__IO uint8_t*)&hspi->Instance->DR); |
There was a problem hiding this comment.
Can tmpreg not be initialised directly ?
| __IO uint16_t tmpreg = 0U; | ||
|
|
||
| /* Read data register to flush CRC */ | ||
| tmpreg = hspi->Instance->DR; |
There was a problem hiding this comment.
Can tmpreg not be initialised directly ?
|
|
||
| #if (USE_SPI_CRC != 0U) | ||
| /* Enable CRC Transmission */ | ||
| if((hspi->RxXferCount == 1U) && (hspi->Init.CRCCalculation == SPI_CRCCALCULATION_ENABLE)) |
There was a problem hiding this comment.
Why are you checking the Xfer count = 1 ? Could that be commented ?
| if(Timeout != HAL_MAX_DELAY) | ||
| { | ||
| if((Timeout == 0U) || ((HAL_GetTick()-Tickstart) >= Timeout)) | ||
| { |
There was a problem hiding this comment.
Is HAL_GetTick() guaranteed to be >= Timeout? What is the impact if it isn't ?
| * @param Tickstart: tick start value | ||
| * @retval HAL status | ||
| */ | ||
| static HAL_StatusTypeDef SPI_CheckFlag_BSY(SPI_HandleTypeDef *hspi, uint32_t Timeout, uint32_t Tickstart) |
There was a problem hiding this comment.
Similar to previous target comment about combining these functions ?
| uint32_t tickstart = 0U; | ||
| __IO uint32_t count = SPI_DEFAULT_TIMEOUT * (SystemCoreClock / 24 / 1000); | ||
| /* Init tickstart for timeout managment*/ | ||
| tickstart = HAL_GetTick(); |
There was a problem hiding this comment.
Can tickstart not be initialised directly ?
| /* Wait until TXE flag is set */ | ||
| do | ||
| { | ||
| if(count-- == 0) |
There was a problem hiding this comment.
Is count guaranteed not to be able to be 0 on entry to this loop ?
|
@adbridge Hi Anna. thanks for reviewing. Nevertheless I'm a bit annoyed because the stm32l0xx_hal_spi.c and stm32l1xx_hal_spi.c are files from the STM32 SDK2 named Cube or hal. This is the standard SDK as delivered on st.com and we include it as it is and base our MBED development on it. Most of those files don't follow the MBED coding rules. I can try to answer some your comments, but I am not the author of those lines and would like to avoid modifying them as it would diverge from official files ... let me know your thoughts |
| { | ||
| if(hspi->State == HAL_SPI_STATE_BUSY_RX) | ||
| { | ||
| hspi->State = HAL_SPI_STATE_READY; |
| } | ||
| else | ||
| { | ||
| hspi->State = HAL_SPI_STATE_READY; |
| __IO uint32_t count = SPI_DEFAULT_TIMEOUT * (SystemCoreClock / 24 / 1000); | ||
|
|
||
| /* Init tickstart for timeout management*/ | ||
| tickstart = HAL_GetTick(); |
There was a problem hiding this comment.
Can tickstart not be initialised directly?
| /* Wait until TXE flag is set */ | ||
| do | ||
| { | ||
| if(count-- == 0) |
There was a problem hiding this comment.
Is count guaranteed not to be 0 on loop entry ?
|
@LMESTM I have made some review comments, if you could take a look please. Also could you post some test results that verify your changes ? Thanks |
|
@adbridge About tests results, it's not in a table format but here is the result as mentioned in the description : |
|
Hi @LMESTM don't worry I wasn't reviewing against MBED guidelines, merely looking at consistencies within the changes to what was already there and areas where potential issues could lie. They are just comments and feedback, I didn't see anything that would necessarily block us from accepting. |
|
@adbridge ok thanks for the feedback. I will share your comments with the team in charge of the driver so that they can take them into account - next STM32 HAL update should then include updates in line with your feedback. I have provided some answers where I could in the meantime (I have not answered styling issue). |
|
Thanks @LMESTM looks good to go then |
Ports for Upcoming Targets 3241: Add support for FRDM-KW41 ARMmbed/mbed-os#3241 3291: Adding mbed enabled Maker board with NINA-B1 and EVA-M8Q ARMmbed/mbed-os#3291 Fixes and Changes 3062: TARGET_STM :USB device FS ARMmbed/mbed-os#3062 3213: STM32: Refactor us_ticker.c + hal_tick.c files ARMmbed/mbed-os#3213 3288: Dev spi asynch l0l1 ARMmbed/mbed-os#3288 3289: Bug fix of initial value of interrupt edge in "gpio_irq_init" function. ARMmbed/mbed-os#3289 3302: STM32F4 AnalogIn - Clear VBATE and TSVREFE bits before configuring ADC channels ARMmbed/mbed-os#3302 3320: STM32 - Add ADC_VREF label ARMmbed/mbed-os#3320 3321: no HSE available by default for NUCLEO_L432KC ARMmbed/mbed-os#3321 3352: ublox eva nina - fix line endings ARMmbed/mbed-os#3352 3322: DISCO_L053C8 doesn't support LSE ARMmbed/mbed-os#3322 3345: STM32 - Remove TIM_IT_UPDATE flag in HAL_Suspend/ResumeTick functions ARMmbed/mbed-os#3345 3309: [NUC472/M453] Fix CI failed tests ARMmbed/mbed-os#3309 3157: [Silicon Labs] Adding support for EFR32MG1 wireless SoC ARMmbed/mbed-os#3157 3301: I2C - correct return values for write functions (docs) - part 1 ARMmbed/mbed-os#3301 3303: Fix #2956 #2939 #2957 #2959 #2960: Add HAL_DeInit function in gpio_irq destructor ARMmbed/mbed-os#3303 3304: STM32L476: no HSE is present in NUCLEO and DISCO boards ARMmbed/mbed-os#3304 3318: Register map changes for RevG ARMmbed/mbed-os#3318 3317: NUCLEO_F429ZI has integrated LSE ARMmbed/mbed-os#3317 3312: K64F: SPI Asynch API implementation ARMmbed/mbed-os#3312 3324: Dev i2c common code ARMmbed/mbed-os#3324 3369: Add CAN2 missing pins for connector CN12 ARMmbed/mbed-os#3369 3377: STM32 NUCLEO-L152RE Update system core clock to 32MHz ARMmbed/mbed-os#3377 3378: K66F: Enable LWIP feature ARMmbed/mbed-os#3378 3382: [MAX32620] Fixing serial readable function. ARMmbed/mbed-os#3382 3399: NUCLEO_F103RB - Add SERIAL_FC feature ARMmbed/mbed-os#3399 3409: STM32L1 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3409 3416: Renames i2c_api.c for STM32F1 targets to fix IAR exporter ARMmbed/mbed-os#3416 3348: Fix frequency function of CAN driver. ARMmbed/mbed-os#3348 3366: NUCLEO_F412ZG - Add new platform ARMmbed/mbed-os#3366 3379: STM32F0 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3379 3393: ISR register never re-evaluated in HAL_DMA_PollForTransfer for STM32F4 ARMmbed/mbed-os#3393 3408: STM32F7 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3408 3411: STM32L0 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3411 3424: STM32F4 - FIX to add the update of hdma->State variable ARMmbed/mbed-os#3424 3427: Fix stm i2c slave ARMmbed/mbed-os#3427 3429: Fix stm i2c fix init ARMmbed/mbed-os#3429 3434: [NUC472/M453] Fix stuck in lp_ticker_init and other updates ARMmbed/mbed-os#3434
Up to now Aysnch API was not activated on L0 and L1 devices.
This is now done with this PR.
Activation required a few modifications to improve the IRQ handling performances as asynch API provides enhanced SPI throughput but also require high IRQ handling rate, and L0 / L1 are relatively slow MCUs.
Status
READY
The Asynch SPI trasnfers have been tested ok @ 100kHz, 450kHz and 1MHz, for all L0 and L1 boards.
Also non regression was run on NUCLEO_F411RE board.