Add 5s and 10s average of ampere to help calibrate ampere sensor. #4656
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds i18n keys for 5s/10s amperage averages, updates motors and power tabs to track rolling amperage histories and compute/display 5s/10s averages, and extends corresponding HTML templates with new rows/elements for these metrics. No removals; existing refresh cycles now update new UI fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Telemetry as Telemetry Stream
participant MotorsTab as Motors Tab JS
participant PowerTab as Power Tab JS
participant DOM as UI Elements
User->>Telemetry: Start data feed
Telemetry-->>MotorsTab: amperage
Telemetry-->>PowerTab: amperage
rect rgba(230,246,255,0.5)
note over MotorsTab,PowerTab: On each refresh tick
MotorsTab->>MotorsTab: push amperage to 5s/10s histories<br/>trim to sizes (20/40)
MotorsTab->>MotorsTab: compute avg5s, avg10s
MotorsTab->>DOM: update motors avg5s/avg10s elements
PowerTab->>PowerTab: push amperage to 5s/10s histories<br/>trim to sizes (25/50)
PowerTab->>PowerTab: compute avg5s, avg10s
PowerTab->>DOM: update battery avg5s/avg10s elements
end
note right of DOM: Labels from i18n (EN/zh_CN)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds 5-second and 10-second rolling averages for amperage readings to help calibrate ampere sensors. The implementation provides more stable amperage readings by calculating moving averages over recent measurements.
Key changes:
- Added amperage averaging logic with configurable history buffers
- Extended UI to display 5s and 10s average amperage values in both power and motors tabs
- Added localization support for the new average amperage labels
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tabs/power.html | Added table rows for displaying 5s and 10s amperage averages |
| src/tabs/motors.html | Added span elements for displaying amperage averages in motors tab |
| src/js/tabs/power.js | Implemented amperage averaging logic and UI updates for power tab |
| src/js/tabs/motors.js | Implemented amperage averaging logic and UI updates for motors tab |
| locales/zh_CN/messages.json | Added Chinese translations for new amperage average labels |
| locales/en/messages.json | Added English translations for new amperage average labels |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const power = { | ||
| supported: false, | ||
| analyticsChanges: {}, | ||
| amperageHistory5s: [], // Store last 5 seconds of amperage readings for average calculation | ||
| amperageHistorySize5s: 25, // Number of readings to keep for 5 seconds average (5Hz * 5s = 25) | ||
| amperageHistory10s: [], // Store last 10 seconds of amperage readings for average calculation | ||
| amperageHistorySize10s: 50, // Number of readings to keep for 10 seconds average (5Hz * 10s = 50) |
There was a problem hiding this comment.
The comment assumes a 5Hz update rate, but this assumption should be documented or made configurable since update rates may vary depending on hardware or configuration.
| const power = { | |
| supported: false, | |
| analyticsChanges: {}, | |
| amperageHistory5s: [], // Store last 5 seconds of amperage readings for average calculation | |
| amperageHistorySize5s: 25, // Number of readings to keep for 5 seconds average (5Hz * 5s = 25) | |
| amperageHistory10s: [], // Store last 10 seconds of amperage readings for average calculation | |
| amperageHistorySize10s: 50, // Number of readings to keep for 10 seconds average (5Hz * 10s = 50) | |
| // Configurable update rate in Hz (readings per second). Change this value if hardware/configuration differs. | |
| const updateRateHz = 5; | |
| const power = { | |
| supported: false, | |
| analyticsChanges: {}, | |
| amperageHistory5s: [], // Store last 5 seconds of amperage readings for average calculation | |
| amperageHistorySize5s: updateRateHz * 5, // Number of readings to keep for 5 seconds average (updateRateHz * 5s) | |
| amperageHistory10s: [], // Store last 10 seconds of amperage readings for average calculation | |
| amperageHistorySize10s: updateRateHz * 10, // Number of readings to keep for 10 seconds average (updateRateHz * 10s) |
| amperageHistorySize5s: 20, // Number of readings to keep for 5 seconds average (4Hz * 5s = 20) | ||
| amperageHistory10s: [], // Store last 10 seconds of amperage readings for average calculation | ||
| amperageHistorySize10s: 40, // Number of readings to keep for 10 seconds average (4Hz * 10s = 40) |
There was a problem hiding this comment.
The motors tab uses different update rate assumptions (4Hz) compared to the power tab (5Hz). This inconsistency could lead to different averaging behaviors between tabs and should be standardized.
| amperageHistorySize5s: 20, // Number of readings to keep for 5 seconds average (4Hz * 5s = 20) | |
| amperageHistory10s: [], // Store last 10 seconds of amperage readings for average calculation | |
| amperageHistorySize10s: 40, // Number of readings to keep for 10 seconds average (4Hz * 10s = 40) | |
| amperageHistorySize5s: 25, // Number of readings to keep for 5 seconds average (5Hz * 5s = 25) | |
| amperageHistory10s: [], // Store last 10 seconds of amperage readings for average calculation | |
| amperageHistorySize10s: 50, // Number of readings to keep for 10 seconds average (5Hz * 10s = 50) |
src/js/tabs/motors.js
Outdated
| const amperageAverage5s = amperageSum5s / TABS.motors.amperageHistory5s.length; | ||
|
|
||
| // 10 seconds average | ||
| TABS.motors.amperageHistory10s.push(currentAmperage); |
There was a problem hiding this comment.
Using TABS.motors instead of self is inconsistent with the pattern used in the power tab and makes the code less maintainable. Consider using self for consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/motors.js (1)
652-678: Consider usingselffor consistency.The amperage averaging implementation is correct and follows sound logic. However, the code uses
TABS.motors.amperageHistory5sandTABS.motors.amperageHistory10sinstead ofself.amperageHistory5sandself.amperageHistory10s. While functionally equivalent, this is inconsistent with the rest of the file, which predominantly usesselfto reference instance properties.Apply this diff to align with the file's conventions:
// 5 seconds average -TABS.motors.amperageHistory5s.push(currentAmperage); -if (TABS.motors.amperageHistory5s.length > TABS.motors.amperageHistorySize5s) { - TABS.motors.amperageHistory5s.shift(); +self.amperageHistory5s.push(currentAmperage); +if (self.amperageHistory5s.length > self.amperageHistorySize5s) { + self.amperageHistory5s.shift(); } -const amperageSum5s = TABS.motors.amperageHistory5s.reduce((sum, value) => sum + value, 0); -const amperageAverage5s = amperageSum5s / TABS.motors.amperageHistory5s.length; +const amperageSum5s = self.amperageHistory5s.reduce((sum, value) => sum + value, 0); +const amperageAverage5s = amperageSum5s / self.amperageHistory5s.length; // 10 seconds average -TABS.motors.amperageHistory10s.push(currentAmperage); -if (TABS.motors.amperageHistory10s.length > TABS.motors.amperageHistorySize10s) { - TABS.motors.amperageHistory10s.shift(); +self.amperageHistory10s.push(currentAmperage); +if (self.amperageHistory10s.length > self.amperageHistorySize10s) { + self.amperageHistory10s.shift(); } -const amperageSum10s = TABS.motors.amperageHistory10s.reduce((sum, value) => sum + value, 0); -const amperageAverage10s = amperageSum10s / TABS.motors.amperageHistory10s.length; +const amperageSum10s = self.amperageHistory10s.reduce((sum, value) => sum + value, 0); +const amperageAverage10s = amperageSum10s / self.amperageHistory10s.length;Optional: Add defensive null check and extract duplicate logic.
Consider adding a guard for
FC.ANALOG.amperageto prevent potentialNaNvalues:const currentAmperage = FC.ANALOG.amperage ?? 0;Additionally, the 5s and 10s averaging logic is duplicated. You could optionally extract this into a helper function to reduce duplication:
function updateAmperageAverage(history, maxSize, currentValue) { history.push(currentValue); if (history.length > maxSize) { history.shift(); } const sum = history.reduce((acc, val) => acc + val, 0); return sum / history.length; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
locales/en/messages.json(3 hunks)locales/zh_CN/messages.json(3 hunks)src/js/tabs/motors.js(4 hunks)src/js/tabs/power.js(3 hunks)src/tabs/motors.html(1 hunks)src/tabs/power.html(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/js/tabs/power.js (2)
src/js/fc.js (1)
FC(132-1006)src/js/localization.js (1)
i18n(7-7)
src/js/tabs/motors.js (2)
src/js/gui.js (1)
TABS(8-8)src/js/localization.js (1)
i18n(7-7)
🔇 Additional comments (10)
src/tabs/power.html (1)
126-133: Battery average rows look good
The new 5 s/10 s rows fit the existing template structure and line up with the IDs consumed inpower.js. 👍src/tabs/motors.html (1)
282-283: Motors power panel update looks solid
Nice addition—labels/classes match the JS bindings for the new rolling averages.locales/zh_CN/messages.json (2)
2242-2262: Thanks for keeping zh_CN in sync
The added keys cover the new amperage averages and follow the existing wording style.Also applies to: 2242-2262
4015-4020: Battery average labels translated consistently
Good to see the power tab labels localized alongside the motors entries.src/js/tabs/power.js (3)
14-17: State additions make sense
Persisting separate 5 s/10 s histories right on the tab state keeps the logic tidy. Good call.
23-26: Nice reset hook
Resetting both histories during tab init prevents stale averages when revisiting the tab.
235-369: Average computation reads clearly
The rolling buffer + reduce pattern is straightforward and pairs cleanly with the new UI rows. 👍src/js/tabs/motors.js (3)
36-39: LGTM!The amperage history size calculations are correct. The 4Hz sampling rate (250ms interval at line 682) multiplied by the time windows yields the correct array sizes: 20 samples for 5 seconds and 40 samples for 10 seconds.
84-86: LGTM!Properly resetting the amperage history arrays on initialization ensures a clean state.
474-475: LGTM!Caching the UI element references follows the established pattern in this file and improves performance by avoiding repeated DOM queries.
Co-authored-by: Copilot <[email protected]>
|
|
Preview URL: https://pr4656.betaflight-app-preview.pages.dev |
1 participant
Add 5s and 10s average of ampere to help calibrate ampere sensor.
Summary by CodeRabbit