feat(ble): reassemble multi-fragment frames for every session command (#42) - #48
feat(ble): reassemble multi-fragment frames for every session command (#42)#48kb1ibt wants to merge 1 commit into
Conversation
…flip-dots#42) Fragment reassembly lived inside _process_telemetry_packet and only ran for _TELEMETRY_COMMANDS, so any other multi-fragment session frame (e.g. the C2000 G2 c490 device-info blob) had only its first fragment decrypted and the rest dropped (flip-dots#42). Extract it into a shared _reassemble()/_join_fragments() that runs in _process_notification ahead of the cipher split, so telemetry and unknown session frames share one reassembler regardless of the AES variant (GCM vs CBC). Single vs fragment is decided by the live notification length (ATT_MTU - 3, via the ff09 _FRAME_OVERHEAD) rather than the frag byte, so families that put no frag byte on singles (the A91B2 station) need no per-device override; a short single keeps a 0x11 frag byte only when it is a valid single marker. Runs start only on index 1 and terminate on the <index><total> count (so an exact multiple of the cap, with no short tail, still completes); a partial/cold fragment that cannot decrypt is dropped rather than crashing the notification handler. Adds tests/test_reassembly.py (single-no-frag, 0x11 single, two-fragment, exact-multiple-no-tail, cold index!=1) and gives the mock client a realistic 256-byte MTU so the length gate exercises as it does on device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
flip-dots
left a comment
flip-dots
left a comment
There was a problem hiding this comment.
I have not tested this yet but it looks pretty good, I have just a few questions and improvements I would like to see to the tests and comments.
| return dev | ||
|
|
||
|
|
||
| def test_short_single_without_frag_byte_kept_whole() -> None: |
| #: (e.g the C1000 Gen 2 uses ``c421``/``c900`` instead of ``c402``/``c405``). | ||
| _TELEMETRY_COMMANDS: tuple[str, ...] = ("c402", "4300", "c405") | ||
|
|
||
| #: Fixed ff09-frame overhead between the on-wire notification value and the |
There was a problem hiding this comment.
This comment is not very useful for someone who does not know thew context of this PR. It should ideally make sense on its own.
| _LOGGER.debug( | ||
| f"Initializing Solix device '{ble_device.name}' with" | ||
| f"address '{ble_device.address}' and details '{ble_device.details}'" | ||
| f"address '{ble_device.address}' and details '{ble_device.details}'", |
There was a problem hiding this comment.
Ideally these unrelated formatting changes would not be present in the PR since it makes reviewing more difficult but I am fine with them being left in since the tooling complains if you don't and I have not properly formatted the codebase yet.
| telemetry. | ||
| def _reassemble(self, cmd: bytes, payload: bytes) -> bytes | None: | ||
| """Reassemble a possibly-fragmented session frame into one payload. | ||
|
|
There was a problem hiding this comment.
This does a good job of explaining how the fragmentation works but not what it is.
I.e maybe start the main comment body with something like:
Some Anker devices split the payload across multiple packets, this function is used to combine fragmented payloads or pass through non-fragmented payloads before they can be further processed and/or decrypted.
| ``None`` while fragments are still outstanding. | ||
| """ | ||
| if not payload: | ||
| return payload |
| self._fragment_buffers[cmd_key][index] = payload[1:] | ||
| return self._join_fragments(cmd_key) | ||
|
|
||
| # A run only starts on a full-length first fragment. The length guard stops a |
There was a problem hiding this comment.
Ideally leave out specific terminology like a "run", something like: "subsequent fragments of a fragmented payload will always be preceded by a fragment which is the maximum size and this is used for x and the maximum size is determined by y" would get the same message across without needing to know additional context.
| self._fragment_totals[cmd_key] = total | ||
| return self._join_fragments(cmd_key) | ||
|
|
||
| # Standalone single notification. Strip the frag byte only when it is a valid |
There was a problem hiding this comment.
Try to stick to pre-existing terminology like fragmented, non-fragmented, etc.
| return payload | ||
|
|
||
| def _join_fragments(self, cmd_key: bytes) -> bytes | None: | ||
| """Join a completed fragment run, or ``None`` if more fragments are due.""" |
There was a problem hiding this comment.
Try to avoid the use of run, something like: Return the merged payload if all fragments are present in the buffer and reset or return None if fragments are still missing.
| if len(self._fragment_buffers[cmd_key]) < fragment_total: | ||
| _LOGGER.debug("Waiting for remaining fragments...") | ||
| return | ||
| async def _process_telemetry_packet( |
There was a problem hiding this comment.
I think it makes more sense to move the _process_telemetry_packet() override from prime_device.py to device.py. Catching exceptions here isn’t super useful.
|
|
||
| # Reassemble multi-fragment frames before the cipher, so telemetry | ||
| # and unknown session frames share one reassembler (SolixBLE #42). | ||
| payload = self._reassemble(cmd, payload) |
There was a problem hiding this comment.
Would it not make more sense to put the re-assembler before the payload is used anywhere? That way _listen_for_packet() can be used to listen for fragmented payloads?
| mock_bleak_client.start_notify.side_effect = self.start_notify | ||
| # Emulate an Anker 256-byte ATT MTU so fragment reassembly (which gates on | ||
| # the live ``mtu_size - 3`` notification cap) behaves as it does on device. | ||
| mock_bleak_client.mtu_size = 256 |
There was a problem hiding this comment.
I think we are going to need tests for different MTU sizes, someone reports the MTU size for their F2000 is 247 rather than 256.
2 participants
Split out of #45 per review feedback — the first of several smaller, focused PRs.
Reassembles multi-fragment BLE frames for every session command before the cipher, so telemetry and unknown session frames share one reassembler instead of only reassembling telemetry. Fixes truncated/dropped multi-packet frames on commands whose payloads exceed the MTU.
device.py: unify fragment reassembly across all session commands — per-command buffers, single-vs-fragment classification, and a full-length threshold derived from_FRAME_OVERHEAD(not hardcoded).tests/test_reassembly.py: classification, the length threshold, 3-fragment runs, and interleaved per-command buffers.Closes #42.
This is the base cut of the #45 split; the c490 summary decode, Prime negotiation, Prime device support, and docs will follow as separate PRs.