[codex] SafetyNET/Inmarsat and NMEA amendment cleanup#11
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR replaces the comm.inmarsat data model with a new notifications.inmarsat.safetynet model, reworking Inmarsat SM/SMB header/body assembly, CR terminal sentence handling, and commit logic. It adds NMEA Phase 3 sentence support (ABK/AGA/BCL/MOB), extends the sentence parser for tag blocks and SMB fragments, updates RX connector dispatch wiring, and adjusts CI/build/test infrastructure. ChangesInmarsat SafetyNET Migration
NMEA Phase 3 Amendments (ABK/AGA/BCL/MOB)
Build and CI Infrastructure
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant RxConnector as Nmea0183RxConnector
participant Inmarsat as apply_inmarsat
participant Assembly as NmeaSafetyNetAssemblyRecord
participant Model as notifications.inmarsat.safetynet
RxConnector->>Inmarsat: apply_sentence(SM1..SM4 / SMB)
Inmarsat->>Assembly: apply_safetynet_header / apply_safetynet_smb
Assembly-->>Inmarsat: fragment/body state
Inmarsat->>Model: commit_inmarsat_safetynet_message
Model-->>Inmarsat: latest_message + history updated
sequenceDiagram
participant RxConnector as Nmea0183RxConnector
participant CrHandler as apply_inmarsat_can/crq/dsm/tmd
participant SafetyNet as safetynet.latest_message
RxConnector->>CrHandler: apply_sentence(CAN/CRQ/DSM/TMD)
CrHandler->>SafetyNet: inmarsat_cr_mark_message_acknowledged / field updates
SafetyNet-->>CrHandler: updated metadata/timestamps
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 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.
💡 Codex Review
signalk-mini/modules/nmea0183/src/nmea0183_connector/sentence_parser.hpp
Lines 128 to 129 in d39b693
When a multi-fragment SMB body arrives while an IMK/CRTMD multipart message is in progress, marking SMB as a generic fragment here makes apply_sentence() feed it through update_multipart_message_state() before the dedicated SafetyNET assembler runs. That resets state_.inmarsat_message to the SMB identity, so the next fragment of the original Inmarsat/CRTMD message is treated as a bad non-first fragment and never commits. Since apply_safetynet_smb() already assembles SMB in state_.inmarsat_safetynet, avoid routing SMB through the shared multipart record or skip the generic update for it.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
This PR now covers phases 1 through 4 of the SafetyNET/Inmarsat-C cleanup.
Phase 1 moves Inmarsat storage out of comm and into notifications.inmarsat.safetynet, while keeping NMEA sentence interpretation in the connector.
Phase 2 adds structured SM1, SM2, SM3, SM4, and SMB SafetyNET support, including header fields, area addressing, body assembly, completion state, duplicate tracking, and mandatory reception flags.
Phase 3 adds handlers for ABK, AGA, BCL, and MOB using existing typed model records. ABK maps into AIS acknowledgement and data-link status. AGA maps into AIS group assignment. BCL maps into AIS base-station location data. MOB maps into the existing special notification vessel-message record.
Phase 4 adds CR terminal/network handling for CRCAN, CRCRQ, CRDSM, and CRTMD. Cancel/query/status use existing notification event/text records, and CRTMD payloads are stored under notifications.inmarsat.safetynet with multipart assembly.
Validation
Latest CI passed: Linux build/test and Arduino AtomS3R compile are green. The Linux test step includes the fixture test target.
Summary by CodeRabbit
New Features
Bug Fixes