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.

[CTapp.1]

MISRA is not really an issue here. Sure, casting from void * is "banned" by a rule - but it is not mandatory (so the conversions are "restricted", not "banned", so a justifiable violation can be supported through the use of a deviation (and most projects are expected to include deviations). The MISRA rule is there to ensure that undefined behaviour does not arise when a void * pointer is incorrectly converted to pointer to object (alignment, incompatible type, ...). However, the use of void * to refer to a device context is easy to justify, as the conversions involved are T * ->  void * -> T * (where T is the same type), there is no risk of the conversion leading to undefined behaviour. This is they type of use case that could readily be supported by the use of a Deviation Permit, as explained in MISRA Compliance.

Declaration of interest: I am currently a member of the MISRA C Working Group, co-author of MISRA Compliance and chair of the MISRA C++ Working Group.