ParadoxAlarmInterface/pai

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

已關閉

#611 建立於 2026年8月10日

 (0 則留言) (0 個反應) (0 位負責人)Python (113 個分叉)auto 404
bugenhancementgood first issueprotocol

倉庫指標

星標
 (414 顆星)
PR 合併指標
 (PR 指標待抓取)

描述

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.

貢獻者指南