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

[Feature Request] Pass arbitrary data pointer in callbacks #23

Closed willtoth closed 1 year ago

willtoth commented 3 years ago

Feature Request I very much appreciate the new callback functions added to the STM32Cube HAL, making it much easier to create our own middleware that integrates into the STM32Cube ecosystem. However there is still a limitation, the callbacks only provide pointers to their own handles.

For example, looking at the SPI callbacks:

https://github.com/STMicroelectronics/STM32CubeG4/blob/master/Drivers/STM32G4xx_HAL_Driver/Inc/stm32g4xx_hal_spi.h#L150-L160

https://github.com/STMicroelectronics/STM32CubeG4/blob/master/Drivers/STM32G4xx_HAL_Driver/Inc/stm32g4xx_hal_spi.h#L781

Lets say I am making a generic driver for some SPI device, which I want to scale to multiple SPI instances. With the callback API I can easily create a callback and register it to an STM32Cube driver. However inside of the callback context, the user only has access to the SPI handle assigned, with no way to know which user driver it came from without creating some global lookup.

Easy Solution

Either:

1) Allow passing a pointer which is stored with the callback:

HAL_StatusTypeDef HAL_SPI_RegisterCallback(SPI_HandleTypeDef *hspi, HAL_SPI_CallbackIDTypeDef CallbackID, pSPI_CallbackTypeDef pCallback, void* context);

2) Add a 'user pointer' field to the SPI_HandleTypeDef handle, so the user could say

hspi1->p_context = p_myDriverContext;

ASELSTM commented 3 years ago

Hi @willtoth,

Allow me first to thank you for your contribution. Your request has been forwarded to our development team. We will get back to you as soon as we have more details.

With regards,

ASELSTM commented 3 years ago

Hi @willtoth,

Please excuse this somewhat late reply. Our development teams agreed on the point you raised. This point has already been raised internally and they have planned a global update since a while.

However, this is likely to require some time to be deployed. We cannot share a date for the moment, but we will do our best to have such a global update deployed and released soon. We count on your patience.

Thank you again for having tackled the subject.

With regards,

ASELSTM commented 3 years ago

ST Internal Reference: 16277

koniarik commented 1 year ago

Any updates for this? :) As much as I can understand this takes some time, one year seems like plenty of time...

BOJIT commented 1 year ago

Hi @ASELSTM,

Is there an expected release for this feature? My team have been using some workarounds to get around this issue for quite some time.

If this is currently not under development, would your team accept a PR to fix? I currently have been adding a context field to all *_HandleTypedef structs using a shell script.

Would the following solution be an option?

typedef struct __I2C_HandleTypeDef
{

/* Rest of struct... */

#if (USE_HAL_I2C_REGISTER_CALLBACKS == 1)

  /* Rest of callback pointers... */

  /*!< I2C Msp Init callback                     */
  void (* MspDeInitCallback)(struct __I2C_HandleTypeDef *hi2c);
  /*!< I2C Msp DeInit callback                   */

#if (USE_HAL_CALLBACK_CONTEXT == 1)
  void *UserContext;
#endif /* USE_HAL_CALLBACK_CONTEXT */

#endif  /* USE_HAL_I2C_REGISTER_CALLBACKS */
} I2C_HandleTypeDef;

This way without explicitly enabling the feature, the structs are unchanged.

ASELSTM commented 1 year ago

Hi @willtoth,

Unfortunately, there is no plan to deploy this in the near future. We will keep you informed, should there be any update in this plan.

Please allow me to close this issue. Thank you for your comprehension and thank you again for your report.

With regards,

yaqwsx commented 1 year ago

@ASELSTM Would you mind explaining why you decided this is a useless feature? The proposal by @BOJIT is very reasonable, costs you a couple of manhours to implement, it preserves backward compatibility, and helps to write a cleaner code.