Off-by one error in HardwareTimer ticks values?

[matthijskooijman]

The HardwareTimer class applies a -1 to the timer tick values it sets. e.g. in setOverflow():

Arduino_Core_STM32/cores/arduino/HardwareTimer.cpp

Lines 465 to 474 in cc05df9

	case HERTZ_FORMAT:
	period_cyc = getTimerClkFreq() / overflow;
	Prescalerfactor = (period_cyc / 0x10000) + 1;
	LL_TIM_SetPrescaler(_timerObj.handle.Instance, Prescalerfactor - 1);
	ARR_RegisterValue = (period_cyc / Prescalerfactor) - 1;
	break;
	case TICK_FORMAT:
	default :
	ARR_RegisterValue = overflow - 1;
	break;

And similarly in setCaptureCompare().

This is a bit unexpected. When using TICK_FORMAT, I would expect that the value you pass is set into the hardware directly. One might say that the tick number passed to these function identifies the number of the tick and is 1-based and must be translated to 0-based when writing to the register, but I did not expect this. I passed 0 to this function only to find that the capture never triggered, apparently since -1 was written to the register.

Furthermore, when using TICK_MICROSECONDS, it would also make sense to pass 0. If you do that now, the -1 also causes an underflow and the compare never triggers. This also happens when you pass a non-zero but low value (depending on the value of the prescaler), so just passing a fixed low value is not always helpful either.

So: I would think that all these -1's should just be removed? Or is there any compelling argument I'm missing?

[ABOSTM]

Hi @matthijskooijman,
From my point of view, TICK_FORMAT means that the value correspond to the number of ticks expected by the user. So 1 in TICK_FORMAT means you want 1 tick. A period of 0 tick is a non sense.
Then Hardware register is 0-based. So We need to write 0 in the register to get 1 tick.
Aim of the API is to abstract hardware complexity to the user, so he should not care about 0-based or 1-based, simply :

• 1 TICK_FORMAT = 1 tick
• 1 MICROSEC_FORMAT = 1 microsecond

I would not change that.
The change you request means 1 MICROSEC_FORMAT = 2 microsecond !!
This is not acceptable for me for an Arduino API.

The same for MICROSEC_FORMAT:
A period of 0 microsecond is non sense for me.

[matthijskooijman]

So 1 in TICK_FORMAT means you want 1 tick. A period of 0 tick is a non sense.

This is probably true for overflow. I checked the datasheet, and indeed, the timer counts up to and including the overflow value, so to get 1 tick period, you would need to write 0 to the register. So, agreed, for overflow applying the -1 for TICK_FORMAT is the right thing to do.

I was going to argue that for MICROSEC_FORMAT, you could get an underflow when passing a small us value, but looking at it again this underflow really only happens when you pass a value of 0 (since for other small values the prescaler will simply be adjusted to get a bigger period in ticks). Maybe if the timer input frequency is in the kHz range, this might occur, but I guess that's quite a corner case.

However, for the capture/compare register, which is where I noticed this bug, things are I think different. If I pass (1, TICK_COMPARE_FORMAT), I would expect to get a compare match when the counter reaches 1, not 0. If I pass (0, TICK_COMPARE_FORMAT), I would expect a compare match when the counter reaches 0, not 0xffff (-1).

Similarly, if you pass a lower us value, the underflow could also happen. Say the prescaler is configured so that 1 tick is 10us, then passing any us value less than 10 will cause the CC register to underflow to 0xffff, where I would think it should be 0.

So, it seems I have not thought things through enough when I generalized my issue from CC to overflow and then focused primarily on overflow instead of CC. Sorry for that! Looking at it again, I agree that setOverflow is fine, but I think the -1's need to be removed from setCaptureCompare:

Arduino_Core_STM32/cores/arduino/HardwareTimer.cpp

Lines 690 to 735 in cc05df9

	void HardwareTimer::setCaptureCompare(uint32_t channel, uint32_t compare, TimerCompareFormat_t format)
	{
	int timChannel = getChannel(channel);
	uint32_t Prescalerfactor = LL_TIM_GetPrescaler(_timerObj.handle.Instance) + 1;
	uint32_t CCR_RegisterValue;
	if (timChannel == -1) {
	Error_Handler();
	}
	switch (format) {
	case MICROSEC_COMPARE_FORMAT:
	CCR_RegisterValue = ((compare * (getTimerClkFreq() / 1000000)) / Prescalerfactor) - 1 ;
	break;
	case HERTZ_COMPARE_FORMAT:
	CCR_RegisterValue = (getTimerClkFreq() / (compare * Prescalerfactor)) - 1;
	break;
	case PERCENT_COMPARE_FORMAT:
	CCR_RegisterValue = ((__HAL_TIM_GET_AUTORELOAD(&(_timerObj.handle)) + 1) * compare) / 100;
	break;
	case RESOLUTION_1B_COMPARE_FORMAT:
	case RESOLUTION_2B_COMPARE_FORMAT:
	case RESOLUTION_3B_COMPARE_FORMAT:
	case RESOLUTION_4B_COMPARE_FORMAT:
	case RESOLUTION_5B_COMPARE_FORMAT:
	case RESOLUTION_6B_COMPARE_FORMAT:
	case RESOLUTION_7B_COMPARE_FORMAT:
	case RESOLUTION_8B_COMPARE_FORMAT:
	case RESOLUTION_9B_COMPARE_FORMAT:
	case RESOLUTION_10B_COMPARE_FORMAT:
	case RESOLUTION_11B_COMPARE_FORMAT:
	case RESOLUTION_12B_COMPARE_FORMAT:
	case RESOLUTION_13B_COMPARE_FORMAT:
	case RESOLUTION_14B_COMPARE_FORMAT:
	case RESOLUTION_15B_COMPARE_FORMAT:
	case RESOLUTION_16B_COMPARE_FORMAT:
	CCR_RegisterValue = ((__HAL_TIM_GET_AUTORELOAD(&(_timerObj.handle)) + 1) * compare) / ((1 << format) - 1) ;
	break;
	case TICK_COMPARE_FORMAT:
	default :
	CCR_RegisterValue = compare - 1;
	break;
	}
	__HAL_TIM_SET_COMPARE(&(_timerObj.handle), timChannel, CCR_RegisterValue);
	}

Looking there a bit closer, I noticed that the percent value does not have the -1, but instead uses the ARR + 1:

Arduino_Core_STM32/cores/arduino/HardwareTimer.cpp

Lines 707 to 708 in cc05df9

	case PERCENT_COMPARE_FORMAT:
	CCR_RegisterValue = ((__HAL_TIM_GET_AUTORELOAD(&(_timerObj.handle)) + 1) * compare) / 100;

I wonder if that is correct? Maybe this is intended to reverse the -1 applied in setOverflow, so using the period rather than the maximum timer value in this calculation. However, I think this will break when passing 100%: (ARR + 1) * 100 / 100 = ARR + 1, a value that the timer will never reach (so 100% will never match, which is probably not what is intended). What the correct calculation is probably depends on how you want to do rounding , but I think the current one is not correct in any case. I think this also applies to the RESOLUTION_xB_FORMAT handling.

[ABOSTM]

Hi @matthijskooijman,
So we agree on setOverflow(). Fine.
And as you said I should protection against underflow.

Now lets discuss about compare.
You are right, ... at least partialy:

• Yes I need to remove -1 for MICROSEC_COMPARE_FORMAT, HERTZ_COMPARE_FORMAT and TICK_COMPARE_FORMAT.
  (probably a bad copy/patse from setOverflow(); remark: there was no reverse +1 in the getCaptureCompare())
  By the way I also remove -1 in setCount(). Same bad copy/paste.

• For PERCENT_COMPARE_FORMAT, ARR + 1 is normal: The real period is ARR + 1. This is the reverse of -1 applied in setOverflow().
  No need to remove -1 (there is none).
  Then it is a little bit tricky about 100% ==> ARR+1:
  Let's use PWM1 output mode as example, from hardware point of view (GPIO Timer output),
  if CaptureCompare register CCRx = ARR,
  then PWM output signal will be set to HIGH until CNT = CCRX = ARR,
  as soon as CNT = CCRx = ARR output signal is set LOW for 1 Tick, until CNT = ARR+1 =0.
  In other words, the last tick is LOW and we cannot reach 100%.
  One solution is to set CCRx strictly greater than ARR, and in this case output signal is 100% HIGH. Thus in general ARR +1 is good ...
  ... But this highlight a new issue:
  what happens if ARR is set to the max value = 0xFFFF? ARR+1 means 0x10000.
  On 32bit timer it will work (even if HardwareTimer consider all timer as 16bit).
  On 16bit timer we will set 0 --> 0%. So, in this case we are not able to reach really 100%.
  But having 0% is not acceptable, so I will patch to check this specific case and set CCRx=0xFFFF.
  It means 100% is not exactly reach but we lose only 1 tick out of 65535 => 99,998..%
  (the other solution for application is to change mode to TIMER_OUTPUT_COMPARE_FORCED_ACTIVE).

• For RESOLUTION_xB_FORMAT it is more or less the same:
  let's take a simple example: RESOLUTION_8B_FORMAT [0..255]. with ARR = 255. and Compare value max: 255
  (ARR+1)* compare/(1<<8 -1) = 256 * 255/255 = 256 = ARR+1 . This is similar to PERCENT_COMPARE_FORMAT.

Thanks for highlighting those issues.

[matthijskooijman]

Yes I need to remove -1 for MICROSEC_COMPARE_FORMAT, HERTZ_COMPARE_FORMAT and TICK_COMPARE_FORMAT.
Agreed.
By the way I also remove -1 in setCount(). Same bad copy/paste.
Good point, had not even noticed that.

One solution is to set CCRx strictly greater than ARR, and in this case output signal is 100% HIGH. Thus in general ARR +1 is good ...

Ah, good point. When using PWM, 100% should indeed mean "never reach compare match", so the current code is ok for that.

I was more thinking not from a PWM pov, but e.g. when using toggle on compare match. Then, when you set the compare register at 100%, I would expect to trigger compare match at the very end of the timer cycle, rather than never. But, thinking on this a bit more, I guess if you really need that, you can just use setCaptureCompare(getOverflow() - 1), especially since only 100% has this problem, 99% will always generate a match, even with a very large or very small overflow value. So I agree to leave these as-is, best to leave the PERCENT (and RESOLUTION) formats usable for meaningful PWM output indeed.

what happens if ARR is set to the max value = 0xFFFF? ARR+1 means 0x10000.
But having 0% is not acceptable, so I will patch to check this specific case and set CCRx=0xFFFF.

Good point indeed. Agreed on the solution, it is not perfect, but probably acceptable.

Thanks for following up on this :-)

[Laserjones]

Hello! I'm a beginner and I only partially understand the above discussion. I'm still not sure whether the CaptureCompare range to be used in the code is really 1..0x10000 instead of 0..0xFFFF as the documentation says. That would mean that I would have to set CaptureCompare=1 for a duty cycle of 0 ticks (i. e. no output voltage, which must be possible of course)? That's a bit counterintuitive. Best regards, Joerg