Skip to content

Conversation

@demvlad
Copy link
Contributor

@demvlad demvlad commented Dec 23, 2025

The current BBExplorer has bug in BF 4.5.1 and older logs.
The ATTITUDE disabledFieldsFlags is undefined, and !that.isFieldDisabled().ATTITUDE value is true. As result, the computed fields have wrong values.
Bug fix: define ATTITUDE and SERVO disabledFieldsFlags as true for older BF firmware version.

Summary by CodeRabbit

  • New Features
    • Added ATTITUDE and SERVO fields in flight logs, giving access to more telemetry for deeper analysis and diagnostics.
    • Improved compatibility with BetaFlight 2025.12.0+ to ensure SERVO is correctly recognized and mapped.
    • Default handling added to avoid undefined behavior on older firmware, preserving stable playback and analysis.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

Walkthrough

Adds ATTITUDE and SERVO to FlightLog.prototype.isFieldDisabled (defaulting to disabled) and extends a BetaFlight firmware-version (>= 2025.12.0) override to remap SERVO (bit 15) while preserving existing field mappings.

Changes

Cohort / File(s) Summary
Field mapping update
src/flightlog.js
Added ATTITUDE and SERVO to the public isFieldDisabled map (initialized true); extended BetaFlight firmware override (>= 2025.12.0) to include SERVO mapping (bit 15) and preserve prior mappings for ATTITUDE, ACC, DEBUG, MOTORS, GPS, RPM, GYROUNFILT.

Sequence Diagram(s)

(Skipped — changes are a small mapping update and do not introduce multi-component sequential control flow.)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • PR #874 — Also modifies ATTITUDE handling in src/flightlog.js (field inclusion and attitude computation guards).
  • PR #858 — Modifies isFieldDisabled logic in src/flightlog.js with firmware-version gating and disabled-field mapping changes.

Suggested labels

RN: BUGFIX

Suggested reviewers

  • haslinghuis
  • nerdCopter

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main bug fix: resolving undefined ATTITUDE fields disabled flag for older BF firmware versions, which matches the changeset.
Description check ✅ Passed The PR description clearly explains the bug, its impact, and the proposed fix, but does not follow the repository's PR template structure and lacks issue reference.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b3d2c8 and 836566c.

📒 Files selected for processing (1)
  • src/flightlog.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ctzsnooze
Repo: betaflight/blackbox-log-viewer PR: 851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
Learnt from: nerdCopter
Repo: betaflight/blackbox-log-viewer PR: 822
File: .github/workflows/deploy-preview.yml:17-19
Timestamp: 2025-05-14T16:08:27.254Z
Learning: For betaflight/blackbox-log-viewer repository, the team is aware of the security implications of using `pull_request_target` without specifying `persist-credentials: false` in the checkout action, and has chosen to proceed with the current approach. This concern should not be flagged in future reviews.
Learnt from: haslinghuis
Repo: betaflight/blackbox-log-viewer PR: 862
File: src/header_dialog.js:1400-1403
Timestamp: 2025-09-29T19:52:17.402Z
Learning: In the Betaflight blackbox-log-viewer codebase, the sysConfig.firmware property can be either a string or number. When comparing firmware versions for PID controller type selection, only integral parts are checked (e.g., >= 3, >= 2), so JavaScript's type coercion handles string-to-number comparisons correctly. The maintainer has confirmed this mixed-type approach is acceptable.
Learnt from: haslinghuis
Repo: betaflight/blackbox-log-viewer PR: 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.
📚 Learning: 2025-09-29T19:52:17.402Z
Learnt from: haslinghuis
Repo: betaflight/blackbox-log-viewer PR: 862
File: src/header_dialog.js:1400-1403
Timestamp: 2025-09-29T19:52:17.402Z
Learning: In the Betaflight blackbox-log-viewer codebase, the sysConfig.firmware property can be either a string or number. When comparing firmware versions for PID controller type selection, only integral parts are checked (e.g., >= 3, >= 2), so JavaScript's type coercion handles string-to-number comparisons correctly. The maintainer has confirmed this mixed-type approach is acceptable.

Applied to files:

  • src/flightlog.js
🔇 Additional comments (2)
src/flightlog.js (2)

1808-1811: Excellent fix for the undefined ATTITUDE field bug!

This correctly addresses the issue where !that.isFieldDisabled().ATTITUDE would evaluate to !undefined = true on older firmware versions (< 2025.12.0), causing incorrect computed field values. By defaulting both ATTITUDE and SERVO to true (disabled), the code now properly handles older logs where these fields weren't available.

The impact on line 270's heading field logic is now correct:

  • Old firmware: heading fields only added when both GYRO and ACC are enabled
  • New firmware: heading fields added when ATTITUDE is enabled OR both GYRO and ACC are enabled

1813-1825: No changes needed. The firmware version "2025.12.0" is correct and aligns with Betaflight's official calendar-based versioning scheme (CalVer, format YYYY.M.PATCH) adopted in 2025. The 2025.12 release includes expanded IMU/attitude handling and Blackbox logging support for ATTITUDE quaternion data, making the bit remapping logic appropriate for this version.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/flightlog.js (1)

1808-1810: Good fix for preventing undefined field access.

Setting ATTITUDE and SERVO to true (disabled) by default is the correct approach to prevent undefined behavior in older firmware versions where these fields don't exist. This ensures expressions like !that.isFieldDisabled().ATTITUDE evaluate correctly as false for older firmware.

Minor comment improvement

The comment could be slightly more precise:

-    // define new version fields as disabled, to prevent using of undefined values
+    // Define newer firmware fields as disabled by default to prevent undefined behavior in older firmware
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57fe3ff and 5b3d2c8.

📒 Files selected for processing (1)
  • src/flightlog.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ctzsnooze
Repo: betaflight/blackbox-log-viewer PR: 851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
Learnt from: haslinghuis
Repo: betaflight/blackbox-log-viewer PR: 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.
Learnt from: haslinghuis
Repo: betaflight/blackbox-log-viewer PR: 862
File: src/header_dialog.js:1400-1403
Timestamp: 2025-09-29T19:52:17.402Z
Learning: In the Betaflight blackbox-log-viewer codebase, the sysConfig.firmware property can be either a string or number. When comparing firmware versions for PID controller type selection, only integral parts are checked (e.g., >= 3, >= 2), so JavaScript's type coercion handles string-to-number comparisons correctly. The maintainer has confirmed this mixed-type approach is acceptable.
🔇 Additional comments (1)
src/flightlog.js (1)

1813-1825: Bit remapping logic is correct.

The code properly reassigns disabled field flags for Betaflight 2025.12.0+: ATTITUDE takes bit 8, with ACC through SERVO shifted to bits 9–15. This version is used consistently across the codebase for feature gating, and the logic is sound.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 23, 2025
@demvlad
Copy link
Contributor Author

demvlad commented Dec 23, 2025

log4.5.txt
The BF 4.5.1 log file.
The setpoint fields are wrong without this bug fix - the throttle setpoint is very small.

@sonarqubecloud
Copy link

@github-actions
Copy link

Preview URL: https://pr876.betaflight-blackbox.pages.dev

@haslinghuis haslinghuis merged commit 28dc780 into betaflight:master Dec 23, 2025
5 checks passed
@demvlad demvlad deleted the olde_ver_bug branch December 24, 2025 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants