Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
PR Summary by QodoAdd CRSF altitude selection and accurate PR size baselines
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1. Malformed baselines evade retention
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if (telemetryConfig()->crsfGpsAltSource == CRSF_GPS_ALT_MSL) { | ||
| altitudeCm = gpsSol.llh.alt; |
There was a problem hiding this comment.
2. Synthetic altitude labeled raw 🐞 Bug ≡ Correctness
The MSL branch reads the post-processed gpsSol.llh.alt, which GPS-fix estimation overwrites with gpsOrigin.alt + baro.BaroAlt during an eligible GPS outage. Radios can therefore receive synthetic barometric altitude despite the setting being documented as raw GNSS MSL.
Agent Prompt
## Issue description
Ensure CRSF MSL telemetry uses a safely retained raw GNSS altitude rather than the processed GPS solution that fix estimation may overwrite.
## Issue Context
`gpsSolDRV` contains driver data but is explicitly unsafe to access asynchronously. Introduce or use a safe snapshot/accessor with clearly defined no-fix behavior rather than reading `gpsSolDRV` directly from telemetry code.
## Fix Focus Areas
- src/main/telemetry/crsf.c[242-251]
- src/main/io/gps.c[83-92]
- src/main/io/gps.c[264-327]
- src/main/io/gps.c[344-351]
- src/main/io/gps.h[124-160]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Addressed the automated review findings in 9eb30e9:
|
|
@sensei-hacker thanks for retargeting to maintenance-10.x. While rebasing I noticed that 10.x already covers this since #11168: with The only difference is that my setting decouples the GPS-frame altitude source from the baro/vario frame format, which I doubt anyone needs. Unless you see value in that, I'll close this as superseded. |
|
Reopened: keeping this open until there is a view on whether decoupling the GPS-frame altitude source from the baro/vario frame format (which #11168 ties together via |
| --jq '.[] | select(.tag_name | test("^size-baseline-[0-9a-f]{40}$")) | | ||
| [.created_at, .tag_name, | ||
| ((.body // "") | capture("(?m)^branch: (?<b>[A-Za-z0-9._/-]+)$") | .b // "?")] | @tsv' |
There was a problem hiding this comment.
1. Malformed baselines evade retention 🐞 Bug ☼ Reliability
list_per_commit_baselines applies .b // "?" only after capture(...), so notes without a matching first-line branch: marker never reach the intended fallback bucket and instead disappear from or fail the release-listing pipeline. Any per-commit release with missing or malformed branch notes can therefore stop prune before its deletion loop, while the caller only warns about the failure, preventing both the per-branch limit and global cap from processing old reports.
Agent Prompt
## Issue description
Make baseline listing tolerate per-commit releases whose notes lack a valid first-line `branch:` marker. Ensure every matching release is emitted under the documented `?` fallback bucket instead of disappearing from jq output or causing the listing pipeline to fail and abort pruning; use `try`, optional matching, or group the entire capture pipeline before applying the fallback.
## Issue Context
The pruning policy explicitly intends the `?` bucket and global cap to cover orphaned baselines and note-parse failures. Missing or malformed notes must therefore remain in the pruning input, and coverage should include such a per-commit baseline to verify that retention continues processing all releases; this is especially important because pruning failures are intentionally reduced to warnings after publishing.
## Fix Focus Areas
- .github/scripts/publish-size-baseline.sh[95-100]
- .github/scripts/publish-size-baseline.sh[102-142]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Pruning is housekeeping: a failure here must not fail the publish (the | ||
| # baseline itself already landed above), or the nightly would look broken | ||
| # for a cosmetic reason. Warn loudly instead. | ||
| prune || echo "::warning::per-commit baseline pruning failed (see stderr)" >&2 |
There was a problem hiding this comment.
2. Pruning api failures stay silent 🐞 Bug ◔ Observability
prune is invoked on the left side of ||, which disables Bash's errexit behavior for commands inside the function despite set -e, allowing a failed release-list pipeline to continue into successful empty-input processing. When GitHub release enumeration fails, no baselines are pruned and the promised warning is not emitted, so repeated retention failures can go unnoticed.
Agent Prompt
## Issue description
The outer `prune || warning` construct suppresses `errexit` within the function, allowing release-list failures to be masked by later successful commands. Explicitly check and return failure from the listing/sorting pipeline so the warning path reliably runs.
## Issue Context
Pruning is intentionally non-fatal, but its failures are supposed to produce a visible warning rather than silently skipping retention enforcement.
## Fix Focus Areas
- .github/scripts/publish-size-baseline.sh[102-114]
- .github/scripts/publish-size-baseline.sh[139-142]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 9eb30e9 |
…s_alt_source) On maintenance-10.x the altitude sent in the CRSF GPS frame is tied to the baro packet format: crsf_use_legacy_baro_packet = OFF sends the GNSS altitude above mean sea level, ON the estimated altitude above the arming point. This adds crsf_gps_alt_source to choose the GPS-frame altitude on its own: AUTO follow crsf_use_legacy_baro_packet (default, unchanged output) ESTIMATED estimated altitude above the arming point MSL GNSS altitude above mean sea level The conversion expression is the same as before, so AUTO is bit-identical to the current output. PG_TELEMETRY_CONFIG is bumped to 12 for the new field; docs/Settings.md regenerated.
9eb30e9 to
97d6d11
Compare
|
Rebased onto CI for the rebased commit on my fork (all targets, SITL on all platforms, unit tests, docs check): https://github.com/Raffi1202/inav/actions/runs/34359457345. The one red job there is the Parameter Group Version Check, which fails on every PR at the moment; #11885 fixes it. |
Those files belong to iNavFlight#11885, which replaces check-pg-versions.sh with a Python checker. Carrying a copy here only produces a conflict once either lands, and it is unrelated to this change.
Yes, CRSF telemetry defines the GPS packet as carrying GPS altitude- altitude above the ellipsoid, aka MSL. If you want MSL, the GPS packet is the place to get that. If you want AGL, the baro packet is the place to get that. |
2 participants
Problem
The altitude field of the CRSF GPS frame (0x02) is what EdgeTX/OpenTX radios show as
GAlt. Since #11168 that field followscrsf_use_legacy_baro_packet: with the settingOFF(default) it carries the GNSS altitude above mean sea level and the altitude above the arming point moves to the barometer/vario frame; withONthe legacy packet set returns and the GPS frame carries the estimated altitude again.Altitude source and packet format are therefore one decision. Someone who needs the legacy packet set - an older radio, an existing Lua script - cannot have MSL as
GAlt, and someone on the new packet set cannot keep the estimated altitude there. No issue asks for this; it follows the discussion in #10934, where the CLI option that became #11168 was requested.Cause
src/main/telemetry/crsf.c:244onmaintenance-10.x:One setting selects both the frame set (
crsf.c:695) and the altitude source.Change
A new setting
crsf_gps_alt_source = AUTO | ESTIMATED | MSLthat touches only the altitude field of the GPS frame.AUTO(default) followscrsf_use_legacy_baro_packet, so the conversion expression and the output stay bit-identical for anyone who sets nothing.ESTIMATEDalways sendsgetEstimatedActualPosition(Z),MSLalwaysgpsSol.llh.alt.PG_TELEMETRY_CONFIGis bumped 11 to 12 for the added struct member.Note that
MSLsendsgpsSol.llh.altas it stands, which GPS fix estimation overwrites with a baro-derived value during an eligible outage (src/main/io/gps.c:346). That matches the other fields of this frame, which come from the same solution.Test
Not run on hardware. Cause verified by reading
crsf.c:244andcrsf.c:695onmaintenance-10.x. Fork CI for the head commit, all targets plus the four SITL builds and unit tests, green: https://github.com/Raffi1202/inav/actions/runs/34374230553Flash / RAM
Not measured. The upstream firmware CI has not been released for this PR, and the fork build has no size baseline for this branch.
Docs
docs/Settings.mdregenerated fromsettings.yamlwithsrc/utils/update_cli_docs.py; the new entry describes what each of the three values sends and howAUTOrelates tocrsf_use_legacy_baro_packet.Open question for a maintainer
This only adds value if decoupling the two is wanted. #11168 already covers what #10934 asked for, and nobody has asked for the altitude source on its own - if the answer is that the coupling is fine, this can be closed as superseded.