STMicroelectronics / STM32CubeG4

STM32Cube MCU Full Package for the STM32G4 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
Other
182 stars 98 forks source link

FDCAN DataLength Field Clarification/Usability #13

Closed willtoth closed 3 years ago

willtoth commented 4 years ago

Describe the set-up My application is using STM32G4 series, with FDCAN in Classic Mode. We have other devices that use STM32Fxxx with bxCAN, and are building a compatibility layer.

While looking through the documentation for configuring the DLC, it is surprising to find that the DLC is forced to be a define of 'type' FDCAN_data_length_code' which is equivalent to using DLC << 16. I understand that this is used later to 'or' with the TX RAM field, however it is already shifted back once to back-calculate the number of bytes to copy when calling HAL_FDCAN_AddMessageToTxFifoQ or when receiving.

Since the shift is already being done internal to the HAL driver, wouldn't it make sense to shift left internally, instead of making the interface confusing to the user? The driver can still check that it is a valid DLC, and keep the same defined values.

https://github.com/STMicroelectronics/STM32CubeG4/blob/master/Drivers/STM32G4xx_HAL_Driver/Inc/stm32g4xx_hal_fdcan.h#L604-L619

ASELSTM commented 4 years ago

Hi @willtoth,

Thank you for this report. You request has been forwarded to our development teams. We will get back to you as soon as they provide us with their feedback.

With regards,

ASELSTM commented 4 years ago

Hi @willtoth,

According to our development team, referring to the reference manual RM0440. The DLC is always in the position 16 to 19 in the TX fifo/buffer or the RX fifo/buffer, that’s why we have defined the possible values of the field DLC shifted by 16. Thus, there is no need to modify the API internally when intending to write a DLC field.

image

In fact, the number of data bytes can be obtained by shifting the DLC value by 16 to the right, which is already done internally in the function FDCAN_CopyMessageToRAM called by the HAL_FDCAN_AddMessageToTxFifoQ API.

Indeed, the FDCAN_CopyMessageToRAM function is used to copy data in the RAM, for that we should retrieve the right number of data bytes by shifting the DLC value by 16 to right.

Untitled

That’s why we shift DLC value by 16 to right.

With regards,

willtoth commented 4 years ago

Hi,

Thanks for the response, and sorry I was unclear in my original issue. I understand that the DLC in the RAM register is shifted by 16 internal to the device. My point here is to simplifiy the API to the end user and make it more clear, by allowing the user to enter the actual CAN DLC as an integer (i.e. native to the CAN protocol) instead of the shifted value (i.e. native to the STM32G4). The driver implementation is not even an optimization since a shift is already being done.

Since the DLC is a data size, it is already not very intuitive to expect a data size parameter to be defined the way it is in this driver. For example the CAN driver on STM32F3 this call is:

HAL_StatusTypeDef HAL_CAN_AddTxMessage(CAN_HandleTypeDef *hcan, CAN_TxHeaderTypeDef *pHeader, uint8_t aData[], uint32_t *pTxMailbox)

Where the DLC of CAN_TxHeaderTypeDef is defined as

uint32_t DLC;

https://github.com/STMicroelectronics/STM32CubeF3/blob/master/Drivers/STM32F3xx_HAL_Driver/Inc/stm32f3xx_hal_can.h#L161

Another example is the UART HAL:

HAL_StatusTypeDef HAL_UART_Transmit(UART_HandleTypeDef *huart, uint8_t *pData, uint16_t Size, uint32_t Timeout);

In the UART API, the user doesn't care how the size parameter is being used, just that it is the size of the data.

ASELSTM commented 3 years ago

Hi @willtoth,

We understand that this will make the API easier for the end user and ensure a better understanding of the code. However, according to our development team, the impact of this change is very important. Therefore, our development team regrets to decline your request. Thank you for your understanding.

Please allow me then to close this thread. Thank you for your comprehension and thank you again for your contribution.

With regards,