Update Label SQ — accept 'B' separator, fix DDMM coordinate encoding#423
Update Label SQ — accept 'B' separator, fix DDMM coordinate encoding#423thepacket 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 5 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 |
There was a problem hiding this comment.
Pull request overview
Updates the Label_SQ decoder to better match real-world ground-station squitter payloads by broadening separator parsing and fixing latitude/longitude decoding to proper DDMM/DDDMM decimal-minutes.
Changes:
- Accepts any uppercase separator flag (e.g.,
B), exposing it asSEPin formatted output. - Corrects coordinate decoding from naive
/100to DDMM/DDDMM → decimal degrees. - Tightens regex field widths and adds richer formatted output (message type, category, provider suffix).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Lat is 4 digits (DDMM), lng is 5 digits (DDDMM). Separator before | ||
| // frequency is typically 'V' but 'B' (and possibly others) have been | ||
| // observed; capture as a raw flag rather than constraining to 'V'. | ||
| const regex = | ||
| /0(\d)X(?<org>\w)(?<iata>\w\w\w)(?<icao>\w\w\w\w)(?<station>\d)(?<lat>\d+)(?<latd>[NS])(?<lng>\d+)(?<lngd>[EW])V(?<vfreq>\d+)\/.*/; | ||
| /0(\d)(?<cat>[A-Z])(?<org>[A-Z])(?<iata>\w{3})(?<icao>\w{4})(?<station>\d)(?<lat>\d{4})(?<latd>[NS])(?<lng>\d{5})(?<lngd>[EW])(?<sep>[A-Z])(?<vfreq>\d+)(?:\/(?<suffix>\w*))?/; | ||
| const result = message.text.match(regex); | ||
|
|
||
| if (result?.groups && result.length >= 8) { | ||
| decodeResult.raw.category = result.groups.cat; | ||
| decodeResult.raw.network = result.groups.org; | ||
|
|
||
| // DDMM → decimal degrees (deg + min/60), signed by hemisphere | ||
| const latRaw = result.groups.lat; | ||
| const lngRaw = result.groups.lng; | ||
| const latDeg = Math.floor(Number(latRaw) / 100); | ||
| const latMin = Number(latRaw) % 100; | ||
| const lngDeg = Math.floor(Number(lngRaw) / 100); | ||
| const lngMin = Number(lngRaw) % 100; | ||
| const latitude = | ||
| (latDeg + latMin / 60) * (result.groups.latd === 'S' ? -1 : 1); | ||
| const longitude = | ||
| (lngDeg + lngMin / 60) * (result.groups.lngd === 'W' ? -1 : 1); |
There was a problem hiding this comment.
The SQ parsing behavior changed (DDMM/DDDMM decoding, 5-digit longitude, and non-'V' separators), but the existing unit tests for this plugin will no longer match these rules (e.g., Label_SQ.test.ts currently uses a 4-digit longitude and expects lat/lng to be parsed by raw/100). Please update/add tests to cover the corrected DDMM/DDDMM conversion and at least one non-V separator (e.g., B), so CI doesn’t regress this again.
There was a problem hiding this comment.
Maintainer reviewer agree — this is the merge blocker. I ran the test suite locally against the PR head: Label_SQ.test.ts fails (expect(res.raw.groundStation).toBeDefined() is undefined) because the existing JFK fixture (4075 lat, 7398 lng) is no longer valid under the corrected DDMM model AND the longitude regex tightened from \\d+ to \\d{5}. Please rewrite the JFK fixture with realistic DDMM/DDDMM (e.g. 4038 for 40°38′N, 07346 for 73°46′W) and assert with toBeCloseTo. Also add the Gatwick example from your PR body (02XALGWEGKK15109N00012WB136975/ARINC) — that exercises both the B separator and DDDMM with leading zeros.
| // observed; capture as a raw flag rather than constraining to 'V'. | ||
| const regex = | ||
| /0(\d)X(?<org>\w)(?<iata>\w\w\w)(?<icao>\w\w\w\w)(?<station>\d)(?<lat>\d+)(?<latd>[NS])(?<lng>\d+)(?<lngd>[EW])V(?<vfreq>\d+)\/.*/; | ||
| /0(\d)(?<cat>[A-Z])(?<org>[A-Z])(?<iata>\w{3})(?<icao>\w{4})(?<station>\d)(?<lat>\d{4})(?<latd>[NS])(?<lng>\d{5})(?<lngd>[EW])(?<sep>[A-Z])(?<vfreq>\d+)(?:\/(?<suffix>\w*))?/; |
There was a problem hiding this comment.
The regex uses \w{3} / \w{4} for IATA/ICAO, which also matches digits and underscores. Since these fields are documented here as airport codes and other captured fields are constrained to [A-Z], consider tightening these to [A-Z]{3} and [A-Z]{4} to avoid accepting invalid station identifiers.
| /0(\d)(?<cat>[A-Z])(?<org>[A-Z])(?<iata>\w{3})(?<icao>\w{4})(?<station>\d)(?<lat>\d{4})(?<latd>[NS])(?<lng>\d{5})(?<lngd>[EW])(?<sep>[A-Z])(?<vfreq>\d+)(?:\/(?<suffix>\w*))?/; | |
| /0(\d)(?<cat>[A-Z])(?<org>[A-Z])(?<iata>[A-Z]{3})(?<icao>[A-Z]{4})(?<station>\d)(?<lat>\d{4})(?<latd>[NS])(?<lng>\d{5})(?<lngd>[EW])(?<sep>[A-Z])(?<vfreq>\d+)(?:\/(?<suffix>\w*))?/; |
There was a problem hiding this comment.
Maintainer reviewer agree — Should Fix. Tightening \\w to [A-Z] for IATA/ICAO is the right call: \\w accepts [A-Za-z0-9_], none of which (except A-Z) are valid in airport codes. The Copilot suggested edit is good as written — please apply.
| if (result?.groups && result.length >= 8) { | ||
| decodeResult.raw.category = result.groups.cat; |
There was a problem hiding this comment.
result.length >= 8 is tied to the number of capture groups in the regex and doesn’t provide a stable correctness check (it will change if the regex is refactored). Since you already guard on result?.groups, consider removing the length check (or replacing it with explicit group presence checks if needed).
There was a problem hiding this comment.
Maintainer reviewer agree — Should Fix. result?.groups (truthiness) is the correct guard; result.length >= 8 is brittle and tied to capture-group count. With named groups the safer pattern is to test the specific result.groups.lat etc. you intend to use. Drop the length check.
| var formattedNetwork = 'Unknown'; | ||
| if (decodeResult.raw.network == 'A') { | ||
| if (decodeResult.raw.network === 'A') { | ||
| formattedNetwork = 'ARINC'; | ||
| } else if (decodeResult.raw.network == 'S') { | ||
| } else if (decodeResult.raw.network === 'S') { | ||
| formattedNetwork = 'SITA'; | ||
| } | ||
| var formattedCategory = String(decodeResult.raw.category || ''); | ||
| if (formattedCategory === 'X') { |
There was a problem hiding this comment.
This file is the only one in lib/plugins using var (most plugins use const/let, e.g., lib/plugins/Label_20_POS.ts:18-24). Consider switching formattedNetwork/formattedCategory to let (or const where possible) to match project conventions and avoid function-scoped variables.
There was a problem hiding this comment.
Maintainer reviewer agree — Should Fix. The var was inherited from pre-existing code, but project convention is let/const. Since you're already touching this block, switching both formattedNetwork and formattedCategory to let is a clean opportunistic improvement.
kevinelliott
left a comment
kevinelliott
left a comment
There was a problem hiding this comment.
Summary
Two real bug fixes to Label_SQ:
- The separator before the VDL frequency is no longer constrained to
V— observedB(and the regex now accepts any uppercase letter). Exposed asSEP. - Coordinate decoding fixed from naive
raw / 100(which wrongly conflates DDMM with hundredths) to proper DDMM/DDDMM decimal-minutes. Cross-checked against EGKK Gatwick (51.15, -0.20) and KFTY (33.766667, -84.516667). Lat regex tightened to\d{4}, lng to\d{5}.
Also adds Category, Provider Suffix, and Message Type rows.
Verdict
Comment-only review. The DDMM coordinate fix is unambiguously correct — the old /100 math was wrong and produced bogus coordinates (Gatwick rendered as 33.46 instead of 51.15 per the PR body). The separator broadening is real-world-justified. Cannot merge as-is — the existing test fails because the test fixture was constructed against the old (buggy) coordinate model.
Must Fix
-
Existing test fails.
Label_SQ.test.ts:47— the test feeds4075and7398(4-digit lat AND 4-digit lon), then assertslatitude.toBeCloseTo(40.75). With the new DDMM rule,4075would mean 40°75′, which is invalid; the test fixture is no longer consistent with the corrected coordinate model. Even if it were valid DDMM (e.g. 4045 → 40°45′ = 40.75), the longitude width changed from\d+to\d{5}, so the existing 4-digit7398no longer matches the regex at all →groundStationisundefined→ tests assertionexpect(res.raw.groundStation).toBeDefined()fails.Please rewrite the JFK fixture using realistic DDMM/DDDMM input:
4038(40°38′ = 40.6333°N) for lat,07346(73°46′W = -73.7666°W) for lng — those are JFK's true coords. Then assert withtoBeCloseTo. Also add the Gatwick example from your PR body — it's the canonical real-world non-Vseparator case. -
Add a
Bseparator test. The whole point of the PR's first fix is unobserved coverage of theBseparator. Without a fixture usingB, that path stays unvalidated. (Copilot raised this — I agree.)
Should Fix
\w{3}/\w{4}for IATA/ICAO is too loose —\wincludes digits and underscores. Tighten to[A-Z]{3}and[A-Z]{4}per Copilot's suggested edit. ICAO codes are uppercase letters; this is a reasonable correctness/strictness tightening. (Copilot raised this — I agree.)result.length >= 8check — agreed with Copilot, this depends on capture-group count and is fragile. Theresult?.groupstruthiness check is sufficient. Drop the length check.var formattedNetwork,var formattedCategory— every other plugin inlib/pluginsuseslet/const. The pre-existingvarwas inherited; fixing it here is a clean opportunistic improvement. (Copilot raised this — I agree.)
Nits
Math.round(latitude * 1e6) / 1e6to clamp to 6 decimals — fine, but slightly noisy in the value. Consider justNumber(latitude.toFixed(6))for readability. Bikeshed.- The new
'Squitter (ID / Uplink Test — ground-to-air broadcast)'description value contains an em dash. Some downstream consumers may not handle non-ASCII well. Probably fine but worth noting. - The category interpretation
'X (ground-to-air uplink broadcast)'mixes raw and gloss invalue. Same comment as #421 — consider keeping the raw character only and exposing the gloss separately. - The
latd: [NS]group when latd === 'S' is honored, but the docblock notes it's not observed in the wild. Worth keeping a synthetic unit test for completeness.
Backward Compat / Regression Risk
- Output values change: anywhere
decodeResult.raw.groundStation.coordinates.latitudewas previously40.75(from4075/100), it's nowMath.round((40 + 75/60) * 1e6) / 1e6 = 41.25. So consumers reading these coords will see different numbers — but the old numbers were wrong, so this is correctness-positive. - Regex tightening regresses real messages? The lat/lng widths went from
\d+to\d{4}/\d{5}. Any squitter with a non-standard width that previously matched will now fail. The PR body asserts that\d{4}/\d{5}is the documented DDMM format, so this should be safe — but worth checking the Vancouver case0EYVRCYVR14911N12310WV136975/ARINCmentioned in your docblock: lat4911(4 digits ✓), lng12310(5 digits ✓). Good. - The
Bseparator broadening is a strict acceptance widening — pure additive; reduces false negatives. - New rows (
MSGTYP,CAT,SEP,PROVSFX) informatted.items— purely additive; existing positional consumers (e.g.items[0]previously wasNETT) will see different rows becauseMSGTYPis unshifted to the front. This is a position breaking change; please call it out in the PR body. The existing test (rightly) uses.find(i => i.code === 'NETT')rather than positional indexing, so the find-pattern is robust — but downstream consumers may not be.
Test Coverage
Currently broken. Once fixed:
- JFK with realistic DDMM (above).
- Gatwick
02XALGWEGKK15109N00012WB136975/ARINC— exercises DDDMM with leading zeros AND theBseparator. - A southern-hemisphere or eastern-hemisphere fixture (synthetic) to exercise the sign flips.
- Negative test for invalid lat/lng width.
Outstanding Existing Comments
Four inline comments from Copilot. My stance, replied inline:
- #3141132042 (tests not updated): Strongly agree. This is the blocker. Must Fix.
- #3141132051 (
\w→[A-Z]tightening): Agree. Should Fix. - #3141132063 (
result.length >= 8is fragile): Agree. Should Fix. - #3141132072 (
var→let/const): Agree. Should Fix.
All four are actionable and uncontested.
Thanks @thepacket! The DDMM fix is exactly right and the cross-check against real airport coords is the right way to verify. Please update the test fixture, add the B-separator and Gatwick cases, and address the Copilot points before merge.
Two fixes to
Label_SQground-station squitters:Separator no longer constrained to
V—B(and other letters) observed in real messages (02XALGWEGKK15109N00012WB136975/ARINC). Regex now accepts any uppercase letter and exposes it asSEPin the formatted output.Coordinate encoding corrected to DDMM decimal-minutes — was previously dividing the raw integer by 100, conflating minutes with hundredths. Now correctly splits
5109→ 51°09′ → 51.15 and00012→ 000°12′ → 0.20. Cross-checked Gatwick (51.15, -0.2 ✓) and KFTY (33.766667, -84.516667 ✓; previously rendered as 33.46).Lat/lng regex widths tightened to
\d{4}/\d{5}to match the documented DDMM/DDDMM format.npm run buildpasses.