Using actual log fields state instead of disabled flags for computed fields #880
Conversation
WalkthroughHeading and GPS-derived field injection now depend on actual source-data presence (e.g., Changes
Sequence Diagram(s)(omitted — changes are localized gating logic within a single module and do not introduce multi-component sequential flows) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @src/flightlog.js:
- Around line 292-300: The GPS computed-field checks for
fieldNameToIndex["GPS_coord[0]"] and ["GPS_velned[0]"] run before
fieldNameToIndex is populated, so gpsCartesianCoords, gpsDistance,
gpsHomeAzimuth and gpsTrajectoryTiltAngle never get pushed to fieldNames; fix by
constructing/populating fieldNameToIndex from the source field list (same
approach used for heading fields) before these two if checks in src/flightlog.js
so the lookups work and the GPS-derived fields are added to fieldNames.
- Around line 915-927: The GPS trajectory tilt angle is only written when
gpsVelNED exists for the whole log and when per-frame velocity is present,
causing inconsistent frames; modify the block that references gpsVelNED,
srcFrame, destFrame and fieldIndex so the code always writes a value for the
trajectory tilt angle field for every frame: compute trajectoryTiltAngle as you
do when Vn/Ve/Vd exist and velocity > minVelo, but if any of gpsVelNED indices
or the per-frame velocity components are null/undefined or velocity <= minVelo,
set trajectoryTiltAngle to null (or the chosen missing-value sentinel) and then
always assign destFrame[fieldIndex++] = trajectoryTiltAngle to preserve
consistent frame structure.
- Around line 900-912: The condition that gates GPS processing mistakenly uses
truthiness on srcFrame[gpsCoord[0]] so a value of 0 is treated as missing;
update the check in the block that references gpsTransform, gpsCoord and
srcFrame to explicitly test for null/undefined (e.g., srcFrame[gpsCoord[0]] !=
null and srcFrame[gpsCoord[1]] != null and srcFrame[gpsCoord[2]] != null) so
zero coordinates are accepted, leaving the rest of the logic that computes
gpsCartesianCoords, destFrame assignments, distance and homeAzimuth unchanged
(ensure fieldIndex handling remains consistent).
- Around line 900-912: The code only writes GPS-derived fields into destFrame
when gpsTransform && gpsCoord && srcFrame[gpsCoord[0]] is truthy, causing frames
to have inconsistent lengths; update the block around the gpsTransform/gpsCoord
check so that for every frame you always advance fieldIndex and write the same
sequence of GPS fields (x,y,z,groundDistance,homeAzimuth) into destFrame even
when srcFrame[gpsCoord[0]] is missing by computing gpsCartesianCoords when
available and otherwise assigning null (or 0) placeholders for
gpsCartesianCoords.x/y/z, the ground distance, and homeAzimuth; keep the same
variable names (gpsTransform, gpsCoord, srcFrame, destFrame, fieldIndex,
gpsCartesianCoords, homeAzimuth) so splice(fieldIndex) later produces consistent
frame lengths across all frames.
- Around line 270-273: The check for computed heading fields runs before
fieldNameToIndex is populated, so it always fails; move or invoke the code that
builds/populates fieldNameToIndex from the parsed fields array before the
conditional that tests fieldNameToIndex["imuQuaternion[0]"], ["gyroADC[0]"], and
["accSmooth[0]"], and then perform the check using explicit existence tests
(e.g., hasOwnProperty or the in operator) with correct grouping:
(imuQuaternion[0] exists) || (gyroADC[0] exists && accSmooth[0] exists); if
true, push "heading[0]", "heading[1]", "heading[2]" into fieldNames.
🧹 Nitpick comments (2)
src/flightlog.js (2)
270-271: Consider adding parentheses for clarity.The condition mixes
||and&&operators. While JavaScript precedence evaluates this correctly asfieldNameToIndex["imuQuaternion[0]"] || (fieldNameToIndex["gyroADC[0]"] && fieldNameToIndex["accSmooth[0]"]), adding explicit parentheses would improve readability.🔎 Suggested improvement
- if (fieldNameToIndex["imuQuaternion[0]"] || - fieldNameToIndex["gyroADC[0]"] && fieldNameToIndex["accSmooth[0]"]) { + if (fieldNameToIndex["imuQuaternion[0]"] || + (fieldNameToIndex["gyroADC[0]"] && fieldNameToIndex["accSmooth[0]"])) { fieldNames.push("heading[0]", "heading[1]", "heading[2]"); }
275-290: Inconsistency: Mixed use of field presence checks and disabled flags.Lines 275, 279, and 288 still check
!that.isFieldDisabled().PID,!that.isFieldDisabled().SETPOINT, etc., while the newly changed heading and GPS field logic (lines 270-273, 292-300) checks for actual field presence. This creates an inconsistent pattern where some computed fields use the new approach and others use the old approach.For consistency with the PR objective ("Using actual log fields state instead of disabled flags"), consider updating these remaining checks to also verify actual field presence instead of disabled flags.
Example approach
// For PID sum fields if (fieldNameToIndex["axisP[0]"] && fieldNameToIndex["axisI[0]"]) { fieldNames.push("axisSum[0]", "axisSum[1]", "axisSum[2]"); } // For rcCommands fields if (fieldNameToIndex["rcCommand[0]"] || fieldNameToIndex["setpoint[0]"]) { fieldNames.push("rcCommands[0]", "rcCommands[1]", "rcCommands[2]", "rcCommands[3]"); } // For axisError fields if (fieldNameToIndex["gyroADC[0]"] && (fieldNameToIndex["rcCommand[0]"] || fieldNameToIndex["setpoint[0]"])) { fieldNames.push("axisError[0]", "axisError[1]", "axisError[2]"); }Note: This would require building
fieldNameToIndexbefore these checks, as noted in the previous critical issue.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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.
🧬 Code graph analysis (1)
src/flightlog.js (1)
src/flightlog_parser.js (1)
that(163-480)
demvlad
force-pushed
the
computed_fields_define_issue
branch
from
7efd42b to
4e3abd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/flightlog.js:
- Around line 902-914: The code currently assumes per-frame GPS values exist
when gpsTransform and gpsCoord are present; instead, check the actual frame
values using non-strict null checks (e.g., srcFrame[gpsCoord[0]] != null) for
the latitude/longitude/alt indices before computing coordinates; if any of the
three are null/undefined write null placeholders into destFrame for the three
Cartesian components plus the distance and azimuth (preserving field order and
incrementing fieldIndex accordingly), otherwise perform the existing WGS_BS
transform, distance (Math.sqrt) and azimuth (Math.atan2) calculations exactly as
before; reference variables: gpsTransform, gpsCoord, srcFrame, destFrame,
fieldIndex, gpsCartesianCoords.
♻️ Duplicate comments (1)
src/flightlog.js (1)
916-929: Potential issue: Same missing per-frame GPS velocity data not handled.Similar to the GPS coordinates issue above, this code only checks
gpsVelNED(whether the field exists in the schema) without verifying that velocity data is present in the current frame. IfGPS_velned[0]exists in the log but individual frames havenullorundefinedvelocity values, the computation will use invalid data.Impact: May produce
NaNvalues or errors during frames with missing GPS velocity data.🔎 Suggested fix: Add explicit null checks for velocity data
// Calculate trajectory tilt angle by NED GPS velocity if (gpsVelNED) { + if (srcFrame[gpsVelNED[0]] != null && srcFrame[gpsVelNED[1]] != null && srcFrame[gpsVelNED[2]] != null) { const Vn = srcFrame[gpsVelNED[0]], Ve = srcFrame[gpsVelNED[1]], Vd = srcFrame[gpsVelNED[2]]; const velocity = Math.hypot(Vn, Ve, Vd); const minVelo = 5; // 5cm/s limit to prevent division by zero and miss tiny noise values let trajectoryTiltAngle = 0; if (velocity > minVelo) { const angleSin = Math.max(-1, Math.min(1, Vd / velocity)); trajectoryTiltAngle = -Math.asin(angleSin) * 180 / Math.PI; // [degree], if velo is up then >0 } destFrame[fieldIndex++] = trajectoryTiltAngle; + } else { + // Write null to maintain consistent frame structure + destFrame[fieldIndex++] = null; + } }This ensures consistent frame structure across all frames when GPS velocity fields are defined.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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.
📚 Learning: 2025-08-14T02:50:17.745Z
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.
Applied to files:
src/flightlog.js
🔇 Additional comments (2)
src/flightlog.js (2)
270-273: Excellent fix: Field presence now checked correctly.The change from
fieldNameToIndexlookups tofieldNames.includes()correctly addresses the critical bug identified in previous reviews. SincefieldNamesis populated fromparser.frameDefsbefore this check (lines 250-266), the field presence test now works as intended.
292-302: Well-structured GPS field injection.The nested conditionals properly check for GPS data availability:
- Outer check for GPS disabled flag reduces unnecessary work
- Inner checks use
fieldNames.includes()to verify source data presence- Separate checks for coordinate-based vs velocity-based fields align with data dependencies
This correctly addresses the critical bug from previous reviews where
fieldNameToIndexwas checked before being populated.
Co-authored-by: Mark Haslinghuis <[email protected]>
demvlad
dismissed stale reviews from haslinghuis and coderabbitai[bot]
via
ee762de
|
|
Preview URL: https://pr880.betaflight-blackbox.pages.dev |
|
Checked on log, what had similar issue first |
3 participants
Using actual log fields state instead of disabled flags for computed fields definition.
It works for this @ctzsnooze olde version log
HasProblem.txt
But it needs more checking for another log files.
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.