Add Label A6 ADS-C Periodic Position decoder plugin#409
Add Label A6 ADS-C Periodic Position decoder plugin#409thepacket wants to merge 1 commit intoairframesio:masterfrom
Conversation
New plugin registered in official.ts and MessageDecoder.ts.
|
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 29 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 (3)
✨ 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
Adds Label_A6_ADS to surface ARINC-622 envelope fields (ground address, IMI, tail, hex payload, optional CRC) for label A6. Plugin is registered in both lib/MessageDecoder.ts and lib/plugins/official.ts. Build passes per the description. Thoughtful inline ASCII art makes the wire format easy to follow.
Verdict
Needs changes before merge. The decode logic is reasonable for an unparsed-payload pass-through, but a few correctness concerns and the lack of tests are blockers.
Must Fix
- No tests. Every other plugin under
lib/plugins/ships a companion*.test.ts. Please add one with at least one happy-path message and one malformed input that hits the early-return branches. Uselib/plugins/Label_15.test.tsas a template. - PR title vs. content mismatch. The PR title says "ADS-C Periodic Position" but A6 in ARINC 620 is "Request ADS Reports" (RAR), and the plugin description correctly reflects that ("Request ADS Report (RAR) — ADS-C contract"). Either rename the PR/file to match the RAR semantics, or clarify why "Periodic Position" is the intended product name. Right now a downstream consumer searching for "A6" gets contradictory descriptors.
- Speculative 2-char CRC branch. ARINC-622 ATS messages use a 16-bit CRC encoded as 4 ASCII hex digits. The
else if (adjAfter.length >= 6)branch (line 111-114) splits the trailing 2 chars and labels themcrc_or_tail_byte("short — possibly payload byte"). This is guessing. Please either (a) only emit a CRC when there are exactly 4 trailing hex chars, or (b) leave those bytes insideads_payload_hexand skip the CRC field entirely. Surfacing an "uncertain" CRC under the samecrc_hexraw key as the real CRC will confuse downstream code.
Should Fix
- Tail-extraction heuristic is brittle. The "look at the first 8 chars and find the last non-hex char" rule will misidentify any tail composed entirely of hex characters (e.g.
JA788A,B12345,A6CDEF, …). The cascade approach used in PR #411 (Label_AA_CPDLC.ts) — try a list of known registration patterns and accept the first whose remainder is even-length hex — would be more robust here too. Consider extracting that into a shared utility so all FANS 1/A plugins use the same logic. - Non-standard
rawkeys.air_data,ads_payload_hex,crc_hex,imiaren't part of the canonicalRawFieldsset defined inlib/DecoderPluginInterface.ts. The interface allows extension via the index signature, but please check with maintainers whether you should add the well-known ones (imi,crc) to that interface for consistency across A6/AA/B0 plugins. payload_hexvalue contains the byte count.value: '${hex} (${hex.length / 2} bytes)'— putting the byte count into the human-readable value is fine, but storing the byte count separately as a raw field (the way PR #411 does withcpdlc_payload_bytes) is more downstream-friendly.
Nits
- The "Direction" item is hardcoded to "Uplink (ground → aircraft)" but the docstring concedes A6 can carry RAR or other ADS-C protocol messages depending on IMI. Consider deriving direction from the IMI rather than asserting uplink unconditionally.
decodeResult.raw.imi = imi;— also surface this informatted.itemsonly if it's something other thanADS, otherwise it's redundant with the Message Type row.
Tests
Missing entirely. Suggested fixtures:
- Happy path:
/NYCODYA.ADS.N183AM 0804140A28C4 01(matches the wire-format example). - Hex-only tail: a
JA…registration that's all hex chars, to confirm the cascade can still find a sensible split (this will require the cascade fix above to pass). - Malformed: missing IMI dot, missing payload, non-hex payload — should set
decoded: falseanddecodeLevel: 'none'.
Notes
- Registration: confirmed in both
lib/MessageDecoder.ts(line 81) andlib/plugins/official.ts(line 67) at the PR HEAD. - FANS 1/A consistency: please coordinate the tail-extraction logic and
crcraw-key naming with PR #411 (Label AA) and PR #412 (Label B0) so the three plugins behave the same on overlapping fields.
Thanks @thepacket!
Adds a decoder for FANS 1/A ADS-C periodic position reports carried under label A6. Ground + tail + contract-request-number surfaced; binary payload preserved as hex for downstream ARINC-745 decoding.
npm run buildpasses.