Add PDU mode for concatenated multi-segment SMS#2
Merged
Conversation
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd PDU mode for concatenated multi-segment SMS with rate limiting and reassembly
WalkthroughsDescription• Implements PDU mode (AT+CMGF=0) for SMS send/receive, replacing text mode to support messages of any length • **New modules:** - pdu.c/h — 3GPP TS 23.040 encoder/decoder with UCS2/GSM7 support, UDH concatenation, and comprehensive Unicode sanitization (strips null bytes, bidi overrides, zero-width chars, BOM, variation selectors, tag blocks) - sms_reassembly.c/h — 8-slot bounded reassembly store with per-sender cap (2 slots), 10-min TTL, LRU eviction weighted by fragment count, and duplicate detection - sms_io.c/h — Extracts SMS send/CMTI handling from main daemon, supports both PDU and legacy text modes with inter-segment pacing (150ms default) • **Rate limiting overhaul:** Switched from 64-entry ring buffer to leaky-bucket counter (fixes silent rejection at limits > 64); separate segment-level bucket for 200/hr budget • **Concurrency improvements:** Dual command queues (MQTT commands drain ahead of deferred CMTI events) prevent hangup/dial stalls behind multi-segment bursts; CMGR/CMGD use 500ms timeout • **MQTT schema extension:** Optional data object for segments_sent/segments_total observability; existing consumers ignore unknown fields • **Rollback support:** --legacy-sms flag enables text mode fallback • **AT command layer:** New at_command_send_pdu() for two-phase PDU transmission with hex validation • **Modem init refactored:** pdu_mode parameter selects AT+CMGF mode; AT+CSMP only sent in text mode • **Documentation:** CLAUDE.md aligned with OASIS house style (critical rules top, @imports for standards, terse gotchas) • **Test coverage:** 19 PDU tests (encode/decode, UDH, sanitization, GSM7, round-trip) + 8 reassembly tests (eviction, caps, timeouts, duplicates); all 6 suites pass Diagramflowchart LR
A["SMS Input<br/>CMTI Event"] -->|"pdu_decode<br/>UDH concat"| B["sms_reassembly<br/>8-slot store"]
B -->|"complete"| C["Inbound SMS<br/>reassembled"]
D["MQTT Command<br/>send SMS"] -->|"sms_io_send<br/>PDU encode"| E["Rate Bucket<br/>leaky-bucket"]
E -->|"segment pacing<br/>150ms"| F["AT+CMGS<br/>PDU mode"]
F -->|"SIM7600"| G["Outbound SMS<br/>multi-segment"]
H["Config"] -->|"pdu_mode flag"| I["modem_init<br/>AT+CMGF=0/1"]
I -->|"selects"| F
I -->|"selects"| A
File Changes1. src/pdu.c
|
Code Review by Qodo
1. at_write_raw() return ignored
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+508
to
+510
| char esc = 0x1B; | ||
| at_write_raw(ctx, &esc, 1); | ||
| ctx->pending.type = AT_PENDING_NONE; |
There was a problem hiding this comment.
1. at_write_raw() return ignored 📘 Rule violation ≡ Correctness
In at_command_send_pdu, the timeout path calls at_write_raw() without checking whether the ESC abort write succeeded. Ignoring this return can hide serial I/O failures and leave the modem in an unexpected state after a prompt timeout.
Agent Prompt
## Issue description
`at_command_send_pdu()` ignores the return value of `at_write_raw()` when attempting to send an ESC to abort `AT+CMGS` after a prompt timeout.
## Issue Context
This is error-handling code on a serial I/O path; failing to detect/log an abort-write failure makes diagnosing modem state issues much harder.
## Fix Focus Areas
- src/at_command.c[508-516]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+224
to
+230
| char data_buf[128]; | ||
| build_segment_data_json(data_buf, sizeof(data_buf), sent, total); | ||
|
|
||
| switch (rc) { | ||
| case SMS_IO_SEND_OK: | ||
| mqtt_publish_response(action, request_id, true, NULL, NULL, NULL, data_buf); | ||
| break; |
There was a problem hiding this comment.
2. sms_io.c ignores return values 📘 Rule violation ≡ Correctness
New code ignores return values from at_command_send() and build_segment_data_json(), and then may publish an uninitialized data_buf to MQTT. This can mask CMGD failures and can produce malformed MQTT responses under buffer/allocation failure paths.
Agent Prompt
## Issue description
`sms_io.c` introduces ignored return values:
- `cmgd_fire_and_forget()` discards `at_command_send()` status.
- `sms_io_send_and_respond()` discards `build_segment_data_json()` status and may pass an uninitialized `data_buf` to `mqtt_publish_response()`.
## Issue Context
Even if these failures are rare, ignoring them makes behavior non-deterministic during low-memory conditions or modem/storage errors.
## Fix Focus Areas
- src/sms_io.c[48-53]
- src/sms_io.c[224-230]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+383
to
+387
| at_status_t rc = at_command_send(at, cmd, &resp, AT_TIMEOUT_SMS_STORAGE); | ||
| if (rc != AT_OK) { | ||
| OLOG_WARNING("CMGR PDU failed at idx %d: %s", sms_index, at_status_str(rc)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
3. Cmgr failure skips delete 🐞 Bug ☼ Reliability
On AT+CMGR failure in both text-mode and PDU-mode inbound handlers, the code returns without issuing CMGD, leaving the message in modem storage. This contradicts the new short storage timeout policy and can cause the modem inbox to fill, preventing further SMS reception.
Agent Prompt
### Issue description
Inbound CMTI handlers return on `AT+CMGR` failure without deleting the modem storage slot. With the new 500ms storage timeout, transient failures can accumulate undeleted messages until the modem inbox fills.
### Issue Context
`AT_TIMEOUT_SMS_STORAGE` is introduced specifically to avoid long main-thread stalls and the comment states the timeout path should fall back to CMGD fire-and-forget.
### Fix Focus Areas
- src/sms_io.c[306-314]
- src/sms_io.c[379-387]
- include/echo.h[56-63]
### Recommended fix
- In both handlers, when `rc != AT_OK`, log and call `cmgd_fire_and_forget(at, sms_index)` before returning.
- If you still want to preserve messages on *non-timeout* errors, gate the deletion specifically on `rc == AT_TIMEOUT` (and possibly other retryable cases), but ensure the documented timeout policy is enforced.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Flip ECHO from text mode (AT+CMGF=1) to PDU mode (AT+CMGF=0) so SMS send/receive works for messages of any length. Outbound is UCS2-only; inbound decodes both UCS2 and GSM7 (most handsets default to GSM7 for plain ASCII). Multi-segment messages concatenate via the UDH reference header on both paths. New modules: - pdu.c/h — encoder, decoder, UDH concat, UCS2+GSM7, hardened bounds checks at every length prefix, UCS2 sanitization (strips NULs, bidi overrides, zero-width, BOM, variation selectors, Unicode tag block) - sms_reassembly.c/h — 8-slot inline store, 2 slots/sender, 10-min TTL, LRU eviction weighted by fragment count - sms_io.c/h — extracts send + CMTI handling from oasis-echo.c, keeps the main daemon under its size limit Rate buckets reworked to a leaky-bucket counter; the old 64-entry ring silently never rejected at limits > 64 (fix needed for the 200/hr segment budget). Inter-segment pacing (150ms default) avoids SIM7600 wedging on back-to-back concat sends. CMTI handling now uses a dedicated deferred queue drained after MQTT commands, and CMGR/CMGD use a 500ms timeout, so a 10-segment inbound burst can no longer stall a concurrent hangup/dial for 40s. MQTT response schema gains an optional `data` object for segments_sent / segments_total observability. Existing consumers ignore unknown fields. Legacy text mode reachable via --legacy-sms for rollback. Tests: 19 PDU + 8 reassembly. All 6 suites pass. Also: align CLAUDE.md with the OASIS house style (same structure as DAWN's CLAUDE.md — critical rules up top, @imports for ARCHITECTURE / CODING_STYLE_GUIDE, terse gotchas section).
KerseyFabrications
force-pushed
the
feature/pdu-concatenated-sms
branch
from
6f07fcc to
6502a4f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Flip ECHO from text mode (AT+CMGF=1) to PDU mode (AT+CMGF=0) so SMS send/receive works for messages of any length. Outbound is UCS2-only; inbound decodes both UCS2 and GSM7 (most handsets default to GSM7 for plain ASCII). Multi-segment messages concatenate via the UDH reference header on both paths.
New modules:
Rate buckets reworked to a leaky-bucket counter; the old 64-entry ring silently never rejected at limits > 64 (fix needed for the 200/hr segment budget). Inter-segment pacing (150ms default) avoids SIM7600 wedging on back-to-back concat sends.
CMTI handling now uses a dedicated deferred queue drained after MQTT commands, and CMGR/CMGD use a 500ms timeout, so a 10-segment inbound burst can no longer stall a concurrent hangup/dial for 40s.
MQTT response schema gains an optional
dataobject for segments_sent / segments_total observability. Existing consumers ignore unknown fields.Legacy text mode reachable via --legacy-sms for rollback.
Tests: 19 PDU + 8 reassembly. All 6 suites pass.
Also: align CLAUDE.md with the OASIS house style (same structure as DAWN's CLAUDE.md — critical rules up top, @imports for ARCHITECTURE / CODING_STYLE_GUIDE, terse gotchas section).