Update Label 21 POSN — decode track/GS/wind#420
Update Label 21 POSN — decode track/GS/wind#420thepacket wants to merge 1 commit intoairframesio:masterfrom
Conversation
Extended to handle additional sub-formats and/or bug fixes.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
kevinelliott
left a comment
There was a problem hiding this comment.
Summary
Extends Label_21_POS to decode previously-unknown fields in the 9-field POSN format: ground track (field 1), groundspeed × 100 (field 4), and wind speed (field 5). Also rewrites processPosition to use a single regex that handles both spaced (N 34.398W 80.393) and unspaced (N 37.761W121.680) longitude forms, and captures the trailing decimal digit that the previous fixed-width substring dropped. Decode level bumped partial → full.
Verdict
Comment-only review. The intent is solid: the previous code had buggy hemisphere-letter logic (used && where || was needed, so the guard never triggered) and dropped a digit of precision. The new regex-based parser is cleaner and more correct. Cannot merge as-is — it breaks the existing test.
Must Fix
-
Existing test fails.
Label_21_POS.test.ts:28assertsdecodeLevel === 'partial'anditems.length === 6. With the new code,decodeLevelisfulland items count grows to 8 (added Track, GS, Wind). Also:items[0].valuewas'39.840 N, 75.790 W'(lossy — note the trailing0) — with the new regex it should now be'39.841 N, 75.790 W'(correct, full precision).remaining.text === ' 220,22051, 34'becomes''becauseunknownArris no longer called.
Please update the test. The existing assertion
'39.840 N, 75.790 W'is itself a fingerprint of the old precision bug — fixing it to'39.841 N, 75.790 W'should be called out in the diff so reviewers know it's intentional. -
Per-message coordinate change: the position string changes for any longitude or latitude where the dropped digit was non-zero. This is a small but real output change. Mention it in the PR body so consumers don't get surprised.
Should Fix
- The position regex
^([NS])\s*(\d+(?:\.\d+)?)\s*([EW])\s*(\d+(?:\.\d+)?)$is permissive — it will acceptN99999E99999etc. Tightening to bounded magnitudes (lat ≤ 90, lon ≤ 180) would prevent garbage decode of malformed messages. The existing code at least had a length guard. Currently the new code only requires the pattern to match — it never validates magnitudes. - Documentation says "raw integer; 24306 → 243.06 kt" but you
toFixed(2)the result. For25820 / 100 = 258.20that's fine; for an integer-divisible case like24300 / 100 = 243.00the output243.00 ktis slightly noisy. ConsiderNumber((gsKts).toFixed(2))orgsKts.toFixed(2).replace(/\.?0+$/, ''). Minor.
Nits
groundspeedformatter exists inResultFormatter(line 301 ofresult_formatter.ts) and emits${value} knotswith codeGSPD. You instead inline a custom row with codeGSand unitkt. Consider using the formatter for consistency with other plugins (Label 10/12/15 etc.) — though it would round to integer kt rather than two decimals. Worth at least a comment explaining why you diverged.- Same point for wind: there is no
windSpeedformatter today, so a custom row is fine, but be aware the existingwindDataformatter handles wind-with-direction in route reports. - Typo "aiports" → "airports" — already fixed in the diff. Good.
Backward Compat / Regression Risk
- Loosening
processPositionfrom 16-char fixed format to a regex broadens what's accepted. The OLD code had a buggy guard (used&&instead of||, so it always returned without effect — meaning all messages reached the substring extraction regardless). So functionally, the parser was already accepting anything; this PR just makes the parser correct. Net: lower regression risk than it looks. - The
&&/||guard fix means the function used to silently produce NaN for non-N/S leading messages. New code returns cleanly. Strict improvement. - Output changes: dropped
unknownArrof fields 1/4/5 → these now appear as labelled rows. Consumers readingremaining.textfor those values will lose them. Reasonable.
Test Coverage
Currently narrower (test breaks). Once updated:
- Add a fixture with longitude ≥ 100° (e.g.
N 37.761W121.680from your PR body) to lock the unspaced variant. - Add a southern-hemisphere or eastern-hemisphere fixture if you can find one — the docblock says they're hypothetical. If unobserved, fine to skip but a unit test of the regex with synthetic input would be cheap.
- Assert the new Track, GS, Wind rows by code/value.
Outstanding Existing Comments
None. Only an auto-generated CodeRabbit rate-limit notice on the PR.
Thanks @thepacket! Position-parser fix is clearly correct and the new decoded fields are valuable. Please update the test fixture and call out the coordinate-precision change in the description.
Extends
Label_21_POSto decode previously-unknown fields in the 9-field POSN format:24306= 243.06 ktFixes position parsing to handle both spaced (
N 34.398W 80.393) and unspaced (N 37.761W121.680) forms, and includes the final digit of each coordinate that the previous substring bounds dropped.Decode level bumped from
partialtofull.npm run buildpasses.