Add Label H1 APM (ACMF Snapshot) decoder plugin#417
Add Label H1 APM (ACMF Snapshot) decoder plugin#417thepacket 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 14 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_H1_APM (Aircraft Performance Monitoring / ACMF Snapshot) for H1 messages matching APM <seq> <tail> <flight> <origin> <dest> <YYMMDDHHMMSS> followed by a comma-delimited ACMF body. Registered last in MessageDecoder.ts/official.ts. No companion test file.
Verdict
Comment-only review — there's a real, demonstrable header-regex bug that breaks the headline #CFBAPM use case from the PR description. Please address before merge. There are also a handful of registration/coexistence and convention concerns specific to adding a second H1 plugin to a label that already has Label_H1_OHMA.
Must Fix
-
Header regex does not match
#CFBAPM ...— the very prefix the PR description andqualifiers().preamblesadvertises.The regex is:
/(?:^|\b)APM\s+(?<seq>\d+)\s+.../For input
#CFBAPM 6 HL8573 KAL075 RKSI CYVR 200426221336:^alternative fails — the string starts with#, notAPM.\balternative fails —\bis a transition between a word char and a non-word char. In…BAPM, bothBandAare word chars, so there is no word boundary between them.APMis not at a\bhere.
Verified empirically:
IN : "APM 6 HL8573 KAL075 RKSI CYVR 200426221336" → match @ 0 IN : "#CFBAPM 6 HL8573 KAL075 RKSI CYVR 200426221336" → NO MATCH IN : "#CFBAPM_REPORTAPM 6 HL8573 KAL075 RKSI CYVR 200426..." → NO MATCH IN : "#CFBAPM\nAPM 6 HL8573 KAL075 RKSI CYVR 200426221336" → match @ 8So the plugin currently only decodes messages that literally start with
APM(the first sample) or have the literalAPMtoken preceded by a non-word character such as\n(the fourth sample). The middle two — which are the patterns the doc-comment andqualifiers().preambles: ['#CFBAPM']actually claim to support — fail.Recommended fix: drop the
\band explicitly skip any leading prefix. For example:const m = text.match(/^(?:#?CFB)?APM\s+(?<seq>\d+)\s+(?<tail>...)/);
or strip a
#CFBAPM_REPORT/#CFBAPMlead before applying the existing regex. Either way, please add explicit test cases for the prefixed forms — that's what the qualifier preambles and the PR description both advertise. -
Add a
Label_H1_APM.test.ts. No tests at all. Required minimum:qualifiers()shape (preambles included), happy path with bareAPM ...,#CFBAPM ...,#CFBAPM_REPORT..., malformed timestamp, and a non-matching string.
Should Fix
-
Coexistence with
Label_H1_OHMA— preamble check passes (OHMA usesOHMA,/RTNBOCR.OHMA,#T1B/RTNBOCR.OHMA; APM usesAPM,#CFBAPM). No string-prefix collision. Confirmed by inspection. However, several existing H1 plugins (Label_H1_M_POS, others) declare no preambles and run for every H1 message. Their internal regexes (^M\d{2}A...) won't grab APM input, but please be explicit and assert that in tests so future refactors don't regress. -
HHMMSS validation gap.
hh,mm,ssare sliced and shoved straight intoconvertHHMMSSToTodwithout bounds checks. Garbage like999999999999would parse without complaint. Reject whenhh > 23,mm > 59,ss > 59(and ideallymo/ddlikewise). -
Date is dropped from the canonical timestamp.
ResultFormatter.timestamp(decodeResult, DateTimeUtils.convertHHMMSSToTod(...))produces a "seconds since midnight" value that loses theYYMMDDpart. You preserve the date inraw.report_dateand the UI shows it via the customcode: 'DATE'item, but downstream consumers readingraw.message_timestampwill see a time-of-day-only number that doesn't agree with the date. Either:- Use
DateTimeUtils.convertDateTimeToEpoch(hh+mm+ss, 'DD MM YY')(or the right helper — seeLabel_15.tsline 47) somessage_timestampis a real epoch, or - Skip
ResultFormatter.timestampand stick with the explicitreport_date/report_timeitems.
As-is, this hits the project's "Both
ResultFormatter.timestamp()AND explicit{type:'time'}push" antipattern (you pushreport_date/report_timeyourself in addition to the formatter call). - Use
-
ResultFormatter.temperature(decodeResult, oat)for OAT is appropriate semantically, but the formatter's value-string ('-28.81 degrees') is OAT, while ACMF snapshots also include TAT in nearby fields. Consider documenting why you intentionally only surface OAT. -
**
ResultFormatter.altitude(decodeResult, Number(alt))for35001ft is correct but ACMF altitude is typically pressure altitude — worth surfacing that as a label suffix ('Pressure Altitude') since other plugins use plain "Altitude" for GPS/baro. Optional.
Nits
decodeResult.formatted.items.unshift({...})for the message-type row is fine, but the rest of the items usepush. Mixing the two is a small readability cost; consider building items in order.- The Mach validation regex
/^\.\d+$|^\d\.\d+$/is fine but excludes0.837(with leading zero) and1.0(M1.0+ for fighters/test). Loosen to/^-?\d*\.?\d+$/if you want to be defensive. decodeResult.raw.acmf_version = acmfVer;— string is fine, but if every observed value isACMF\d{4}, consider parsing the digits separately for easier filtering.- Doc comment mentions
#CFBAPM_REPORTexplicitly — make sure your final regex actually handles that form (current one doesn't).
Tests
None added. Please add Label_H1_APM.test.ts. See "Must Fix" for coverage.
Notes — Registration & Coexistence
- Registration order in
MessageDecoder.ts: APM is appended last (line 81), afterLabel_H1_OHMA(line 64). For label-specific dispatch this is fine; thelabelIndexkeys by label and order within the label only matters when multiple plugins could match. - OHMA preambles vs APM preambles:
['OHMA', '/RTNBOCR.OHMA', '#T1B/RTNBOCR.OHMA']vs['APM', '#CFBAPM']. No prefix collision. ✓ - Preamble-less H1 plugins (
M_POS,OHMAdoes have preambles, etc.): their internal regexes won't match an APM message. Confirmed by inspection. ✓ - **The qualifier
preambles: ['APM', '#CFBAPM']is correct as written, but the dispatcher'smessage.text.startsWith(p)short-circuits on these. As long as the wire really starts withAPMor#CFBAPM, the plugin will be invoked — but then the header regex still has to actually match, which is where the bug above bites.
Thanks @thepacket! The ACMF snapshot decoding logic itself is solid — it's just the entry-point regex and missing tests that need attention.
Adds a decoder for H1 channel ACMF (Airplane Condition Monitoring Function) cruise performance snapshots, matching the
#CFBAPM_REPORTpattern.Wire format
Surfaces tail, flight, origin, destination, YYMMDDHHMMSS timestamp, ACMF version, Mach, OAT, pressure altitude, dual-engine N1/N2/EGT. Wild-guess fields (fuel flows, gross weight, flag soup) intentionally not surfaced.
npm run buildpasses.