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.
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.