setInterval on a running timer results in a period significantly longer than the specified period

[stripwax]

Describe the bug

Changing the timer period incurs additional unexpected delays, resulting in actual period being longer than the specified period (for the period in which it was changed). Behaviour observed with TC3 on SAMD21.

Steps to Reproduce

From inside the ISR, call setInterval . In my usecase, my ISR is toggling an output bit and then calling setInterval (for a variable-period output signal). By measuring the output, you can determine the "actual" period length or frequency.

Expected behavior

Expect the period of a square wave output to be equal (or very very close) to what was specified in the call to setInterval

Actual behavior

The period of the square wave is approximately 100us longer than the specified period.

Information

Seeeduino Xiao M0 (SAMD21)

[stripwax]

Interpretation

As I understand it, what's actually happening is as follows:

1. Through a few intermediate calls, calling setInterval will end up at setPeriod_TIMER_TC3 .
2. setPeriod_TIMER_TC3 will (among other things) disable the timer, change the prescaler, attempt to remap Count, and then re-enable the timer.
3. My interpretation is that the instructions to read the Count register are incorrect, because the RREQ hasn't been sent, and as a result Count is always zero. In other words, the Count is (effectively) reset to 0 every time setInterval is called.
4. This effectively extends the period by the length of the setInterval execution, which seems to be approximately 100us.
5. Through experimentation, it seems that Count will read 0 when the timer is disabled. The suggested remedy is therefore to issue RREQ before disabling the timer, and read Count before disabling the timer, and then writing the (remapped) Count back again after changing the prescaler
6. It's also possible that setting the Count before setting CC[0] could cause the Count to prematurely reset to zero (per docs, "Count is compared continuously to CC[0] and attempting to set Count to a value greater than CC[0] will immediately reset Count to zero". The suggested remedy is to first set CC[0] to 0xffff (uint16_t max), then set the (remapped) Count, then set CC[0] to the new correct _compareValue
7. I also believe that the map() call is incorrect. It appears to have been adopted from an article about smooth frequency sweeps, and therefore interpolating between two DIFFERENT frequencies makes sense in that use case, but does not make sense when you want to change a period of a waveform to a specified period. The specific issue here is that the Count might be remapped inappropriately, to a value either higher or lower than it should, since the arguments of the map() call result in remapping Count from the old 0-CC range, to the new 0-_compareValue range, instead of remapping Count purely based on the change (if any) of the prescaler. In other words, it is my opinion that, if the prescaler did not change (and CC[0] did change), then Count should not be remapped at all; and if the prescaler did change then the value of compareValue itself should be irrelevant to how to rescale Count. The suggested remedy is to (a) keep track of the prior prescaler and compare to the new prescaler,(b) if prescaler unchanged then do not change Count (and as a bonus, no need to disable the timer because the only reason the timer is disabled is to change the prescaler), (c) if prescalar is changed then use the change to scale up or down Count via left (or right) bitshift

I attempted some local changes with the some of the suggested remedies and can confirm that the following appears to eliminate most of the 100us offset:

1. Prior to disabling the timer, issue RREQ and read Count into a local variable.
2. After disabling the timer, rewrite Count using map()

I did not yet try the other ideas (using change in prescaler, rather than change in CC[0], to do the correct map; or avoiding disabling timer entirely if prescaler didn't change)

[stripwax]

#include "SAMDTimerInterrupt.h"

SAMDTimer ITimer(TIMER_TC3);
//SAMDTimer ITimer(TIMER_TCC);

volatile uint32_t myClockTimer = 0, lastMicros = 0;

const int nPeriods = 12;
volatile const uint32_t periods[nPeriods] = {2272,2272,2272,2272,1136,2272,568,999,1001,999,2272,2272};
volatile uint32_t actual_periods[nPeriods] = {0,0,0,0,0,0,0,0,0,0,0,0};
volatile int p_i = 0;

void clock(void)
{
  myClockTimer = micros() - lastMicros; //2us come from here
  lastMicros = micros();
  if (p_i<nPeriods) {
    actual_periods[p_i] = myClockTimer;
    p_i++;
  }
  if (p_i<nPeriods) {
    unsigned long p = periods[p_i];
    ITimer.setInterval(p, clock);
  }
  else
  {
    ITimer.setInterval(10000000, clock);
  }
}

void setup()
{
  Serial.begin(115200);
  while (!Serial) delay(10);

  Serial.print(F("\nStarting setInterval validation on ")); Serial.println(BOARD_NAME);
  Serial.println(SAMD_TIMER_INTERRUPT_VERSION);
  Serial.print(F("CPU Frequency = ")); Serial.print(F_CPU / 1000000); Serial.println(F(" MHz"));
}

void loop()
{
  lastMicros = micros();
  unsigned long p = periods[0];
  ITimer.attachInterruptInterval(p, clock);
  delay(5000); // more than sufficient, given the timings provided in periods[]
  for(int i=0; i<nPeriods; i++) {
    Serial.print(F("myClockTimer "));
    Serial.print(periods[i]);
    Serial.print(F(", actual = "));
    Serial.println(actual_periods[i]);
  };
  while(1) delay(5000);
}

Starting setInterval validation on __SAMD21G18A__
SAMDTimerInterrupt v1.6.0
CPU Frequency = 48 MHz
myClockTimer 2272, actual = 2381
myClockTimer 2272, actual = 2380
myClockTimer 2272, actual = 2381
myClockTimer 2272, actual = 2381
myClockTimer 1136, actual = 1244
myClockTimer 2272, actual = 2381
myClockTimer 568, actual = 677
myClockTimer 999, actual = 1111
myClockTimer 1001, actual = 1119
myClockTimer 999, actual = 1111
myClockTimer 2272, actual = 2380
myClockTimer 2272, actual = 2380

[stripwax]

Similar observation, but slightly smaller delay, when using TCC on SAMD21:

Starting setInterval validation on __SAMD21G18A__
SAMDTimerInterrupt v1.6.0
CPU Frequency = 48 MHz
myClockTimer 2272, actual = 2354
myClockTimer 2272, actual = 2355
myClockTimer 2272, actual = 2355
myClockTimer 2272, actual = 2355
myClockTimer 1136, actual = 1218
myClockTimer 2272, actual = 2355
myClockTimer 568, actual = 651
myClockTimer 999, actual = 1082
myClockTimer 1001, actual = 1084
myClockTimer 999, actual = 1081
myClockTimer 2272, actual = 2355
myClockTimer 2272, actual = 2355

[stripwax]

With my forked version of SAMDTimerInterrupt I achieve the following, which is a much better result. With my changes, in addition to the READREQ fix, I also avoid changing REG_GLCK_CLKCTRL when the timer has already been initialized, and also avoid additional writes to CTRLA for disabling the clock, setting 16bit mode, and setting match mode, since those are all already set. My earlier analysis of the cause (and therefore the necessary fix) might have been incorrect or incomplete, and I now suspect that what is happening is that some of the initialization calls that happen inside setFrequency are contributing to the resetting or delaying of COUNT. As a result, I'm not really certain which of my changes are really the fix for this issue (noting that my fork also includes a fix for #18).

Results for TC3. (my fork doesn't include all the corresponding TCC fixes, just TC3)

Starting setInterval validation on __SAMD21G18A__
SAMDTimerInterrupt v1.7.0
CPU Frequency = 48 MHz
myClockTimer 2272, actual = 2358
myClockTimer 2272, actual = 2276
myClockTimer 2272, actual = 2275
myClockTimer 2272, actual = 2276
myClockTimer 1136, actual = 1181
myClockTimer 2272, actual = 2193
myClockTimer 568, actual = 634
myClockTimer 999, actual = 938
myClockTimer 1001, actual = 1005
myClockTimer 999, actual = 1003
myClockTimer 2272, actual = 2171
myClockTimer 2272, actual = 2276

As you can see, the initial period does last longer than expected (and this occurs when initialized=false) - maybe this is normal and unavoidable but it is still not ideal. With my other changes, the latency on that first call has been reduced, since that first period is now 2358 rather than 2381, but still considerably higher than the requested 2272. The repeated 2272 periods are very close (i.e. +4 us) to the expected period. Changing the prescaler seems to cause additional unpredictability, which I imagine is consistent with the incorrect rescaling approach being used. I will see if my suggested fixes resolved those discrepancies also.

https://github.com/stripwax/SAMD_TimerInterrupt

[stripwax]

I just tried making a quick version of the prescaler comparison changes I mentioned, and that seems to resolve most of the issue. I've committed that to my fork also. This could certainly be improved further (i.e. constant bitshift rather than while loop - ideally without redundantly tracking both prescaler_bits and 2^prescaler_bits).

Starting setInterval validation on __SAMD21G18A__
SAMDTimerInterrupt v1.7.0
CPU Frequency = 48 MHz
myClockTimer 2272, actual = 2354
myClockTimer 2272, actual = 2269
myClockTimer 2272, actual = 2270
myClockTimer 2272, actual = 2269
myClockTimer 1136, actual = 1133
myClockTimer 2272, actual = 2269
myClockTimer 568, actual = 569
myClockTimer 999, actual = 997
myClockTimer 1001, actual = 1002
myClockTimer 999, actual = 1000
myClockTimer 2272, actual = 2274
myClockTimer 2272, actual = 2270

[khoih-prog]

It seems that you're using the Timer Interrupt in a most weird way I can think of.
Changing the interval inside the interrupt repetitively and measure that only the first interval !!!

That's why I didn't spend time to answer, expecting you to understand by yourself this is not correct way to use. I also won't waste my time to deal with this strange use-case and leave that for your hobby.

I'm closing this issue and won't reopen it until you prove this is a real bug of the library while using it correctly.

[stripwax]

Thanks for the response. The usecase in question (as mentioned in the initial comment) is for a variable-period squarewave. The project in question is TZXDuino/MAXDuino. These are well-established Arduino applications which work on a variety of devices but did not yet include SAMD21 support, which I am adding myself. You can find my changes in my TZXDuino fork.
Without my changes, the periods of the waveform are consistently wrong (too long by approximately 100us). One fix would be to change the code to subtract approx 100us from the period length, but that fix does not make sense to me.
Hence I attempted to investigate the problem in the library I was using (SAMDTimerInterrupt) to add SAMD support to this TZXDuino project.

It is clear you think that my usecase is invalid and that I am using the library wrongly. I am open to your suggestion of how it should be being used correctly to support this usecase. Alternatively, please reconsider reopening this issue and evaluating my changes in my fork.

[stripwax]

Additional comment- the reason my example is only measuring the first interval, is because the first interval is the one that is wrong. The subsequent intervals will be correct - measuring those does not illustrate the problem. To put it another way, the requirement is to generate different numbers of pulses of different duration, and the observed bug is that the duration of the first pulse after a call to setInterval does not match the specified interval.
If there is a more correct way to use your library to achieve this, then I apologise but would still appreciate a hint showing how to do this without encountering the problem I described. Many thanks!

[khoih-prog]

In that use-case where accuracy is very important for even the first interval, using any library, or even the SDK, is the wrong choice. You have to write the registers directly, even using assembly if possible, to avoid overhead in uS range.

No matter how the library is rewritten, it can't never achieve anything accurate enough.

Good Luck,

[stripwax]

The requirement is that the interval after the call to setInterval corresponds to the specified interval - the changes in my fork achieve this. Could you take a look?

…

On Tue, 19 Apr 2022, 15:25 Khoi Hoang, ***@***.***> wrote: In that use-case where accuracy is very important for even the first interval, *using any library, or even the SDK*, is the wrong choice. You have to write the registers directly, even using assembly if possible, to avoid overhead in uS range. No matter how the library is rewritten, it can't never achieve anything accurate enough. Good Luck, — Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABF47HEFPAXQMZSHFUG4QCDVF263ZANCNFSM5TBIKZAA> . You are receiving this because you authored the thread.Message ID: ***@***.***>