ParadoxAlarmInterface/pai

GSM interface: replace hand-rolled CRLF framing with shared LineFramer, fix echo suppression and unbounded buffer

クローズ

#611 opened on 2026/08/10

 (0 件のコメント) (0 件のリアクション) (0 人の担当者)Python (113 件のフォーク)auto 404
bugenhancementgood first issueprotocol

Repository metrics

Stars
 (414 個のスター)
PR merge metrics
 (PR metrics pending)

説明

paradox/interfaces/text/gsm.py hand-rolls CRLF line framing inline in SerialConnectionProtocol.data_received, while the rest of the connections package has moved framing into dedicated framing modules behind ConnectionProtocol.reset_framing(). A recent cleanup of that loop (dead branch removal, delimiter consumption, lazy logging) left the structure and several pre-existing defects untouched. This issue tracks the follow-up.

Defects

1. Unbounded buffer growth

self.buffer grows without limit when no \r\n ever arrives — line noise at the wrong baud rate, a modem in PDU/binary mode, or a stuck AT+CMGS prompt awaiting \x1a. find returns -1 on every callback and the only escapes are reset_framing() or connection_lost, neither of which fires on a healthy-but-noisy link.

paradox/connections/prt3/framing.py already solves this with MAX_LINE_LENGTH = 512 plus a discard-and-warn path. FrameBuffer.take/compact also avoids the O(n^2) self.buffer = self.buffer[r + 2:] re-slicing on every frame.

2. \r-only echo defeats echo suppression

Many V.25ter modems echo the command terminated by a bare <CR>, then send <CR><LF>OK<CR><LF>. Given send_message(b"AT") followed by data_received(b"AT\r\r\nOK\r\n"), the extracted frame is b"AT\r", which does not equal last_message (b"AT"). The echo is forwarded to on_message, lands in SerialCommunication.queue, and GSMTextInterface.write treats it as the response — desynchronizing the request/response stream for the rest of the session.

Fix: frame.rstrip(b"\r") before the comparison, or compare against last_message.rstrip().

3. last_message is sticky

It is cleared only on an exact match or on connection_lost. Since connect() issues ATE0, echo is normally off, so last_message retains the last command indefinitely (e.g. b"AT+CUSD=1" after init, or the whole AT+CMGS=... blob after an SMS) and will silently swallow the first later frame that happens to equal it. Echo suppression should be scoped to the first frame after a write, not "any frame, ever".

4. A raising on_message aborts the drain loop

GSMTextInterface.data_received calls data.decode() and json.loads with no guard. A non-UTF-8 SMS or a malformed +CMT header raises straight out through protocol.data_received into the asyncio transport, stalling any already-buffered complete frames until the next byte arrives.

Structural cleanup

5. Promote LineFramer to paradox/connections/framing.py

LineFramer is fully generic (parameterised terminator, no PRT3 specifics) but lives in connections/prt3/. Importing it from interfaces/text/ today would couple GSM to an unrelated panel transport. Promote it first, updating PRT3's import.

6. Have GSM delegate to LineFramer(terminator=b"\r\n")

This closes item 1 for free. Note the semantic mismatch: LineFramer keeps the terminator and drops whitespace-only lines, whereas GSM's echo comparison and on_message consumers expect it stripped. Needs either a strip_terminator flag or a caller-side strip.

Echo suppression is an application concern and should stay in the protocol/interface layer — do not push last_message into LineFramer.

7. Rename GSM's SerialConnectionProtocol

It collides with paradox.connections.serial.protocol.SerialConnectionProtocol. The two are not substitutable — the connections one has a sync send_message (called by Connection.write()), GSM's is async def — so a mis-targeted import type-checks and fails only at runtime on the first data_received. Suggested: GsmSerialProtocol or ModemLineProtocol. Single-file rename plus one test import.

8. (Optional) Relocate SerialCommunication under connections/gsm/

It is a parallel hand-rolled reimplementation of the Connection abstraction (own queue, connected_future, open_timeout, make_protocol) sitting in interfaces/. The principled end-state is a paradox/connections/gsm/ package, leaving GSMTextInterface to do AT-command orchestration and SMS parsing only. Low urgency — GSM has one transport, one consumer, and no encryption or variable-length concerns.

Notes

The characterization tests in tests/interfaces/test_gsm.py (test_serial_connection_protocol_reassembles_split_frames, test_serial_connection_protocol_drops_echoed_message) pin the current framing contract and should let the refactor land with confidence. They are not regression tests — the pre-refactor loop passed them too.

コントリビューターガイド