fix(airtime): set secSinceBoot from millis() to address inaccurate airtime calculations - #9778
fix(airtime): set secSinceBoot from millis() to address inaccurate airtime calculations#9778m1nl wants to merge 13 commits into
Conversation
Previous implementation relied on OSThread being triggered every second. When device is sleeping or other tasks take more time airtime OSThread may become delayed and calculations will become invalid.
There was a problem hiding this comment.
Pull request overview
This PR updates the airtime/accounting timebase to avoid relying on a 1 Hz OSThread tick, aiming to make airtime/utilization tracking resilient to scheduling delays (sleep/long tasks).
Changes:
- Replaces
secSinceBoot(manually incremented) withmillis()/1000as the uptime source for airtime calculations. - Removes the
secSinceBootmember and its increment inAirTime::runOnce(). - Decreases the
AirTimeOSThread interval from 1000ms to 100ms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/airtime.h | Removes secSinceBoot state from the AirTime class. |
| src/airtime.cpp | Switches getSecondsSinceBoot() to millis()/1000 and adjusts the airtime thread cadence. |
|
In general this sounds good to me. I don't really like that it's running every 100ms, though. There are not a lot of things that are by default running at a certain interval, and in principle we could get rid of it here. We only need to update airtime when receiving or transmitting a packet, and at that time we could check in which "bin" we are with |
I like this idea too! However that's way much complicated change than I proposed. If needed we can restore 1000ms interval, that should not impact airtime calculation accuracy. I can try to redesign airtime calculation in line with idea you brought up in a separate PR. |
In case device sleeps and periods are missed, ensure the windows which are between previous and current window are also cleared.
|
@robekl I applied changes from your comments, please review |
d8b0a6a to
15c9bd8
Compare
This ensures period won't change during thread execution.
15c9bd8 to
9d0c6c9
Compare
m1nl
changed the title
|
|
||
| uint32_t AirTime::getSecondsSinceBoot() | ||
| { | ||
| return this->secSinceBoot; |
There was a problem hiding this comment.
This is what the AI is referring to. Does it make sense to give the millis treatment here too?
There was a problem hiding this comment.
Hm, from what I can see, Copilot seems to question almost every change just to raise concerns. Earlier, @robekl brought up a valid point that we should keep a stable value of secSinceBoot. Small individual drifts of less than a minute don’t matter much; at least we wouldn’t be accumulating the error, which is what’s happening right now.
The change I proposed initially seemed very small to me, but it looks like we might actually benefit from a full rewrite of the airtime calculation logic. I might try rewriting it from scratch, although I’m not sure how welcome those changes would be, since it touches some internals that haven’t been modified in a long time...
The current airtime calculation relies on the OSThread being triggered every second. However, when the device enters sleep mode or other tasks take longer to execute, the airtime OSThread may be delayed, leading to inaccurate calculations.
I propose replacing the
secSinceBoottimer, which is currently incremented inside the airtime OSThread, with themillis()function from the Arduino framework. Themillis()function returns the number of milliseconds since the board began running the current program. According to the documentation,millis()provides a reliable uptime value regardless of whether the device is sleeping or temporarily busy with other tasks, making it a more robust basis for airtime calculations.I've also decreased interval for airtime OSThread to 100ms - I consider this to be a safe value as most of maintenance tasks run with this interval. We don't need to make it equal to 1000ms, when
millis()invocation provides accurate uptime.