Timer events as DMA triggers - Improving the current situation in CubeMX/HAL

[BarryWhit]

Hello ST,

Today, in two separate threads, I've had to walk two separate newcomers to STM32 through the pitfalls of trying to trigger a DMA to an arbitrary peripheral from a timer event. The threads are:

what-are-gpio-events-how-do-i-connect-an-event-to-a-peripheral 

and

spi-dma-request-triggered-by-timer-compare 

I myself had to struggle (using the G431) not that long ago to make sense of what the CubeMX generated code does, and why it isn't usable.

HAL functions HAL_TIM_IC_Start_DMA/HAL_TIM_OC_Start_DMA hardcode the DMA peripheral register to which read/write is performed, thus they are not usable. They do however perform a lot of prep work, which it is not trivial

for beginners to reconstruct on their own (which they tend to do since they can't figure out what CUbeMX is actually

generating code to do). This is evident by the two threads above as well as my own experience.

There is no HAL API (that I know of) that allows the developer to specify the peripheral-side register address.

I strongly encourage the CubeMX developers to address this shortcoming in an upcoming release, for the following reasons:

1. It's a common use-case which currently isn't addressed. 

2. It's not obvious to a newcomer what the existing API functions (such as HAL_TIM_OC_Start_DMA) are intended for. It takes quite a lot of spleunking to figure out what the HAL functions are tailored. In particular, I think it's confusing to most people that the API ties the *source* of the DMA *trigger* (e.g. timer event X), to the nature (src/target) of the DMA request (e.g. streaming capture values to memory). The hardware doesn't force this,

so no wonder people are baffled when the HAL API does.

3. It seems like this would be relatively simple fix. What is requires is just a generalized version of  HAL_TIM_IC_Start_DMA/HAL_TIM_OC_Start_DMA, which takes the source/target register address as an argument. perhaps HAL_TIM_IC_Start_DMA/HAL_TIM_OC_Start_DMA can even be rewritten to simply call the generic function to eliminate some code duplication.

4. Finally, an separately, I think there's an inconsistency in the way CubeMX handles DMA to different peripherals,

and how this is integrated with the HAL. Consistency is important in order to simplify learning. In my experience, users expect the flow to be "choose a DMA trigger, choose a DMA channel, choose dest/src". In fact, support for triggering DMA from a timer is different between peripherals, and between events.

You can bind a channel with TIMx_CHy or TIMx_UP (for example) from the timer periph's DMA tab, and in the GUI this looks consistent. But the HAL doesn't treat these two equally. Using TIMx_UP gets no dedicated API (you have to call Base_Start and DMA_start yourself), while the IC/OC case gets a dedicated API, which silently assumes that since you want to use the IC/OC as a trigger you must also want to use them as a data src/target (for writing capture values to memory, or reprogramming the compare value for waveform generation). And, if you want to use a timer to do DMA->DAC, it's different again. the DAC peripheral GUI lets you specify the timer as a (conversion) trigger, and if you assign a DMA channel (and call the associated HAL API), the DAC itself will trigger the DMA (AFAICT).  For each of these the users need to figure out the specifics of each as a special case, and this makes the learning curve so much steeper. I've been through each of these cases myself, and I can attest that It takes a lot of effort to find your way though this maze of special cases.

I hope you're listening,

Barry

[BarryWhit]

@Saket_Om wrote:

> Yes, we do have an internal process for validating and testing our APIs.

What I meant was, is it possible to release the API in a future version but mark it "Experimental - subject to change",

so if we flub this, it could be changed?

Additional feedback:

Why is there so much duplication between these two lines?

ConfigureDMAChannel(_, config->PeripheralAddress, config->MemoryAddress, config->DataLength);
HAL_DMA_Start(_, (uint32_t)config->MemoryAddress, (uint32_t)config->PeripheralAddress, config->DataLength);

I think ConfigureDMAChannel was intended to set things like width/increment, circular, direction, and this was just a copy-paste mistake. But for DMA code generation, these are already set by one of the Init functions, isn't it?? (I vaguely recall some DMA_MX_init function or other doing that). Isn't there already a HAL function for this which

this init function calls (I don't recall)?  HAL_TIM_Flexible_DMA_Start's docstring should state "this function assume the DMA's X/Y/Z has already been configured with a call to FOOBAR, as part of the peripheral initialization code".

I also think something like this would stay closer to existing HAL style conventions (note different signature))

struct DMA_Trigger_Config {
    TIM_HandleTypeDef *htim;          // Handle to the timer
    uint32_t Channel;                 // Timer channel acting as the DMA request source
    uint32_t TimerEvent;              // Timer event used to trigger the DMA
};
 
HAL_StatusTypeDef HAL_TIM_Flexible_DMA_Start(
 DMA_Trigger_Config *config,
 PeripheralAddress, MemoryAddress, DataLength) {

    ConfigureTimerForDMA();
    HAL_DMA_Start();
    ...
}

- a struct named "DMA_Trigger_Config" should encapsulate config for the DMA trigger only, not bake in src/dest/length. (also, what if the user wants to DMA to several buffers in some manual buffering scheme, why force them modify the struct between calls?)

- making that change also makes HAL_TIM_Flexible_DMA_Start's signature in line to the other *_DMA_Start functions.

[BarryWhit]

I revised my initial feedback, correcting my own misconception. Please take a look.