HardwareTimer incomplete?

[HendryKaak]

Hey,
I've been struggling with the implementation of the current HardwareTimer class with a custom stm32f401 board and would like to share a few points that caused some problems here and there.

What i've been trying to do is using the HardwareTimer class to setup a PWM output for a buzzer and used a specific timer channel to do so. But the first problem I encountered was that the timer or channel was not disabled when the Pause() function is called. Looking at the code I found out that in the HAL stm32f4xx_hal_tim.h, the timer would only be disabled when all timer channels are disabled:

Arduino_Core_STM32/system/Drivers/STM32F4xx_HAL_Driver/Inc/stm32f4xx_hal_tim.h

Lines 1022 to 1031 in 1e2a598

	#define __HAL_TIM_DISABLE(__HANDLE__) \
	do { \
	if (((__HANDLE__)->Instance->CCER & TIM_CCER_CCxE_MASK) == 0UL) \
	{ \
	if(((__HANDLE__)->Instance->CCER & TIM_CCER_CCxNE_MASK) == 0UL) \
	{ \
	(__HANDLE__)->Instance->CR1 &= ~(TIM_CR1_CEN); \
	} \
	} \
	} while(0)

So disabling the timer never happens because the HAL_TIM_OC_Stop macro is never called with the correct timer channel in the pause() function.

Arduino_Core_STM32/cores/arduino/HardwareTimer.cpp

Lines 101 to 109 in 1e2a598

	/**
	* @brief Pause HardwareTimer: stop timer
	* @param None
	* @retval None
	*/
	void HardwareTimer::pause()
	{
	HAL_TIM_Base_Stop_IT(&(_timerObj.handle));
	}

Only the interrupts are being stopped over there as you can see, but never the channel itself and thus the timer is never stopped.

To resolve this:

• At least give the option to disable a timer channel, but leave the timer enabled when another Input Capture/Output Capture channel is still in use, so that concurrent channel can be used.
• It would probably be a good idea to be able to disable independent channels in place of enabling all of the channels at once (also for concurrent timer channel usage).
• I also wonder about the purpose of pause() and resume(). Essentially, these functions really stop/disable and start/enable the timer, where pause would suggest it just stops the timer without reconfiguring anything (in particular, I would not expect pause to change the current pin output), Perhaps these functions should be renamed and/or better documented? Maybe real pause/resume methods should be added?

Another thing that I found questionable is the setMode() function. AFAIK is that the channels modes are first stored in RAM, but only written to registers when resume() is called. This means that the only way to change a channel's configuration is by restarting the timer. It might be feasible to sometimes reconfigure a channel while the timer is running (an obvious usecase is to disable or re-enable a single channel without affecting the others, but also changing other setttings on the fly would make sense). Maybe setMode should just apply the changes directly? As long as the timer is disabled, there should be no harm in applying the channel settings, I believe?

[fpistm]

@ABOSTM
Any thought on that?

[ABOSTM]

I @HendryKaak, I agree that this is a bug, timer should really be stopped when pause() is called,
And I think it is a good enhancement to propose also an API to pause only 1 channel.

Thus my proposal would be to have 2 API:
void HardwareTimer::pause() // Disable all channel
void HardwareTimer::pauseChannel(channel) // disable only 1 channel and disable corresponding Capture/Compare interrupt only

As far as setMode() is concerned, it is not possible to write directly the register:
in the following example:

  MyTim->setMode(channel, TIMER_OUTPUT_COMPARE_PWM1, pin);
  // MyTim->setPrescaleFactor(8); // Due to setOverflow with MICROSEC_FORMAT, prescaler will be computed automatically based on timer input clock
  MyTim->setOverflow(100000, MICROSEC_FORMAT); // 10000 microseconds = 10 milliseconds
  MyTim->setCaptureCompare(channel, 50, PERCENT_COMPARE_FORMAT); // 50%
  MyTim->attachInterrupt(Update_IT_callback);
  MyTim->attachInterrupt(channel, Compare_IT_callback);
  MyTim->resume();

the timer periperal clock (the clock of register) is enabled only on resume() (thanks to HAL_TIM_Base_Init()). Thus it is not possible to write register on setMode().
For information, I inherit this behavior, as well as pause/resume behavior, from danieleff implementation (see #146 (comment))

[HendryKaak]

I was wondering: Is your idea for the HardwareTimer class to maintain the API (mostly) as is, or if it's OK to apply some bigger adjustments on the API? It seems the code has only been fairly recently added, so I would think some needed but incompatible changes would be acceptable?

Thus my proposal would be to have 2 API:
void HardwareTimer::pause() // Disable all channel
void HardwareTimer::pauseChannel(channel) // disable only 1 channel and disable corresponding Capture/Compare interrupt only

Your comment on pauseChannel() suggests that the channel is being disabled, so why not call the function the same as what the function does? I think disableChannel() would be a better name in this case. The same thing applies to the resumeChannel() function, which also only enables a specified timer channel. This function should probably also be renamed to enableChannel().

the timer periperal clock (the clock of register) is enabled only on resume() (thanks to HAL_TIM_Base_Init()). Thus it is not possible to write register on setMode().
For information, I inherit this behavior, as well as pause/resume behavior, from danieleff implementation (see #146 (comment))

Is it not possible to set the clock earlier on in the code? Alternatively, an idea would be to set the channel config immediately when the timer is disabled, but if the timer is enabled leave the configuration for later (like now)?

[fpistm]

@LinoBarreca
any comment on this?

[LinoBarreca]

@fpistm I honestly read this but then I didn't because I have mixed feelings. HardwareTimer is definitely incomplete so I agree with @HendryKaak ...BUT (and I wrote it uppercase because it's very big) that's how it should be: simple.

Exposing through the HardwareTimer all the characteristics of the underlying TIM devices would make the class a nightmare (and that's why I disagree with @HendryKaak).
Actually the timers can do things way more complicate than outputting a "clock" (square wave, on/off).
Properly manipulating the registers can make timer channels and timers interact with each other so that you could have different output waveforms.
@HendryKaak just have a look here
While the device can definitely do this, I wouldn't recommend trying to implement such advanced functionalities. What could be nice, instead, is to expose through the class, the registers...
(well..actually it can be done as well since you can mix HAL_ calls to the HardwareTimer calls...but this means that you would lose the "control" on the object. The HardwareTimer doesn't know what the user did with the registers. Exposing them to the user allows the user to manipulate the registers THROUGH the HardwareTimer and let's the class know what the user did and properly manage it internally when it's possible)

@HendryKaak I understand you have a problem and maybe there should be a way to. But the workaround is easy: Instead of using the channel to give the output, use the arduino way: in channel ISR you digitalWrite(!digitalRead(CLOCK_PIN)) when you want to stop the clock (or the wave, no matter how you call it) you just detachInterrupt().
Moreover...I'd say that, from what you explained, it seems you're doing it wrong......There's a Tone library. It's designed to do exactly what you want to do. Without the complexity of handling "manually" the channels and the timers.

So....I didn't comment because what I said doesn't really add something useful to the issue..because I don't really have an opinion besides "let's keep it simple" (and his suggestion of void HardwareTimer::pauseChannel(channel) actually seems kinda "easy to understand") 😅

[ABOSTM]

Hi @LinoBarreca , thanks you for sharing your view.
Hi @HendryKaak ,
You are right, the API the recent, and we could think about incompatible change, but I don't feel it necessary for renaming pause/resume.
As I said I think adding HardwareTimer::pauseChannel(channel) is an added value and could be done in future.
Also, as clocking timer peripheral late (in resume) is a pain for several code improvement and is also not intuitive,
I am thinking about enabling timer peripheral clock at HardwareTimer object creation, but this will add extra rework on the whole HardwareTimer library (probably some class attributes would become useless ...), and maybe duplicate call to timer peripheral (in HardwareTimer library, and in timer library)...

[HendryKaak]

Hello @LinoBarreca

Exposing through the HardwareTimer all the characteristics of the underlying TIM devices would make the class a nightmare (and that's why I disagree with @HendryKaak).
Actually the timers can do things way more complicate than outputting a "clock" (square wave, on/off).
Properly manipulating the registers can make timer channels and timers interact with each other so that you could have different output waveforms.
@HendryKaak just have a look here
While the device can definitely do this, I wouldn't recommend trying to implement such advanced functionalities. What could be nice, instead, is to expose through the class, the registers...
(well..actually it can be done as well since you can mix HAL_ calls to the HardwareTimer calls...but this means that you would lose the "control" on the object. The HardwareTimer doesn't know what the user did with the registers. Exposing them to the user allows the user to manipulate the registers THROUGH the HardwareTimer and let's the class know what the user did and properly manage it internally when it's possible)

I agree that exposing most of the HAL functionality through HardwareTimer is a lot of work to maintain. However, even when using the HAL directly for advanced features, I would rather still use the HardwareTimer class for the easiness of certain functions (such as Arduino pin to ST pin assignment), configure the channels for example without to much hassle of searching through the HAL files (which is nicely done right now) and still have the option available to setup custom timer settings.

IIUC you also think so, but you suggest to do all register manipulations through the HardwareTimer class, so the HT class can keep its internal state consistent. While that is probably a good idea in theory, in practice it is probably not feasible because 1) that would prevent using the existing HAL functions to modify the registers (at least AFAICS) and 2) it is generally tricky to parse all the register bits and deduce the current configuration from that.
It might be more feasible to expose the TIM_HandleTypeDef for custom HAL configuration. This way I and many other users can use the HardwareTimer together with other timer settings if they like. And if users break there code by using these more advanced functions, it is really their own problem to fix that. They should stick to the HardwareTimer functionality if they cannot figure out how the HAL can be used. However, the HardwareTimer can of course try to be friendly to external changes by e.g. not making too much assumptions (maybe also reading back registers where appropriate, for a light version of your proposal).
I'll see if I can make a PR for the 'getHandle' function.

@HendryKaak I understand you have a problem and maybe there should be a way to. But the workaround is easy: Instead of using the channel to give the output, use the arduino way: in channel ISR you digitalWrite(!digitalRead(CLOCK_PIN)) when you want to stop the clock (or the wave, no matter how you call it) you just detachInterrupt().

Using the timer with interrupts with digitalWrites in it gives a lot of overhead (and process time overhead) and jitter that the timer can properly do itself. This is not the way it should be done in my opinion. This is why MCU's have the output compare on pins for in the first place, which can do zero-overhead zero-jitter output generation. For the same reason, using Tone is not an option for us.

@ABOSTM

You are right, the API the recent, and we could think about incompatible change, but I don't feel it necessary for renaming pause/resume.

That's OK I guess. It's just that I prefer to keep function names as clear as possible and related to what the function actually does.

As I said I think adding HardwareTimer::pauseChannel(channel) is an added value and could be done in future.

Thanks, that would be great.

Also, as clocking timer peripheral late (in resume) is a pain for several code improvement and is also not intuitive, I am thinking about enabling timer peripheral clock at HardwareTimer object creation, but this will add extra rework on the whole HardwareTimer library (probably some class attributes would become useless ...), and maybe duplicate call to timer peripheral (in HardwareTimer library, and in timer library)...

Yes indeed, but it would be a much more comprehensible that way.