-
-
Notifications
You must be signed in to change notification settings - Fork 156
Replace gyro_to_use with gyro_enabled_bitmask in header dialog #862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace gyro_to_use with gyro_enabled_bitmask in header dialog #862
Conversation
WalkthroughAdds a new gyro_enabled_bitmask sysConfig field parsed from log headers, renders it in the header dialog via a new bitmask helper, and exposes a new UI cell in the HARDWARE OPTIONS row (inserted before the existing gyro_hardware_lpf). Also includes parser numeric/coercion refactors and firmware-version conditional adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant LH as Log Header
participant FLP as flightlog_parser.js
participant SC as sysConfig
participant HD as header_dialog.js
participant UI as index.html (DOM)
LH->>FLP: header["gyro_enabled_bitmask"] = "42"
FLP->>SC: sysConfig.gyro_enabled_bitmask = parseInt("42")
note right of SC #E8F5E9: Stored as numeric bitmask
HD->>SC: read sysConfig.gyro_enabled_bitmask
HD->>HD: setBitmaskParameter("gyro_enabled_bitmask", 42, 8)
HD->>UI: populate `td[name="gyro_enabled_bitmask"]` with "42 (0b00101010)"
UI-->>User: displays "Gyro enable mask" before "Hardware gyro LPF"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
index.html
(1 hunks)src/flightlog_parser.js
(2 hunks)src/header_dialog.js
(3 hunks)
🔇 Additional comments (1)
index.html (1)
1519-1522
: Placement and structure LGTM.The new
gyro_enabled_bitmask
field slots neatly into the hardware options row and matches the surrounding markup, so I’ve got no concerns here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Preview URL: https://pr862.betaflight-blackbox.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
index.html
(3 hunks)src/flightlog_parser.js
(10 hunks)src/header_dialog.js
(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- index.html
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T23:23:01.244Z
Learnt from: haslinghuis
PR: betaflight/blackbox-log-viewer#859
File: src/flightlog_fields_presenter.js:1315-1325
Timestamp: 2025-09-24T23:23:01.244Z
Learning: In the blackbox-log-viewer codebase, firmware version pre-release identifiers (like "-rc1", "-rc2") are stripped from the `firmwareVersion` property, so Betaflight 4.5.0-RC1 would be exposed as just "4.5.0" for version comparison purposes.
Applied to files:
src/flightlog_parser.js
🪛 Biome (2.1.2)
src/flightlog_parser.js
[error] 845-846: This case is falling through to the next case.
Add a break
or return
statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
🔇 Additional comments (15)
src/flightlog_parser.js (9)
302-302
: LGTM!The addition of
gyro_enabled_bitmask
is properly initialized and follows the established pattern for configuration fields.
536-536
: LGTM!Simplifying the undefined check is appropriate here and more idiomatic.
747-747
: LGTM!The switch case properly handles the new
gyro_enabled_bitmask
header field using the same integer parsing pattern as similar fields.
826-826
: LGTM!The division simplifications (removing
.0
suffix) are appropriate in JavaScript, where all numbers are floating-point by default.Also applies to: 840-840, 846-846
987-987
: LGTM!The fallback assignment for unknown firmware versions is appropriate.
1088-1089
: LGTM!Using
Math.max
andMath.min
is more idiomatic than manual comparisons for tracking field statistics.
1447-1447
: LGTM!Replacing bitwise
| 0
coercion withMath.trunc
improves readability and makes the intent explicit.Also applies to: 1456-1456
1676-1676
: LGTM!Division simplifications are consistent with JavaScript's floating-point number handling.
Also applies to: 1681-1682
844-847
: Intentional case fallthrough is correct here.The static analysis tool flags the fallthrough from
motor_idle
todigitalIdleOffset
, but this is intentional as both parameters share identical parsing logic (integer value divided by 100). This is a common and acceptable pattern for consolidating duplicate case logic.src/header_dialog.js (6)
876-880
: LGTM!Minor formatting adjustment with no functional impact.
882-908
: LGTM!The
setBitmaskParameter
helper properly handles bitmask display and storage. The implementation correctly:
- Clears all state when data is null (preventing stale data issues)
- Stores the raw integer value for round-trip saving
- Makes the field readonly to prevent editing of the formatted display
- Provides user-friendly binary representation with tooltip
539-549
: Verify the firmware version ranges for the gyro parameter transition.The PR deprecates
gyro_to_use
(max: "4.5.9") and introducesgyro_enabled_bitmask
(min: "2025.12.0"). Please confirm:
- Is there a firmware version gap between 4.5.9 and 2025.12.0 where neither parameter would display?
- Should
gyro_to_use
have a max closer to "2025.11.9" to ensure continuity?Based on learnings: firmware pre-release identifiers are stripped, but the large version jump suggests a possible versioning scheme change that should be verified.
Based on learnings.
1974-1974
: LGTM!The
setBitmaskParameter
call is properly integrated into the rendering flow alongside the relatedgyro_to_use
parameter.
2035-2038
: LGTM!The raw-value data attribute handling correctly enables round-trip saving of bitmask parameters, preventing the formatted display string from being coerced to 0/1. The implementation properly handles both array and scalar parameters.
Also applies to: 2046-2048
1750-1750
: LGTM!The DOM manipulation change from
insertBefore()
tobefore()
is appropriate—both achieve the same result, withbefore()
being the more modern jQuery syntax.
For 2025.12.0
gyro_to_use
gyro_enabled_bitmask
Summary by CodeRabbit
New Features
UI