Added trajectory tilt angle computed field #875
Conversation
WalkthroughAdds a new computed field Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graph_config.js (1)
487-500: Critical syntax error: Missing closing braces for GPS_velned block.The previous
else ifblock forGPS_velned(lines 487-492) is missing its closing braces before the newgpsTrajectoryTiltAngleblock begins on line 493. This causes the parse errors reported by static analysis and the pipeline failure.Apply this diff to fix the syntax error:
} else if (fieldName.match(/^GPS_velned\[/)) { return { power: 1.0, MinMax: { min: -25, max: 25, + }, + }; } else if (fieldName == "gpsTrajectoryTiltAngle") { return { power: 1.0, MinMax: { min: -90, max: 90, }, };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/flightlog.js(7 hunks)src/flightlog_fields_presenter.js(2 hunks)src/graph_config.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T16:18:25.863Z
Learnt from: demvlad
Repo: betaflight/blackbox-log-viewer PR: 833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
Applied to files:
src/graph_config.js
🧬 Code graph analysis (1)
src/graph_config.js (2)
src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
🪛 Biome (2.1.2)
src/graph_config.js
[error] 493-493: expected , but instead found else
Remove else
(parse)
[error] 493-493: expected : but instead found if
Remove if
(parse)
[error] 493-493: expected , but instead found ==
Remove ==
(parse)
[error] 493-493: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
[error] 493-501: This method is later overwritten by an object member with the same name.
Overwritten with this method.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this method.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Actions: Deployment to Cloudflare Pages
src/graph_config.js
[error] 1-1: vite-plugin-pwa:build: [plugin vite:build-import-analysis] Failed to parse source for import analysis because the content contains invalid JS syntax. If you are using JSX, make sure to name the file with the .jsx or .tsx extension.
🔇 Additional comments (4)
src/flightlog_fields_presenter.js (1)
144-144: LGTM! Consistent field presentation.The friendly name and formatting for
gpsTrajectoryTiltAnglecorrectly follow the same pattern asgpsHomeAzimuth, displaying the angle with one decimal place and the degree symbol.Also applies to: 857-857
src/flightlog.js (3)
33-33: LGTM! Field count and naming correctly updated.The
ADDITIONAL_COMPUTED_FIELD_COUNTis appropriately incremented to 21, and the new field is added in the GPS section with a clear comment explaining its purpose.Also applies to: 289-289
640-644: LGTM! GPS velocity indices properly defined and validated.The
gpsVelNEDarray and its null check follow the same pattern as other GPS-related field indices likegpsCoord, ensuring the field is only processed when GPS velocity data is available.Also applies to: 714-716
758-765: Clarify unrelated quaternion normalization change.This quaternion normalization code appears unrelated to the trajectory tilt angle feature described in the PR. The previous code only handled the case where
m < 1.0, but this adds anelsebranch to normalize whenm >= 1.0.Was this change intentional? If it's a bug fix or improvement, it should ideally be in a separate PR with its own description and testing.
|
|
Preview URL: https://pr875.betaflight-blackbox.pages.dev |
|
@coderabbitai |
|
@demvlad I'll review the latest code improvements in your PR. Let me examine the changes you've made to the trajectory tilt angle implementation. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/flightlog.js (1)
911-926: Good improvements addressing previous review concerns.The implementation now includes:
- ✓ Bounds checking via clamping (line 920) to prevent
NaNfromasin- ✓ Documented threshold units (line 917: "5cm/s")
- ✓ Better numerical stability using
Math.hypotinstead of manual square-root calculation- ✓ Increased threshold from 1 to 5 cm/s (more practical)
The null/undefined handling for velocity components is implicit: if any component is null/undefined,
Math.hypotreturnsNaN, the conditionvelocity > minVeloevaluates tofalse, and the angle safely defaults to 0. While this works correctly, the previous review suggested explicit null checks for clarity.Optional: Explicit null handling for maintainability
If you prefer to make the null handling explicit (as suggested in the previous review), consider:
// Calculate trajectory tilt angle by NED GPS velocity if (gpsVelNED) { const Vn = srcFrame[gpsVelNED[0]], Ve = srcFrame[gpsVelNED[1]], Vd = srcFrame[gpsVelNED[2]]; + + if (Vn == null || Ve == null || Vd == null) { + destFrame[fieldIndex++] = 0; + } else { 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 { destFrame[fieldIndex++] = 0; }This makes the intent clearer for future maintainers, though the current implicit handling is functionally correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/flightlog.js(7 hunks)src/graph_config.js(63 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/graph_config.js
🔇 Additional comments (5)
src/flightlog.js (5)
33-33: LGTM!The field count increment is correct for the new
gpsTrajectoryTiltAnglefield, and the comment accurately documents all computed fields.
288-289: LGTM!The field name addition follows the established naming convention and is correctly positioned with GPS-related fields.
640-644: LGTM!The
gpsVelNEDinitialization follows the established pattern for field index arrays and correctly references the GPS NED velocity components.
714-716: LGTM!The availability check for
gpsVelNEDfollows the same defensive pattern used for other optional field arrays.
758-765: LGTM!The formatting change improves consistency and readability by using explicit braces for the else block.
2 participants
Added trajectory tilt angle computed field. It is computed by using GPS NED velocities.
Now we have both trajectories angle: horizontal and vertical.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.