Add Label A6 ADS-C Periodic Position decoder plugin#409
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
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!
There was a problem hiding this comment.
Pull request overview
Adds a new Label_A6_ADS decoder plugin for ARINC-622/ADS-C messages carried under ACARS label A6, and registers it in both the plugin barrel (lib/plugins/official.ts) and the MessageDecoder plugin list. The decoder parses a /GROUND.IMI.REST envelope and uses a hex/non-hex heuristic to split the trailing portion into aircraft registration, ADS-C payload (hex), and an optional trailing field.
Changes:
- New
Label_A6_ADSplugin parsing the ARINC-622 envelope and ADS-C body for label A6. - Plugin export added to
lib/plugins/official.ts. - Plugin registered in
MessageDecoder'spluginClasseslist.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/plugins/Label_A6_ADS.ts | New decoder; envelope regex, tail/payload heuristic, formatted output items. |
| lib/plugins/official.ts | Re-exports the new plugin from the barrel module. |
| lib/MessageDecoder.ts | Adds the new plugin class to the registered pluginClasses array. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| decode(message: Message, options: Options = {}): DecodeResult { | ||
| const decodeResult = this.initResult( | ||
| message, | ||
| 'Request ADS Reports (RAR — ADS-C contract, uplink)', | ||
| ); |
| * / NYCODYA . ADS . N183AM 0804140A28C4 01 | ||
| * | | | | | | | | | ||
| * | | | | | | | └── Possible trailing CRC (often 4 hex chars, sometimes absent/shorter) | ||
| * | | | | | | └─────────────── ADS-C binary payload, hex-encoded (contract parameters) | ||
| * | | | | | └────────────────────── Air-station address (aircraft registration / tail) | ||
| * | | | | └──────────────────────── Separator | ||
| * | | | └─────────────────────────── IMI (Embedded Message Identifier), typ. "ADS" | ||
| * | | └────────────────────────────── Separator | ||
| * | └────────────────────────────────────── Ground-station ATS facility address (6–8 chars) | ||
| * └──────────────────────────────────────── ARINC-622 message-start delimiter |
| if (decodeResult.raw.crc_hex) { | ||
| const c = String(decodeResult.raw.crc_hex); | ||
| decodeResult.formatted.items.push({ | ||
| type: 'crc', | ||
| code: 'CRC', | ||
| label: 'CRC (trailing hex)', | ||
| value: c.length === 4 ? `${c} (4-hex standard CRC)` : `${c} (short — possibly payload byte)`, | ||
| }); | ||
| } |
| // Heuristic: look at the first 8 chars and find the last non-hex | ||
| // character position `p`. If p >= 3, use p+1 as the split (tail is | ||
| // chars 0..p inclusive). Otherwise default to a 6-char tail, | ||
| // which is the most common registration length. | ||
| let tailLen = 6; | ||
| const window = rest.substring(0, Math.min(8, rest.length)); | ||
| let lastNonHexIdx = -1; | ||
| for (let i = 0; i < window.length; i++) { | ||
| if (!/[0-9A-F]/.test(window[i])) lastNonHexIdx = i; | ||
| } | ||
| if (lastNonHexIdx >= 3) { | ||
| tailLen = lastNonHexIdx + 1; | ||
| } | ||
| tailLen = Math.min(tailLen, rest.length); | ||
|
|
||
| const tailGuess = rest.substring(0, tailLen); | ||
| const afterTail = rest.substring(tailLen); | ||
|
|
||
| // Validate that afterTail is all-hex; if not, shorten tail until it is | ||
| // (covers the case where our heuristic over-included a hex-only | ||
| // trailing character into the tail). | ||
| let adjTail = tailGuess; | ||
| let adjAfter = afterTail; | ||
| let adjustments = 0; | ||
| while ( | ||
| adjAfter && | ||
| !/^[0-9A-F]+$/.test(adjAfter) && | ||
| adjTail.length > 4 && | ||
| adjustments < 4 | ||
| ) { | ||
| adjTail = adjTail.substring(0, adjTail.length - 1); | ||
| adjAfter = rest.substring(adjTail.length); | ||
| adjustments++; | ||
| } | ||
|
|
| * determined cleanly (e.g. the tail contains only hex characters), the | ||
| * raw trailing data is still exposed for the user under `air_data`. | ||
| */ | ||
| export class Label_A6_ADS extends DecoderPlugin { |
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.