-
Notifications
You must be signed in to change notification settings - Fork 16
setInterval on a running timer results in a period significantly longer than the specified period #17
Comments
InterpretationAs I understand it, what's actually happening is as follows:
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:
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) |
|
Similar observation, but slightly smaller delay, when using TCC on SAMD21:
|
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)
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. |
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).
|
It seems that you're using the Timer Interrupt in a most weird way I can think of. 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. |
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. 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. |
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. |
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, |
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:
***@***.***>
|
I will. But first, please help have more tests and post the results in some more important use-cases to see if the change breaks anything. Then when everything seems OK, make a |
Hi @stripwax The new SAMD_TimerInterrupt releases v1.7.0 has just been published. Your contribution is noted in Contributions and Thanks. I'm looking forward to receiving your Best Regards, Releases v1.7.0
|
Updated my test script here:
Output with release version 1.6.0. Note that this shows the timings are wrong for multiple periods, not just the first period, which seems to show that the bug is worse than originally assumed and may have a different root cause (and still needs fixing). Timings are consistent, but consistently wrong by about 110us.
Output with my featurebranch with my changes does indicate a bug of some kind (as evidenced by issue #21 ). See problem on 80000/80001 entries. My output:
|
Fixed in my fork. It looks like you already made the same fix when you applied my idea to your library here 3f496d6 so I'm not sure what the bug is in your version. EDIT oh I see, the same bug in your version. You already found and fixed it, but only in the code for the SAMD51, and not for the SAMD21 - you might want to perhaps consider refactoring it so that the same logic is only implemented once for TC3, not twice.
|
I don't have a SAMD51 device so that isn't going to happen - sorry |
For completeness, here's the test results with 1.9.0
|
Hi @khoih-prog - have you seen the above information? Could you reopen (and consider addressing and fixing via the change in my fork), because the bugs are still present as you can see above. From my test results, maybe you know exactly what the cause is and perhaps my proposed changes are not the correct ones (which is totally fine by me, I don't really care about my own changes, this is your library). but I would like you to understand that there is a bug, and timing is consistently wrong by about 100 us |
Hi @stripwax I'm sorry that I won't merge it because yours is very special use-case and this library is not supposed to be used like that. I suggest you optimize your code to have minimal and consistent time-difference, and if you'd like, write a new library for your waveform-creating use-case, which will be very helpful to certain users. Many thanks for your code anyway. Looking forward to receiving more of your contributions. Regards, |
Ok, thanks for the response. But did you see that the timing error is not
for only the first period but actually every subsequent period too?
I don't think this is a special case. Are you able to reproduce my findings?
…On Mon, 30 May 2022, 15:21 Khoi Hoang, ***@***.***> wrote:
Hi @stripwax <https://github.com/stripwax>
I'm sorry that I won't merge it because yours is very special use-case and
this library is not supposed to be used like that.
I suggest you optimize your code to have minimal and consistent
time-difference, and if you'd like, write a new library for your
waveform-creating use-case, which will be very helpful to certain users.
Many thanks for your code anyway. Looking forward to receiving more of
your contributions.
Regards,
—
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABF47HBTOZ5JLMWD375EZRDVMTFIBANCNFSM5TBIKZAA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sorry that's my final decision. Please don't waste our time, especially your precious time here. I suggest you
I remember that someone requested a similar waveform creating case in certain library (???TimerInterrupt or ???PWM, etc.), and finally settled on using lookup-table, but I don't remember and can't find it now. You can search, if interested. ADDED: Here it is => Duty cycle as integer rather than float #6 Good Luck, |
Noted with thanks. I am sorry you feel this way. You may (or may not) be
interested to know that the popular TimerOne library works correctly in
this regard with other devices. For now I will adopt your earlier
suggestion, which is to NOT use your library.
…On Mon, 30 May 2022, 19:54 Khoi Hoang, ***@***.***> wrote:
Sorry that's *my final decision.*
Please don't waste our time, especially your precious time here.
I suggest you
1. Read *HOWTO Attach Interrupt*
<https://www.arduino.cc/reference/en/language/functions/external-interrupts/attachinterrupt/>
Generally, an ISR should be as *short and fast as possible*. If your
sketch uses multiple ISRs, only one can run at a time, other interrupts
will be executed after the current one finishes in an order that depends on
the priority they have. millis() relies on interrupts to count, so it will
never increment inside an ISR. Since delay() requires interrupts to work,
it will not work if called inside an ISR. micros() works initially but will
start behaving erratically after 1-2 ms. delayMicroseconds() does not use
any counter, so it will work as normal.)
1. Try some examples and see / measure how they're working
2. Use precalculated *lookup* tables, then program the registers
efficiently to minimize the error, if you'd like to create fancy use-cases
for waveform creation, with good enough accuracy.
I remember that someone requested a similar waveform creating case in
certain library (???TimerInterrupt or ???PWM, etc.), and finally settled on
using lookup-table, but I don't remember and can't find it now. You can
search, if interested.
Good Luck,
—
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABF47HG52FNDSWAFLKGPDXLVMUFEPANCNFSM5TBIKZAA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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)
The text was updated successfully, but these errors were encountered: