Interrupt based time for AVR. #2428
Conversation
|
This PR helps a lot. See test results in related issue: |
| // recalibrate if we have some time (>100ms) and it was a while when we did it last time. | ||
| if d > 100000 { | ||
| now := waitTill - d | ||
| if nextTimerRecalibrate < now { | ||
| nextTimerRecalibrate = now + timerRecalibrateInterval | ||
| machine.AdjustMonotonicTimer() | ||
| } | ||
| } |
There was a problem hiding this comment.
@aykevl, you had added TODO comment some time back, and I have added code to recalibrate timer with interval 1 minute when sleep is called for 100ms or more. Do you think one minute interval is too short?
There was a problem hiding this comment.
I doubt we need to recalibrate more often. Pretty sure one minute is sufficient.
There was a problem hiding this comment.
But AdjustMonotonicTimer doesn't calibrate anything? Therefore calling it every minute is totally unnecessary.
In my comment I was referring to calibrating the WDT based on the timer. The timer is already accurate (or as accurate as it can get).
|
This looks like a really great contribution once we can get it merged. Solves a longstanding issue! I left some minor feedback. Also perhaps you want to squash a couple of the commits? |
…the time precision affected when timer-0 reconfigured). Keep all time in nanoseconds. Interrupt based time. Adjust tick cost every 1 minute and when timer-0 is reconfigured (the time precision affected when timer-0 reconfigured). Keep all time in nanoseconds.
|
fixed comments and commits are squash into one. |
|
@aykevl or anyone else have any further feedback before we merge? |
|
Thank you for all your work on this contribution @zdima now merging. |
aykevl
left a comment
There was a problem hiding this comment.
Normally all time related code lives in the runtime. It looks like the only reason it is in the machine package is so that the machine package can call AdjustMonotonicTimer. Perhaps it can be moved to the runtime package and called via a special //go:linkname? That would avoid exposing lots of internal functions in the machine package.
Also, this PR is probably going to increase power consumption and possibly reduce performance because of the constant interrupts. That may be a reasonable tradeoff, but I'm mentioning it here just in case.
| // recalibrate if we have some time (>100ms) and it was a while when we did it last time. | ||
| if d > 100000 { | ||
| now := waitTill - d | ||
| if nextTimerRecalibrate < now { | ||
| nextTimerRecalibrate = now + timerRecalibrateInterval | ||
| machine.AdjustMonotonicTimer() | ||
| } | ||
| } |
There was a problem hiding this comment.
But AdjustMonotonicTimer doesn't calibrate anything? Therefore calling it every minute is totally unnecessary.
In my comment I was referring to calibrating the WDT based on the timer. The timer is already accurate (or as accurate as it can get).
The "calibrate" in this case means adjust the increment value for each interrupt, which depends on timer0 settings.
Agree. User can address this by changing timer0 settings to reduce interrupt intervals. This will reduce power consumption (also may affect accuracy of timer, which is another tradeoff). |
Yes, that would be better. I didn't want to expose those internal functions and that would save me some time :( |
|
Looks like this PR broke
Yes, but this is true for many other things. If users use hardware that is used by the runtime (such as Timer0), they should not expect the runtime to magically deal with that. Therefore, this is in my opinion unnecessary.
A timer will always increase power consumption, even at low frequency. This varies by chip of course. Other chips (such as nrf) have dedicated low-power clocks for this purpose running from a separate crystal. Anyway, just wanted to mentioned this to note as a side effect. |
Simplify the interrupt-based timer code in a few ways:
- Do not recalibrate the timer every 100ms. Instead, rely on the fact
that the machine package will calbrate the timer if necessary if it
makes changes to Timer0.
- Do not configure Timer0 and then set nanosecondsInTick based on that
value. Instead, use a fixed value.
These two changes together mean that in code that doesn't use PWM,
nanosecondsInTick will be constant which makes the TIMER0_OVF interrupt
handler a lot smaller.
Together this reduces the code size of AVR binaries by about 1200 bytes,
making it pretty close to the pre-timer code size (only about 250 bytes
larger).
It also somehow fixes a problem with
tinygo.org/x/drivers/examples/ws2812 on the Arduino Uno. I'm not quite
sure what was going wrong, but bisecting pointed towards the timer code
(#2428) and with this
simplification the bug appears to be gone.
Simplify the interrupt-based timer code in a few ways:
- Do not recalibrate the timer every 100ms. Instead, rely on the fact
that the machine package will calbrate the timer if necessary if it
makes changes to Timer0.
- Do not configure Timer0 and then set nanosecondsInTick based on that
value. Instead, use a fixed value.
These two changes together mean that in code that doesn't use PWM,
nanosecondsInTick will be constant which makes the TIMER0_OVF interrupt
handler a lot smaller.
Together this reduces the code size of AVR binaries by about 1200 bytes,
making it pretty close to the pre-timer code size (only about 250 bytes
larger).
It also somehow fixes a problem with
tinygo.org/x/drivers/examples/ws2812 on the Arduino Uno. I'm not quite
sure what was going wrong, but bisecting pointed towards the timer code
(#2428) and with this
simplification the bug appears to be gone.
Timer-0 is used as interrupt source.
Keep all time in nanoseconds to simplify the math and increase performance.
Adjust tick cost when timer-0 is reconfigured by PWM (the time precision affected when timer-0 reconfigured).