Requested enhancement -- Add UserContext field to HAL drivers

[dmarks-ls]

I have a simple recommendation that would greatly improve your HAL drivers: please add a UserContext field to the handle structures for HAL peripherals (e.g. UART, I2C, SPI).  I'll explain.

I write compatibility drivers for communication peripherals for each MCU our company works with.  That way, no matter which MCU we select for a project, we have a common API for a given peripheral type.  We always enable HAL callbacks for peripherals like UART, I2C, and SPI, since this simplifies registering a pre-made library function with the STM32 HAL.  Each peripheral instance has an associated runtime data structure; for instance, a UART structure contains a receive FIFO queue and transmit FIFO queue.  When my application wants to receive data, it just checks the receive FIFO; the driver I've written handles RX callbacks and stuffs that data into the FIFO.

The problem I run into with the STM32 HAL, however, is that when a callback arrives, it is not simple to associate the peripheral instance with my data structure.  The callback only provides the HAL UART handle structure, so I then have to search through all of my UART structures to find the one corresponding to this particular HAL instance, like this:

typedef struct STM32UartRegEntry {
  STM32Uart_Instance *inst;
} STM32UartRegEntry;
static STM32UartRegEntry registry[PLATFORM_STM32_UART_MAX_INSTANCES] = { 0 };

static STM32Uart_Instance* GetInstance(UART_HandleTypeDef *huart) {
  STM32Uart_Instance *retval = NULL;
  int i;
  for (i = 0; i < PLATFORM_STM32_UART_MAX_INSTANCES; ++i) {
    STM32Uart_Instance *inst = registry[i].inst;
    if (NULL != inst) {
      if (inst->handle_ == huart) {
        retval = inst;
        break;
      }
    }
  }
  return retval;
}

static void UartRxCompleteCallback(UART_HandleTypeDef *huart) {
  bool need_context_switch = false;
  STM32Uart_Instance* inst = GetInstance(huart);
  if (NULL != inst) {
    os_streambuf_send_from_isr(&inst->rx_stream_, inst->rx_buffer_, 1u,
                               &need_context_switch);
    HAL_UART_Receive_IT(inst->handle_, inst->rx_buffer_, 1u);
    os_yield_from_isr(need_context_switch);
  }
}

However, you could greatly improve this by simply providing a user-settable field in the UART handle structure, something like this:

typedef struct __UART_HandleTypeDef
{
  USART_TypeDef  *Instance;        /*!< UART registers base address        */
  [...]
  void *UserContext;   /*!< User-settable context pointer, not used by HAL   */  
} UART_HandleTypeDef;

In my driver, when I initialize this peripheral instance, I would stuff a pointer to my structure into the handle:

void STM32Uart_Init(STM32Uart_Instance *inst, UART_HandleTypeDef *handle,
                    const os_streambuf_t *rx_stream,
                    const os_streambuf_t *tx_stream) {
  /* Copy the peripheral handle. */
  inst->handle_ = handle;
  /* Stuff instance pointer into the peripheral handle. */
  handle->UserContext = inst;
  [...]
}

This would make the first block of code above a LOT simpler and much faster:

static void UartRxCompleteCallback(UART_HandleTypeDef *huart) {
  bool need_context_switch = false;
  STM32Uart_Instance* inst = huart->UserContext;
  if (NULL != inst) {
    os_streambuf_send_from_isr(&inst->rx_stream_, inst->rx_buffer_, 1u,
                               &need_context_switch);
    HAL_UART_Receive_IT(inst->handle_, inst->rx_buffer_, 1u);
    os_yield_from_isr(need_context_switch);
  }
}

Could I simply hack your header file and put that field in there myself?  Yes, but we both know how messy and fraught with peril that is from a software maintenance standpoint.  If you can simply add one void * field to HAL handle structures, that would allow me (and others) to easily extend the functionality of your HAL drivers without messy searches every time an interrupt pops.

Please consider my request, thank you.

Dana M.

[TDK]

Requested many times, no plans to implement it. It does have drawbacks.

Solved: Feature request: Pass arbitrary data pointer in HA... - STMicroelectronics Community

Custom context in callback handlers - STMicroelectronics Community

Is there a way to provide context pointer to HAL c... - STMicroelectronics Community

[Saket_Om]

Hello @dmarks-ls 

I reported your request internally.

Internal ticket number: 210196 (This is an internal tracking number and is not accessible or usable by customers).

[Andrew Neil]

---

@Saket_Om wrote:

Internal ticket number: 210196

---

Another one to add to the collection ... ?

[dmarks-ls]

Apart from eating 4 bytes of RAM, what are the "drawbacks"?  The suggestion of making this "opt-in" with a conditional macro solves that issue.  Alternately, if you're worried about the burden of regression testing all the SDK examples, just give us a way to insert user code into the HAL struct definition like you do with main.c:

  /* USER CODE BEGIN */
  /* USER CODE END */

Regarding Meallem's argument about not putting it into existing CubeFW packages but considering it for "new products"... I'm sorry, but that's weak.  I get that regression testing the SDK examples is not trivial, but STM32U5, H7, etc. are new products, but because they've already been released, you're unwilling to make a simple yet significant improvement to the supporting software?  That's not a good look.

Dana M.

[dmarks-ls]

---

@Andrew Neil wrote:

---

@Saket_Om wrote:

Internal ticket number: 210196

---

Another one to add to the collection ... ?

---

I'd like to think that if enough people express interest/displeasure, they'll eventually decide it's a good thing to do.  Squeaky wheel and all that.

Dana M.

[TDK]

Overall it's a pretty modest change and these are minor issues, but since you asked:

> Apart from eating 4 bytes of RAM, what are the "drawbacks"?

Gating behind a macro solves some problems but introduces others. Now sizeof(UART_HandleTypeDef) is no longer constant. Have two chips talking to each other passing that data? Can't rely on that anymore.

Code complexity goes up. Now existing examples have to choose whether or not this macro is enabled. If someone encounters an example on the web and it doesn't work, they come to ask why UserContext isn't defined. Someone finding a new example but still using an old library encounters similar confusion.

Casting void* to something else violates MISRA rules. Now you can't claim MISRA compliance anymore if that's in use.

They've changed the structure of handles a few times through HAL development. Seems to have reached a steady state, at least with common peripherals. Probably resistant to change it again, especially for *all* existing and future peripherals.

H7 released in 2017. Not exactly old, but I wouldn't call it new either. U5 in 2021, certainly newer.

Not really arguing against. I would have put this feature higher on the list than the user-defined per-peripheral callbacks that are already implemented. They serve similar functions.

[Pavel A.]

Pardon me for jumping in but... as noted by others this idea already has been discussed and has drawbacks.

So chances are that it won't be implemented timely or at all.

So it would be more pragmatic to think of a way to optimize look-up of the instances.

One possibility can be indeed patching of the needed HAL .h files where the "handle" structs are defined. This option should be also MISRA-safe. Keep these patches in your version control, apply when the HAL library is updated. These libraries are not updated very often - much slower than Linux kernel and so on. 

Another way... use a wrapper structure around the HAL "handles". Has been discussed in other threads.

Use one of these tricks and become happy instantly, without  annoying others.

By the way, here is another request for a revolution now...

https://community.st.com/t5/stm32-mcus-embedded-software/please-add-a-preprocessor-define-to-disable-compilation-of-weak/td-p/804409

How could we live all these years without this? really?