Skip to content

Update Label 21 POSN — decode track/GS/wind#420

Open
thepacket wants to merge 1 commit intoairframesio:masterfrom
thepacket:plugin/label-21-update
Open

Update Label 21 POSN — decode track/GS/wind#420
thepacket wants to merge 1 commit intoairframesio:masterfrom
thepacket:plugin/label-21-update

Conversation

@thepacket
Copy link
Copy Markdown

Extends Label_21_POS to decode previously-unknown fields in the 9-field POSN format:

  • Field 1 → ground track (degrees)
  • Field 4 → ground speed ÷ 100 (kt) — raw 24306 = 243.06 kt
  • Field 5 → wind speed (kt)

Fixes 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 partial to full.

npm run build passes.

Extended to handle additional sub-formats and/or bug fixes.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

Warning

Rate limit exceeded

@thepacket has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 55 minutes and 9 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1f70f123-56da-4fb2-8211-5ec324cea295

📥 Commits

Reviewing files that changed from the base of the PR and between 10f9f69 and 50a047d.

📒 Files selected for processing (1)
  • lib/plugins/Label_21_POS.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@kevinelliott kevinelliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Existing test fails. Label_21_POS.test.ts:28 asserts decodeLevel === 'partial' and items.length === 6. With the new code, decodeLevel is full and items count grows to 8 (added Track, GS, Wind). Also:

    • items[0].value was '39.840 N, 75.790 W' (lossy — note the trailing 0) — with the new regex it should now be '39.841 N, 75.790 W' (correct, full precision).
    • remaining.text === ' 220,22051, 34' becomes '' because unknownArr is 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.

  2. 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 accept N99999E99999 etc. 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. For 25820 / 100 = 258.20 that's fine; for an integer-divisible case like 24300 / 100 = 243.00 the output 243.00 kt is slightly noisy. Consider Number((gsKts).toFixed(2)) or gsKts.toFixed(2).replace(/\.?0+$/, ''). Minor.

Nits

  • groundspeed formatter exists in ResultFormatter (line 301 of result_formatter.ts) and emits ${value} knots with code GSPD. You instead inline a custom row with code GS and unit kt. 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 windSpeed formatter today, so a custom row is fine, but be aware the existing windData formatter handles wind-with-direction in route reports.
  • Typo "aiports" → "airports" — already fixed in the diff. Good.

Backward Compat / Regression Risk

  • Loosening processPosition from 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 unknownArr of fields 1/4/5 → these now appear as labelled rows. Consumers reading remaining.text for 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.680 from 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants