Follow-up to review of the two preceding commits.
The write lock added in 00135bb prevented concurrent writes from dropping the
link, but bounded nothing. CommandHandler's own timeout does not cover the
write: send() awaits _sender_func() and only afterwards arms
asyncio.wait(futures, timeout=...). So a stalled write -- observed on hardware
running to minutes -- held the lock indefinitely while every other command
queued behind it, with nothing logged, no error raised and no DISCONNECTED
event. Before the lock only the stalled command hung; the others went out and
could trip the error-19 disconnect, which at least recovered. The lock turned
a bounded, self-healing failure into an unbounded silent one, and also blocked
the post-reconnect CMD_APP_START behind the dead connection's holder.
The write (lock acquisition included) is now bounded by
BLEConnection.WRITE_TIMEOUT. On expiry the link is torn down rather than the
lock merely released: the underlying CoreBluetooth write may still be in
flight, and a second write racing it re-creates the overlap the lock exists to
prevent. Tearing down hands over to the reconnect path, which is bounded and
self-healing.
The lock is now a lazily-created property, mirroring _mesh_request_lock in
commands/base.py, so an instance built without __init__ still works. The two
BLE tests previously assigned _write_lock themselves, which meant deleting the
__init__ line left them green; there is now a test that __init__ provides it.
Also in send_anon_req:
- A failed change_contact_path() no longer proceeds. The device still has the
contact as flood, so sendAnonReq() floods the request, and the server gates
REGIONS/OWNER/BASIC behind isRouteDirect() and drops it -- the caller then
waits out a full path-scaled timeout for a reply that cannot arrive. It now
returns ERROR path_reset_failed.
- encode_reply_path() clamps to the server's 64-byte reply_path buffer, which
MyMesh.cpp memcpys into with no length check, and rejects hash mode 3 (the
4-byte hops Packet::isValidPathLen refuses). Not a regression -- the previous
encoder overflowed identically -- but this function is the chokepoint and its
comment claimed to bound the read.
- The hop count saturates at 63 instead of being masked with & 63, which would
wrap a 64-hop path to zero hops, i.e. request a zero-hop reply from a distant
node. Not reachable from a device-sourced contact (the reader caps the field
at 63) but silent if it ever were.
- The suggested_timeout multiplier now scales by the hops actually emitted
rather than the contact's claimed out_path_len, which can differ once the
encoder clamps or truncates.
Correction to 30446ed's message: the claim that mode 0 is "byte-for-byte
identical" is wrong. Differentially, over 30000 randomised contact fields
restricted to what a device can actually emit, mode 0 diverges in 1177 of
10118 cases -- every one of them a path containing a 0x00 byte, and in every
one the old encoder was the wrong one. The accurate claim is "unchanged for
mode-0 paths containing no 0x00 byte".
An anon request tells the server how to route its answer back. The leading
byte of that reply path packs two fields, which the server unpacks as:
reply_path_len = byte & 63
reply_path_hash_size = (byte >> 6) + 1
Three defects in producing it:
1. The hash mode was never written into the top two bits, so the server always
read a hash size of 1 whatever the contact's real mode was.
2. The path was reversed byte-wise (out_path[::-1]) rather than hop-wise. A
return path visits the same hops in reverse order with each hop's
multi-byte hash intact.
3. reader.py built out_path by stripping every NUL from the fixed 64-byte
field. That trims the padding but also eats a legitimate 0x00 inside a hop
hash, shortening the path and shifting every hop after it. It now takes
out_path_len * hash_size bytes, as PATH_DISCOVERY_RESPONSE already did.
Worked example at hash mode 2 (3 bytes per hop), for a contact two hops away
via aabbcc then ddeeff:
before: lenbyte 0x02, path ffeeddccbbaa
-> server reads 2 hops of 1 byte, replies via ['ff', 'ee']
after: lenbyte 0x82, path ddeeffaabbcc
-> server reads 2 hops of 3 bytes, replies via ['ddeeff', 'aabbcc']
The old form routes the response to hops that do not exist, so it is dropped
and the request times out.
At hash mode 0 both encodings are byte-identical -- the mode contributes
nothing to the high bits and byte-wise reversal equals hop-wise reversal for
single-byte hops -- which is why this stayed latent: mode 0 is the default.
Confirmed by the mode-0 and zero-hop tests passing unchanged against the old
code while the mode-1/mode-2 tests fail.
Scope: only anon requests routed direct to a contact with a known multi-hop
path. Flood requests are unaffected (the server answers via createPathReturn
and ignores the supplied reply path), as is login (handleLoginReq never sets
reply_path_len, so its reply always goes out flood). The neighbors zero-hop
probe is unaffected: length 0 makes hash size irrelevant.
Encoding is extracted into encode_reply_path() so it can be tested directly.
Verified on hardware only for the zero-hop case, which still works; the
multi-hop paths are covered by unit tests, as the test radio has no multi-hop
contacts to exercise on air.
Two overlapping write_gatt_char() calls on the same characteristic drop the
BLE link outright. Observed on macOS/CoreBluetooth as "BLE write failed: 19",
after which the connection is gone and the pending command never completes.
Nothing above the transport guaranteed callers were sequential: schedulers,
health checks, periodic status queries and user commands all issue commands
independently, so any unlucky overlap could take the radio down. The existing
_mesh_request_lock only guards a few binary-request helpers, not the transport.
Reproduced on a companion radio over BLE by issuing send_device_query() and
send_node_discover_req() concurrently:
before: BLE write failed: 19, connected=False, command hung >45s
after: both complete in 0.12s, connected=True
Issued sequentially the same two commands take 0.09s each and are fine, so it
is specifically the overlap. Ruled out as causes beforehand: notification load
(three discovers under a 91-packet firehose kept writes at 0.06-0.17s with the
link stable) and the discover command itself.
The lock is created lazily so it binds to the running loop, and is released on
the failure path so one failed write cannot wedge every later command.
send_anon_req() refused to send whenever the destination pubkey was absent
from the client-side contact cache, returning ERROR contact_not_found.
The contact is consulted for one thing only: building the reply-path bytes
appended to the request. The companion firmware needs no contact of its own --
since FIRMWARE_VER_CODE 13 its CMD_SEND_ANON_REQ handler synthesises a
transient anon contact for an unknown pubkey with out_path_len = 0 (zero-hop
direct). Those entries live in a reserved slot ring, are hidden from
CMD_GET_CONTACTS and are never persisted, so nothing is polluted by them.
The client-side refusal therefore blocked a case the device supports, such as
asking a freshly discovered neighbour for its regions before it has ever been
added as a contact. Fall back to a zero-hop reply path instead.
Two smaller fixes in the same function:
- out_path_len is now read once into a local rather than re-read from the
contact dict after the await. That dict is a live reference other commands
mutate in place (send_msg_with_retry's flood fallback, reset_path); if it
flipped to -1 mid-send the suggested_timeout multiplier became 4000 * 0 = 0,
registering the binary request with a zero timeout so the response was
dropped the moment it arrived.
- The value is clamped at 0. update_contact() normally reflects the change
back onto the dict, but if it fails the dict stays -1 and the unsigned
to_bytes raises OverflowError -- which skipped the reset_path at the end of
the method and left the contact pinned to zero-hop on the device.
Verified against a companion radio on fw ver 13: without the change all five
discovered repeaters were refused client-side in 0.0s with no RF sent; with it
all three answered with their region scopes in ~1.1s.
test_send_anon_req_contact_not_found is replaced -- it codified the removed
limitation -- but its original regression (a TypeError on the NoneType
subscript) stays covered.
Wire-format parity fixes in reader.py for trailing fields the
companion-radio firmware emits but the SDK decoder was dropping, plus an
over-read guard on DEFAULT_FLOOD_SCOPE. Each fix uses the existing
BATT_AND_STORAGE defensive-read pattern (up-front minimum-length check
plus per-field cumulative-length gates), so older firmware that doesn't
emit the trailing fields keeps decoding without raising. Every fix ships
a legacy-frame and modern-frame unit test pair.
AUTOADD_CONFIG (PacketType 25): companion-v1.14.0 firmware emits a
trailing max_hops byte (firmware commit 00566741); the SDK dropped it.
Adds a defensive `if len(data) >= 3` read. Pre-v1.14.0 frames decode to
`{"config": ...}` with no max_hops key.
LOGIN_SUCCESS (PacketType 0x85): the new-style RESP_SERVER_LOGIN_OK path
emits three trailing fields the SDK was dropping — server_timestamp (4B,
firmware commit 0e90b731), acl_permissions (1B, 7947e8a2), and
fw_ver_level (1B, 418ae08b), all first shipped in companion-v1.10.0.
Adds per-field length gates (>=12, >=13, >=14). Legacy 8-byte "OK"-path
frames decode unchanged.
ACK (PacketType 0x82): firmware emits a trailing 4-byte trip_time
(round-trip latency in ms) since companion-v1.0.0a (firmware commit
d9dc76f1, Jan 2025) — on the wire ~16 months but never surfaced by the
SDK. Adds an `if len(data) >= 9` read. Legacy 5-byte ACK frames decode
unchanged.
DEFAULT_FLOOD_SCOPE (PacketType 28): firmware emits a 48-byte populated
frame or a 1-byte sentinel when no scope is set. The SDK unconditionally
read 31+16 bytes, over-reading 47 bytes past the end of the sentinel
frame and dispatching `{"scope_name": "", "scope_key": ""}`. Adds an
`if len(data) >= 48` guard so the sentinel dispatches `{}`. Consumers
detecting "no scope" via `payload["scope_name"] == ""` should switch to
a key-presence check.
RAW_DATA: the payload-framing fix this branch originally carried landed
upstream first in PR #86 (commit 44b21be), with identical decode logic
(discard the reserved 0xFF byte, then read the remaining variable-length
payload). That redundant change is dropped here; this commit retains only
its regression test (test_raw_data_realistic_frame), which exercises the
upstream fix end-to-end — the upstream change shipped without a test.
CHANGELOG:
- Decode max_hops trailing byte in AUTOADD_CONFIG (companion-v1.14.0+).
- Decode server_timestamp / acl_permissions / fw_ver_level trailing
fields in LOGIN_SUCCESS (companion-v1.10.0+).
- Decode trip_time trailing field in ACK (companion-v1.0.0a+).
- DEFAULT_FLOOD_SCOPE dispatches `{}` on the 1-byte sentinel frame
instead of `{scope_name: "", scope_key: ""}`. Detect "no scope" via
key presence.
Tests: 10 tests in tests/unit/test_protocol_surface_gaps.py (legacy +
modern frame pair per finding, including a RAW_DATA regression test for
the upstream fix); all 10 pass on the rebased upstream base.
Why: companion-radio firmware has been emitting these fields on the wire
for between 2 and 16 months; SDK consumers (integrations, bots,
dashboards) lose access to data that is on the wire today.
Why: companion-radio firmware emits RESP_CODE_CHANNEL_DATA_RECV (27) for
group-channel binary data (PAYLOAD_TYPE_GRP_DATA), shipped in
companion-v1.15.0. The SDK had no PacketType value 27, no EventType, and
no reader handler, so these frames hit the unknown-packet-type
fallthrough and the payload was silently dropped.
This adds PacketType.CHANNEL_DATA_RECV = 27 (the previously-skipped enum
slot), EventType.CHANNEL_DATA_RECV, and a reader handler. The fixed
9-byte header (snr, reserved, channel_idx, path_len with the
sentinel/hash-mode encoding) reuses CHANNEL_MSG_RECV_V3's framing; the
typed tail decodes data_type (uint16 little-endian, widened from uint8
in firmware), data_len, and payload (hex string). An up-front length
gate mirrors the defensive-read pattern used by the other handlers.
data_type is exposed as an int (matching the txt_type convention) and
payload as a hex string (matching RAW_DATA's convention for binary data
of unknown encoding); attributes surface channel_idx and data_type so
subscribers can filter without unpacking the payload.
Tests: tests/unit/test_protocol_surface_gaps.py — 5 new tests (enum
slot present, direct-path frame, route-flood path_len bit-split,
under-minimum frame dropped, widened data_type round-trip). Full unit
suite: 146 passed.
Why: MeshCore.subscribe typed callbacks as Callable[[Event], Coroutine[...]]
(async only), while EventDispatcher.subscribe typed them as
Callable[[Event], Union[None, asyncio.Future]] (sync or async). Type-checkers
flag any sync handler passed through MeshCore.subscribe. Fix: align the
annotation to match EventDispatcher's union type; remove unused Coroutine import.
Refs: Forensics report finding R05
Why: send_binary_req() registered the pending request with the reader AFTER
send() returned. If a BINARY_RESPONSE arrives between send() returning and
registration (reachable for TCP-companion proxies), the reader logs "No tracked
request found" and the caller's wait_for_event times out. Fix: pre-register a
placeholder keyed by object id before send(), then swap it for the real tag
from MSG_SENT. On send() failure, the placeholder is cleaned up.
Refs: Forensics report finding R03
Why: Three minor cleanup fixes. M03 adds an else branch in set_flood_scope so
unsupported scope types raise TypeError instead of UnboundLocalError. M05 removes
the dead `out_path_len >> 6` shift in update_contact (high bits always zero due
to reader masking) and initializes path_hash_mode=0 explicitly. M07 normalizes
three `return None` paths in get_contacts to return Event(EventType.ERROR, ...)
so callers can rely on the return type always being Event.
Refs: Forensics report findings M03, M05, M07
Why: 5 seconds is too short for slow-path mesh operations (path-resolving
messaging, long binary responses, remote auth). Also the root cause of tests
that appeared to "hang" — they were falling through to the 5s timeout because
their mock dispatchers don't wire matching responses. Landed as a separate
commit so reviewers can drop it independently if they push back.
Refs: Forensics report finding F09
Why: req_mma() references undefined variables `start` and `end`, causing a
NameError on every call. The logger.error migration warning confirms the method
is intentionally deprecated in favor of req_mma_sync. Since it is broken as
shipped, removing it cannot break any working caller.
Refs: Forensics report finding F13
Why: Five CommandType enum entries had no user-facing SDK method:
SEND_RAW_DATA (25), HAS_CONNECTION (28), GET_CONTACT_BY_KEY (30),
GET_TUNING_PARAMS (43), FACTORY_RESET (51). Added send_raw_data() in
MessagingCommands, has_connection()/get_tuning()/request_factory_reset()/
confirm_factory_reset() in DeviceCommands, get_contact_by_key() in
ContactCommands. FACTORY_RESET uses a two-step token pattern as a
Python-side safety measure against accidental invocation.
Refs: Forensics report finding N09 (also N03 for get_tuning)
Why: Firmware requires strict len > 10 (MyMesh.cpp:1620). When path is
empty, send_trace() builds exactly 10 bytes (cmd+tag+auth+flags), which
is silently rejected. Appending one zero byte when the packet is <= 10
bytes satisfies the firmware gate without changing the semantic content.
Refs: Forensics report finding N05
Why: Three firmware push/response codes had no SDK handler — frames fell
through to the "Unhandled" debug log. CONTACT_DELETED (0x8F) carries a
32-byte pubkey from onContactOverwrite(); CONTACTS_FULL (0x90) is a
1-byte push from onContactsFull(); TUNING_PARAMS (23) is the 9-byte
response to CMD_GET_TUNING_PARAMS carrying rx_delay and airtime_factor.
All three now dispatch typed events. Short frames are guarded.
Refs: Forensics report findings N01, N02, N03
Why: PacketType was missing CONTACTS_FULL (0x90), emitted by
MyMesh::onContactsFull(). CommandType was missing GET_STATS (56),
used by get_stats_core/radio/packets but referenced only as literal
b"\\x38". Adding both enum entries prepares for handler and wrapper
implementations in subsequent commits.
Refs: Forensics report findings N02, R04
Why: asyncio.get_event_loop() inside an async function emits
DeprecationWarning since Python 3.10 and raises in some contexts on
Python 3.12+. The call in CommandHandlerBase.send() is always inside
a running async context where get_running_loop() is the correct API.
Refs: Forensics report finding F19
Why: On Python 3.9/3.10, asyncio.Queue() and asyncio.Lock() bind to
the running event loop at construction time. If the SDK is instantiated
from a synchronous factory before an event loop exists, both primitives
raise "RuntimeError: ... is bound to a different event loop" on first
use. Fix: EventDispatcher defers Queue creation to start(), with a
guard in dispatch() that raises RuntimeError if called before start().
CommandHandlerBase defers Lock creation via a lazy @property accessor.
Both document the contract change in class docstrings.
Refs: Forensics report finding F08
Why: EventDispatcher._process_events() calls task_done() on the queue
immediately after spawning async callback tasks. await queue.join() in
stop() therefore returns as soon as all items are marked done, even if
their async callbacks are still executing. Any caller that does
"await dispatcher.stop(); cleanup()" could race with still-running
callbacks. Fix: after queue.join(), gather all tracked background tasks
before cancelling the dispatch loop.
Refs: Forensics report finding F07
Why: Python's asyncio holds only weak references to tasks created via
create_task(). Under GC pressure (especially Python < 3.11), unretained
tasks can be silently cancelled mid-execution, and any exceptions are
swallowed as "Task exception was never retrieved." Seven call sites
across TCPConnection, BLEConnection, SerialConnection, and
EventDispatcher used fire-and-forget create_task with no stored
reference. Fix: introduce _background_tasks set and _spawn_background()
helper on each class, following the standard pattern from the asyncio
docs (task.add_done_callback(set.discard)).
Refs: Forensics report finding F05
Why: _receive_count incremented inside data_received(), which fires
once per TCP read — not once per completed MeshCore frame. Under TCP
fragmentation a single frame can arrive in multiple segments, inflating
_receive_count relative to _send_count. The disconnect-detection
heuristic (send_count - receive_count >= 5) then never fires because
the receive side is over-counted. Moving the increment into handle_rx
after a complete frame is assembled makes the counter semantically
correct: one increment per MeshCore frame dispatched to the reader.
Refs: Forensics report finding N04
Why: When BLE pairing failed during connect(), the exception was caught
as a warning and connect() continued normally. This left the transport
in a half-usable state — the BLE link was up but the pairing handshake
never completed, so encrypted characteristics could silently fail.
Now the pairing exception disconnects the client and re-raises, giving
the caller a clean failure. Updated the pre-existing
test_ble_connection_with_pin_failed_pairing test to assert the re-raise
behavior instead of the old swallow-and-continue behavior.
Refs: Forensics report finding F17
Why: handle_disconnect resets self.client to self._user_provided_client.
That client's disconnected_callback was not re-registered, so subsequent
BLE disconnects after a successful reconnect cycle were missed —
ConnectionManager never learned the link dropped again. Now re-registers
via set_disconnected_callback with a hasattr guard and try/except for
bleak version compatibility. The next connect() call would also re-create
the client with the callback, but this closes the gap between disconnect
and reconnect.
Refs: Forensics report finding F16
Why: When handle_rx detects an oversize frame header (>300 bytes), it
resets state (header, inframe, frame_expected_size) and re-calls itself
on any remaining data. But when data is empty after the header, the
method fell through to the frame-assembly code with inframe=b"",
eventually dispatching an empty frame to reader.handle_rx. Currently
absorbed by F06's umbrella try/except, but a guaranteed crash if that
guard moves. Adding a bare return after the reset block prevents the
fallthrough in both TCP and serial transports.
Refs: Forensics report finding M06
Why: After create_serial_connection, connect() awaited
_connected_event.wait() with no timeout. If the serial device opened
but connection_made was never called (driver bug, USB adapter glitch),
connect() hung indefinitely. Now wrapped in asyncio.wait_for with a
configurable timeout (default 10s). asyncio.TimeoutError propagates
to the caller for clean failure handling.
Refs: Forensics report finding F18
Why: TCP was the only transport that fired _disconnect_callback on a dead
transport or send failure. Serial and BLE returned silently, leaving
ConnectionManager unaware the link was down. Additionally, transport.write()
(TCP/serial) and write_gatt_char (BLE) had no try/except — any write-time
exception escaped the send() coroutine unhandled. This commit makes all three
transports symmetric: (a) fire _disconnect_callback when transport is None/dead,
(b) wrap the write call in try/except and fire _disconnect_callback on failure.
Refs: Forensics report findings F04, NEW-A
Why: send_appstart() only expected [SELF_INFO]. If firmware returned
RESP_CODE_ERR (version mismatch, unsupported feature flag), wait_for_event
never matched and the command hung until DEFAULT_TIMEOUT (5s) fired.
Bootstrap is called on every initial connect, so a 5s hang on error was
user-visible. Now expects [SELF_INFO, ERROR] so firmware errors are
returned immediately as Event objects instead of timing out.
Refs: Forensics report finding M02
Why: When send_msg() returned an ERROR event (e.g. firmware rejected
the send), the error-check logged the failure but did not return or
continue. Execution fell through to result.payload["expected_ack"],
which raised KeyError because the ERROR payload is {"reason": "..."}.
The retry loop — the entire purpose of this function — never ran.
Now the ERROR path increments attempt counters and continues the
loop, preserving the retry semantics the function name promises.
Refs: Forensics report findings F21, M01
Why: wait_for_event matches a single EventType; when callers pass
[X, ERROR] to send() or wait_for_events, the return value may be an
error response whose payload is {"reason": "..."} — not the command-
specific keys the caller expects. Without a documented contract and
a convenience helper, every call site independently forgets to check
.type before accessing payload keys, leading to KeyError (F21/M01,
M04) or silent fallthrough. The is_error() helper and docstrings on
send()/wait_for_events() establish the contract that subsequent
commits in this branch rely on.
Refs: Forensics report finding F22
ConnectionManager._attempt_reconnect called self.connection.connect()
directly, bypassing MeshCore.connect() which runs send_appstart().
Firmware requires CMD_APP_START after every transport-level connection
to initialize the session. Without it, the reconnected transport has
no active session — sends go unanswered, tcp_no_response fires after
5 attempts, handle_disconnect re-enters _attempt_reconnect, and the
reconnect storm begins.
Fix: add an optional reconnect_callback parameter to
ConnectionManager.__init__. MeshCore passes self._on_reconnect which
calls send_appstart() after the transport reconnects. The callback
is invoked inside _attempt_reconnect immediately after a successful
connect(), before the CONNECTED event is emitted. Callback failures
are logged as warnings but do not break the reconnect — the transport
is up regardless. Default None keeps the API backwards-compatible
for direct ConnectionManager users.
Refs: Forensics report finding F02
_attempt_reconnect previously tail-recursed via asyncio.create_task,
re-assigning self._reconnect_task from inside the running coroutine.
This orphaned the current task — disconnect() cancelled only the
newest pointer, leaving previous-generation attempts in flight. Those
orphaned tasks could set _is_connected = True after the caller thought
the session was closed.
Fix: replace with a single iterative while loop that holds one
persistent task for the entire reconnect session. The task is created
once in handle_disconnect and torn down only on success, max attempts
exhausted, or disconnect() cancellation.
Refs: Forensics report finding F03
The three transports had inconsistent connect() return contracts:
TCP returned an asyncio.Future (fixed by F01), serial returned
self.port (always truthy), BLE returned self.address or None.
ConnectionManager's success check `if result is not None:` was
tautological for TCP and serial. With F01 fixed, the check is now
meaningful for all three.
Add a comprehensive docstring to ConnectionProtocol documenting the
contract: return truthy on success (included in CONNECTED event
payload), return None for soft failure (retry), or raise for hard
failure (also retry, logged). Also import Awaitable for the F02
reconnect_callback type hint that follows.
Refs: Forensics report finding N11
TCPConnection.connect() returned a resolved asyncio.Future wrapping
self.host instead of the plain string. ConnectionManager put this
Future directly into the CONNECTED event payload, which crashed any
downstream serializer (e.g. HA recorder's sanitize_event_data) that
tried to walk the payload dict. BLE and serial already return plain
strings. Fix: delete the Future creation and return self.host
directly.
Refs: Forensics report finding F01
A BATTERY frame with len(data) < 3 caused dbuf.read(2) to return short
bytes; int.from_bytes(b"", ...) silently yielded 0, propagating a bogus
level=0 to HA sensors. Same silent-zero class as N07 (storage fields).
Option B: early-return with debug log, matching the NEW-C pattern for
STATUS_RESPONSE. No BATTERY event is dispatched for malformed frames.
Not in the original forensics report — discovered during G1 N07 work and
logged in issues_log.md. Resolved here because no later branch touches
this handler.
Files changed:
- src/meshcore/reader.py: add `if len(data) < 3: return` guard before
the level read in the BATTERY branch
- tests/unit/test_reader.py: add test_g1_battery_too_short_for_level —
sends a 1-byte frame (type only), asserts no BATTERY event dispatched
and debug log emitted
Three small cleanup fixes bundled per proposal §4.1 commit order.
N08 — CONTROL_DATA empty payload guard. The handler reads
`payload = dbuf.read()` then immediately dereferences `payload[0]`
without checking length. A zero-length payload (firmware truncation
or garbled frame) raises IndexError. Pre-F06 the IndexError would
escape; post-F06 it would log and skip the dispatch via the umbrella.
Adding an explicit `if len(payload) == 0: return` after the read
short-circuits the empty case before it touches `payload[0]`, with a
debug log noting the empty payload. The `return` exits handle_rx
cleanly without engaging the F06 umbrella's parse-error path, which
is the correct behavior — an empty CONTROL_DATA frame is not a parse
error, it's an unusable frame.
F12 — print(res) leftover debug. The RAW_DATA handler had a stray
`print(res)` polluting stdout. Replaced with `logger.debug(res)` to
match the surrounding `logger.debug("Received raw data")` line.
N10 — magic numbers 16 and 17. Two `elif packet_type_value == 16/17`
branches hardcoded the integer values for CONTACT_MSG_RECV_V3 and
CHANNEL_MSG_RECV_V3, both already declared in packets.py:94-95.
Replaced with `PacketType.CONTACT_MSG_RECV_V3.value` and
`PacketType.CHANNEL_MSG_RECV_V3.value` to eliminate drift risk if
the enum is ever renumbered.
Findings: N08 (Info), F12 (Info), N10 (Info)
File: src/meshcore/reader.py
Florent
agessaman
Matthew Wolter
JohannesFriedrich
Jack Kingsman
Matthew Wolter