HardwareTimer: Allow setting preload enable bits

[matthijskooijman]

This PR adds methods to HardwareTimer to change the "preload" setting of the overflow/ARR and CC registers. By enabling preload, updates to these registers can be made at overflow time rather than halfway a period, which can improve the output waveform, as well as prevent missing an overflow event (when decreasing the overflow value to below the current timer value).

These methods are fairly simple and use the LL_TIM HAL functions. I originally implemented this using the higher-level TIM_Base_SetConfig but then found that the prescaler was updated using LL_TIM functions, which did not update _timerObj.handle.Init, so TIM_Base_SetConfig would then revert to a 0 prescaler. So, following the example of setPrescalerFactor, these new methods just write to the registers directly with the LL_TIM functions (which are also more specific, TIM_Base_SetConfig would update a few different registers in addition to the preload enable bit).

[ABOSTM]

Hi @matthijskooijman,
I have several remarks about preload:

• CCRx preload is ENABLED by default by STM32 HAL in functions like HAL_TIM_PWM_ConfigChannel() thus each time setMode() is called
• ARR preload is configured DISABLED thanks to value in constructor thanks to:
  timerObj.handle.Init.AutoReloadPreload = TIM_AUTORELOAD_PRELOAD_DISABLE;

As per the reason you mentioned, I would suggest to change default value in constructor to TIM_AUTORELOAD_PRELOAD_ENABLE
Then I wonder if it is still necessary to propose to disable this feature at Arduino API level ?
If yes, then

1. in setPreloadEnable() for ARR, that is right that we don't need to maintain coherence with value stored in handle:
   Like prescaler, ARR preload is set by HAL only once in constructor thanks to
   HAL_TIM_Base_Init()/TIM_Base_SetConfig() function.
   So it is not mandatory to maintain coherence with value stored in the handle.

2. But for CCRx preload, because it is forced ENABLED by HAL each time setMode() is called, user may be confused if he tries to disable it.
   So I would suggest to no provide ability to change this setting at Arduino API.

From my point of view, this feature is really made for experts, and should not be part of Arduino API.
So my global suggestion would be to enable preload for ARR by default in constructor, and do not provide API to change at Arduino level.
Experts are still able to change that with HAL/LL access.

What do you think about this proposal ?

[matthijskooijman]

As per the reason you mentioned, I would suggest to change default value in constructor to TIM_AUTORELOAD_PRELOAD_ENABLE

I considered that, but I didn't want to change default behaviour. Also, the hardware default is to have preload disabled, so sticking to that might prevent user surprise. On the other hand, if things just work more reliably out of the box, that might also prevent user surpise, so maybe enabled-by-default is indeed better.

But then the question might be: Do we need preload-by-default for CC as well?

Then I wonder if it is still necessary to propose to disable this feature at Arduino API level ?

I'm pretty sure there are applications that need preload disabled, so it is probably best to offer that (even though for our application right now, just flipping the default would have been sufficient).

But for CCRx preload, because it is forced ENABLED by HAL each time setMode() is called, user may be confused if he tries to disable it.
So I would suggest to no provide ability to change this setting at Arduino API.

I had not realized this (only considered that setMode would set a "default" value, but it can of course also change the value later).

Some of this can of course be mitigated by documentation, but that might still leave room for confusion.

Experts are still able to change that with HAL/LL access.

This is true, but direct HAL calls are of course a lot less elegant. If we go that route (or maybe regardless), it might be useful to make the getChannel() and getLLChannel() methods public, since those help to translate between the HardwareTimer channel numbers and HAL channel numbers.

From my point of view, this feature is really made for experts, and should not be part of Arduino API.
So my global suggestion would be to enable preload for ARR by default in constructor, and do not provide API to change at Arduino level.

I can certainly live with this proposal. However, since I already wrote and tested the code, how about adding just the method for ARR, since that one is free of complications, and leaving out the CCRx version to prevent confusion? And changing the default is fine too, of course.

[ABOSTM]

That is fine for me:
changing default value and keeping only ARR method.

[matthijskooijman]

Ok, done. I also added a commit to expose some methods that are useful when talking to the HAL directly, and fixed some coding style issues.

[ABOSTM]

Missing one comment in header.
By the way can you update the wiki API page (new function + new exposed methods):
https://github.com/stm32duino/wiki/wiki/HardwareTimer-library#API
thanks

[matthijskooijman]

I'll update the wiki when it is final (i.e. merged).

[ABOSTM]

LGTM
Thanks