feat(ble): reassemble multi-fragment frames for every session command (#42) by kb1ibt · Pull Request #48 · flip-dots/SolixBLE

kb1ibt · 2026-07-20T17:36:34Z

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.

flip-dots

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.

flip-dots · 2026-07-28T13:51:07Z

+    return dev
+
+
+def test_short_single_without_frag_byte_kept_whole() -> None:


Use test parametrization here

Folding the five cases (single with no frag byte, 0x11 single, two-fragment, exact-multiple-with-no-short-tail, cold index != 1) into one parametrized test, with the MTU as a parameter too so they run at 247 / 253 / 297 rather than only 256.

flip-dots · 2026-07-28T13:52:52Z

    #: (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


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.

Replacing it with: the ff09 frame is header(2) + length(2) + pattern(3) + cmd(2) + payload + checksum(1), so this is the 10 bytes of framing between a notification's on-wire length and its payload length — what you add to a payload to compare it against the notification cap.

flip-dots · 2026-07-28T13:55:48Z

        _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}'",


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.

These are ruff rather than a deliberate reformat — [tool.ruff.lint] select = ["ALL"] in pyproject.toml enables COM812 (missing trailing comma), and .github/workflows/ruff.yml gates the PR on it. Any call whose lines the change touches has to gain one for CI to pass.

flip-dots · 2026-07-28T13:59:57Z

-        telemetry.
+    def _reassemble(self, cmd: bytes, payload: bytes) -> bytes | None:
+        """Reassemble a possibly-fragmented session frame into one payload.
+


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.

Taking your opening more or less verbatim: some Anker devices split a payload across multiple packets, and this combines fragmented payloads or passes non-fragmented ones through before further processing or decryption.

Keeping one detail from the original: it runs before the cipher, which is what lets telemetry and unknown session frames share one reassembler whether the device is GCM or CBC.

flip-dots · 2026-07-28T14:00:51Z

+            ``None`` while fragments are still outstanding.
        """
+        if not payload:
+            return payload


Is this reachable?

Structurally yes, in practice no. A frame is ff09 + length(2) + pattern(3) + cmd(2) + payload + checksum(1), so a 10-byte frame parses to an empty payload and _split_packet will hand one back. But I've never seen one on any of the three devices here, and none of the four captures contains one — even the "bare status" responses (0805, 0822) carry a byte.

So it's purely a guard for payload[0] on the next line, protecting against a malformed or truncated notification rather than anything a device actually sends. Happy to drop it and let a bad frame raise if you'd rather the parser stay strict — it's the sort of defensive line that hides a real bug later.

flip-dots · 2026-07-28T14:09:11Z

+            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


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.

Reworded to: subsequent fragments of a fragmented payload are always preceded by a fragment that fills the notification cap, so a full-length notification means more is coming and a shorter one closes it. The cap is the negotiated ATT MTU minus 3, which per the other thread should come from the MTU the device declares in its stage-2 response rather than from client.mtu_size.

flip-dots · 2026-07-28T14:10:11Z

+            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


Try to stick to pre-existing terminology like fragmented, non-fragmented, etc.

Switched to fragmented / non-fragmented throughout the function, including this one.

flip-dots · 2026-07-28T14:14:02Z

+        return payload
+
+    def _join_fragments(self, cmd_key: bytes) -> bytes | None:
+        """Join a completed fragment run, or ``None`` if more fragments are due."""


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.

Using your wording: return the merged payload if all fragments are present in the buffer and reset, or return None if fragments are still missing.

flip-dots · 2026-07-28T14:21:33Z

-            if len(self._fragment_buffers[cmd_key]) < fragment_total:
-                _LOGGER.debug("Waiting for remaining fragments...")
-                return
+    async def _process_telemetry_packet(


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.

Moving it to device.py. The override only exists because Prime devices pack telemetry into a single frame, which the shared length gate now handles for every family, so there is nothing device-specific left in it.

The exception handling goes with it — it was catching a partial fragment that could not decrypt, and once the gate decides fragmentation by length rather than by trusting payload[0], that case stops arising.

Pushed in a128052, but I was wrong about half of this and want to correct it rather than leave it standing.

The override is gone — it only differed from the base by the guard around decryption, so there is now one implementation in device.py.

The exception handling could not go, though. I removed it, and four of your existing tests failed: solix_packet_1_missing, solix_both_packets_reversed, solix_both_packets_later_out_of_order all feed deliberately broken fragment sequences and assert device._data stays None, and without the guard the CBC decrypt raises Data must be padded to 16 byte boundary straight into the notification callback.

I then tried to make it unnecessary by having _reassemble drop a fragment that arrives with nothing to join it to, which is the more correct fix. That broke four different tests — prime_telemetry_packet, prime_power_bank_telemetry_packet and both MagGo cases. Those devices send non-fragmented payloads with no fragment header, so a ciphertext first byte that happens to read as a valid <index><total> is indistinguishable from a genuine stray fragment. Dropping one throws away real telemetry.

So the two cases can only be told apart by whether the payload decrypts, which means something has to absorb the failure. I have kept it where it was and rewritten the comment to say that, instead of implying it is defensive padding.

Worth noting you have solved this properly in #61 by wrapping the whole of _process_notification in a try/except — that is the right shape, and with it in place this guard could come out. It felt like scope creep to add the same thing here, but say the word and I will.

flip-dots · 2026-07-28T14:25:43Z


+                # Reassemble multi-fragment frames before the cipher, so telemetry
+                # and unknown session frames share one reassembler (SolixBLE #42).
+                payload = self._reassemble(cmd, payload)


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?

You are right. It currently sits inside the session-message branch, after the futures check, so a _listen_for_packet caller gets handed the first fragment rather than the whole payload — which is exactly the bug in get_status_update() on the C1000/C300/C800, where the code listens twice and stitches the two packets together by hand.

Moving it above the futures dispatch. I see #61 already puts it there and drops those manual two-packet stitches as a result.

flip-dots · 2026-07-28T17:14:24Z

            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


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.

Agreed, and that F2000 report is more useful than a second test case — it exposes that neither the current approach here nor a fixed constant is right.

client.mtu_size is unreliable on BlueZ. bleak's backend returns a hardcoded 23 unless _acquire_mtu() has been called, with UserWarning: Using default MTU value. We hit this live on a Pi Zero 2W. Reassembly still works, because the comparison is >= and a real 253-byte fragment clears a threshold of 20 — but the guard silently stops doing its actual job, which is keeping a short frame whose first ciphertext byte happens to look like 0x1x from opening a run that never completes. So on Linux, the platform HA runs on, it's effectively inert.

The device solves this for us: it declares its own MTU in the stage-2 response (0803/4803), field a2 — 253 (fd00) on our A1783 / A91B2 / A2345, and 297 (2901) on a 160W, which lines up with the 247-MTU F2000 being a different number again. That value agrees with ATT_MTU - 3 in every capture we have, and reading it needs no _acquire_mtu() call on any platform.

So I'll change this to prefer the declared a2 and fall back to mtu_size - 3, and parametrize the tests over 247 / 253 / 297 rather than just emulating 256.

Worth noting the same field would fix the _MAX_PACKET_SIZE = 253 constant in #61 — at 247 the cap is 244, so a == 253 test never fires and fragments get treated as complete payloads.

…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>

kb1ibt · 2026-08-11T06:28:37Z

Apologies for the delay — I was at Pennsic for the past 2 weeks with minimal connectivity, so I wasn't able to provide any responses to your review.

The branch is rebased onto current main (it had fallen 22 commits behind) and green — 113 tests, Read the Docs passing. Taking your points in order:

Test parametrization — agreed, converting test_reassembly.py.
Comments that only make sense inside this PR (device.py 60 and 564) and the "run" terminology (573, 589, 596) — all fair. Rewriting them to stand on their own and sticking to fragmented / non-fragmented throughout. Your suggested opening for the _reassemble docstring says what it is before how it works, which mine didn't; I'll use that shape.
Unrelated formatting churn — acknowledged, and sorry for the extra review load. It's the linter reformatting lines adjacent to the ones I touched. I'll keep it out of the next push where I can.
Moving the _process_telemetry_packet override out of prime_device.py — agreed on both counts; device.py is the right home and the exception handling there isn't earning its keep.
Reassembler placement — you're right, and I notice you've already done exactly this in Experiment with using construct for packet parsing #61: reassembling before the futures dispatch so _listen_for_packet receives whole payloads rather than a first fragment. Mine currently sits after the futures check, which is wrong for the same reason. Moving it.
MTU sizes — replying inline, because the F2000 report you linked points at something better than what either of us is doing.

One thing worth flagging given the timing: #57 landed while I was away, so the keep-alive machinery is yours now and the _KEEPALIVE_CMD approach in #51 is superseded. I'll rework that PR onto your _keep_alive() contract rather than competing with it.

Addresses review feedback on flip-dots#48. The fragmentation threshold came from client.mtu_size, which bleak's BlueZ backend reports as a hardcoded 23 unless _acquire_mtu() has been called. That collapsed the threshold to 20, leaving the guard nominally working (the comparison is >=, so real fragments still pass) but unable to do its actual job of keeping a short payload with a plausible-looking header from beginning a fragmented payload that never completes. Linux is where Home Assistant runs, so the guard was inert on the platform that matters most. Devices declare the size themselves as a2 of the stage-2 negotiation response -- 253 on the A1783/A91B2/A2345, 297 on the Prime 160W -- which agrees with ATT_MTU - 3 everywhere it has been observed and needs no _acquire_mtu() call on any platform. Record it there and prefer it, falling back to mtu_size - 3 for devices that omit the field. Reassembly also moves above the packet-future dispatch, so anything waiting on a packet receives a whole payload rather than its first fragment. get_status_update() on the C1000, C300, C800 and F2600 consequently listens once instead of twice and drops its hand-rolled two-packet stitch. Also folds the Prime _process_telemetry_packet() override into the base (it only differed by the guard around decryption), rewrites the comments and docstrings to stand on their own, and switches to fragmented/non-fragmented terminology. Tests are parametrized over the three declared sizes seen in the wild (247 per flip-dots#55, 253, 297), and cover the declared size winning over a BlueZ-default mtu_size and the fallback when nothing is declared. 125 pass.

mypy --strict flagged the reassembly result being assigned straight back into a variable annotated bytes, since _reassemble returns bytes | None. Bind it separately and return early, so the type narrows properly instead of leaking None into everything downstream that indexes the payload.

flip-dots reviewed Jul 28, 2026

View reviewed changes

kb1ibt force-pushed the pr/reassembly branch from b65a401 to 1d2459d Compare August 11, 2026 05:00

kb1ibt mentioned this pull request Aug 11, 2026

Experiment with using construct for packet parsing #61

Draft

kb1ibt added 2 commits August 11, 2026 03:20

		return dev


		def test_short_single_without_frag_byte_kept_whole() -> None:

Uh oh!

Conversation

kb1ibt commented Jul 20, 2026

Uh oh!

flip-dots left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kb1ibt commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants