Skip to content

Feat: add bms_cooldown_on_error option and clear raw BLE stream buffer during JK-BMS AT-command flood - #392

Open
aka-raveren wants to merge 5 commits into
fl4p:masterfrom
aka-raveren:master
Open

aka-raveren wants to merge 5 commits into
fl4p:masterfrom
aka-raveren:master

Conversation

@aka-raveren

@aka-raveren aka-raveren commented Jul 29, 2026

Copy link
Copy Markdown

The Problem:
Some JK BMS (Jikong) models running specific inverter-interfacing firmware versions actively broadcast non-protocol telemetry text messages starting with AT\r\n (AT-command flood) onto the shared BLE/UART bus (as referenced in issue #370).

When heavy load, solar inverter EMI, or prolonged connection sessions cause data packet fragmentation, these junk bytes accumulate exponentially inside BleakClient incoming streams. Under the original code logic, bmslib/models/jikong.py counts these bytes (discarded junk bytes) but leaves them inside self._buffer. Within a few poll cycles, this unmanaged binary noise chokes the asyncio loop queue, leading to continuous TimeoutError spikes. Worse, the Linux host Bluetooth daemon (BlueZ) eventually drops into a critical deadlock state (org.bluez.Error.InProgress / Operation already in progress), which completely freezes data fetching for all other healthy, connected BMS units (e.g., JBD).

The Solution:
This PR introduces a two-tier escalation recovery strategy controlled via options.json / config.yaml:

  1. Low-Level Junk Buffer Suppression (bmslib/models/jikong.py):
    Directly inside the if dropped: routine, we execute .clear() on self._buffer on the fly. This instantaneously vaporizes the AT\r\n text flood out of the active Python RAM before it can bottleneck the async parser. Crucially, the valid static device configuration mappings inside _resp_table (such as num_cells index mapping) are completely preserved, preventing KeyError or IndexError code crashes.

  2. First Tier: Software Isolation (bms_cooldown_on_error):
    If a sampling timeout still slips through, the main loop catches the error, forces a programmatic .close() sequence on the troubled client, and wipes the static Bleak GATT descriptors memory cache using BleakClient._gatt_cache.clear(). This isolates the faulty battery, giving its internal MCU buffer exactly one polling period of silent cooldown time to auto-reset, while allowing other adjacent healthy BMS nodes to continue reporting data to Home Assistant without a single dropped packet.

  3. Second Tier: Hardware Remediation (bt_power_cycle_on_error):
    If the software cache flush fails to bypass a severe OS-level kernel freeze, this option executes a hot power cycle of the hci0 radio controller interface utilizing hciconfig hci0 down/up hooks with proper hardware delay sleep constraints (3 seconds), safely reviving the host adapter state without restarting the daemon container.

Testing:
Tested in a real-world multi-BMS ecosystem (JK BMS + JBD BMS) running continuously for hours under volatile grid load. Programmatic cache and stream clearing successfully bypasses 3800+ junk byte/sec streams on the fly, reducing host CPU overhead and achieving unbroken long-term data collection stability in Home Assistant Core.

@fl4p

fl4p commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Thanks for digging into this, and for the detailed writeup. The problem you're describing is real — I agree that one BMS failing should not take the others down, and #391 shows the same thing. But I can't merge this as it stands. Going through it in the order the diff appears:

bmslib/bt.py — this one is a hard no. It replaces BleakClient with bluek.BleakClient unconditionally, ignoring the ble_stack option entirely. The stack is a global user setting (bleak / bumble / bluek / esphome), and this overrides it for everyone who has bluek installed. It also breaks the reporter in #391 outright: he runs ble_stack: esphome, where BleakClient is deliberately swapped for habluetooth's proxy wrapper, and this would hijack it back to a local kernel socket that cannot reach his BMS at all. When bluek is not installed it logs a warning on every single client creation. If you want bluek, set ble_stack: bluek.

jikong.py — the premise doesn't hold. Junk bytes are not left in self._buffer. feed_frames() already removes them as it resyncs: del buf[:idx] when it finds a header further in, del buf[:-keep] when there is no header at all. The discarded N junk byte(s) count you're reading is the count of bytes it already dropped. The buffer cannot grow unboundedly, so there is nothing to flush.

Worth knowing the history too: clearing the buffer is what the code used to do, and it was removed on purpose in 2.04 — "resync framing on the header instead of clearing the buffer (dropped a frame when a packet held two), fixing timeout waiting 2/3 / crc check failed after reconnect (#377, #370)". A JK notify packet often carries the tail of one frame and the head of the next, so clearing throws away a good frame. This hunk reintroduces that bug.

Also, while ...: self._buffer.pop(0) is O(n²) on a bytearray, so against the 3800 byte/s flood you mention it would cost more, not less.

main.py — both recovery tiers are dead code. I don't think either has ever run:

  • locals().get('bms_list') or globals().get('bms_list') always yields []. bms_list is a local of main(), and fn() is a nested function — it is in neither dict (and since it is never referenced directly, it isn't even captured as a closure variable). So the "soft close" iterates an empty list.
  • BleakClient._gatt_cache does not exist on bleak's client (I checked: hasattr is False; per-service caching lives in bleak_retry_connector). The hasattr guard turns it into a silent no-op.

So the soft tier logs "Soft programmatic flush completed" while having done nothing but sleep 1 s. That is worse than not having it, because the log claims a recovery happened.

The hard tier is actively dangerous. It fires on any exception from any BMS — a single transient TimeoutError, which is routine — and then downs the host controller for ~6 s. On a Home Assistant install that is HA's own Bluetooth going away too, taking every other BLE integration with it. On ble_stack: esphome or bumble it is worse than useless: bt_power() intentionally does nothing there (no local adapter is in the path), but the os.system("hciconfig ... up") still runs regardless, so it can bring up an adapter the add-on isn't even using. os.system also blocks the event loop.

On the actual symptoms, two fixes have landed since you opened this, both from #391 — worth retesting on 2.14 before adding recovery machinery:

  • 2.12: the watchdog error counter never reset during serial sampling, so it accumulated for the lifetime of the add-on. Past ~44 errors every error slept the full 60 s backoff, and at 200 the add-on stopped sampling entirely. That alone produces "JK BMS constantly unresponsive" and "everything freezes".
  • 2.14: a failed BLE subscribe ran an unbounded diagnostic GATT dump that blocked the sampling loop for minutes (30 s per unanswered characteristic read over a proxy). That is very likely the freeze you attributed to a BlueZ deadlock.

What I would take. The isolation idea, done narrowly: in serial mode all BMS share one fetch_loop, and fn() re-raises so one failing device makes every cycle an error. Making that per-BMS — so a dead device backs off on its own without stalling or aborting the healthy ones — is the right fix, needs no new options, and helps everyone. If you want to rework the PR down to that, I'll review it. Please also drop the bt.py hunk and translate the comments to English.

@fl4p

fl4p commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Thanks for digging into this — the symptom is real and #370 needed attention. I've reworked it rather than merged it, because most of the recovery machinery doesn't actually execute:

  • BleakClient._gatt_cache doesn't exist in bleak 2.x or 3.x. It's behind hasattr, so tier 1 silently does nothing and then logs Bleak GATT cache successfully cleared / Soft programmatic flush completed.
  • locals().get('bms_list') can't see bms_list — it's a local of main(), and a string lookup doesn't create a closure cell — and it isn't a global either, so active_bms is always []. BtBms also has no close(), only disconnect().
  • The hciconfig call bypasses the ble_stack early-returns in bt_power() (with bumble the adapter is owned by bumble; with esphome there's no local adapter at all), hciconfig isn't in the add-on image, and os.system(... >/dev/null 2>&1) discards the exit status so that looks like success.
  • bluetooth_adapter and a top-level adapter aren't options — adapter: is per-device — so it always falls through to a hardcoded hci0.
  • Triggering on "any BMS raised" means one JBD hiccup power-cycles the radio for every battery, which is the opposite of isolating the faulty one.

The bt.py commit has to go regardless: it swaps in bluek unconditionally, ignoring ble_stack (the commit message says it parses the option, but the diff doesn't). bluek is pip-installed in the image, so on the default ble_stack: bleak every user would silently move off BlueZ/D-Bus onto kernel sockets; with bumble it fights bumble for exclusive adapter ownership. ble_stack: bluek already does this properly via the shadow package in addon_main.sh.

The jikong iteration went the right way — stripping only leading AT\r\n is safe where the blind .clear() wasn't. But it's inert: by the time dropped is counted, feed_frames has already deleted the junk, and what's left is either the ≤3-byte straddle reserve or the header-aligned head of the next frame. The buffer also can't exceed 300 B on any return path, so the >512 branch is unreachable.

What's landing instead:

  • jikong: a check on the real invariant — len(buffer) < FRAME_SIZE after feed_frames — which logs and resyncs if it's ever violated, so a future framing bug is visible instead of silent. Plus tests, including one for the split-frame case the flush would have broken.
  • bt_power_cycle_on_error (off by default), in sampling.py next to the existing per-BMS backoff: escalates only after repeated forced reconnects of the same BMS, resets on that BMS's next good sample, skips wired and virtual BMS, rate-limited process-wide to one cycle per 10 minutes, and goes through bt_power() so the stack guards apply.

I dropped bms_cooldown_on_error: that tier already exists unconditionally — _num_errors > 20 disconnects the BMS and forces a reconnect.

For the record, my first cut of the escalation had a worse bug than anything in this PR: I keyed the counter on the forced-disconnect branch, which requires is_connected, so on a host that never completes a connect — the exact case the option is for — it never fired at all. Caught in review, fixed, and pinned with a test.

If you can still reproduce the BlueZ InProgress deadlock, a log around the failure would be genuinely useful on #370.

fl4p added a commit that referenced this pull request Aug 18, 2026
… junk (#392)

A proposed fix for the JK-PB 'AT\r\n' flood flushed self._buffer whenever
feed_frames reported dropped bytes. But the junk is already deleted at the
moment it is counted, so what is left buffered is the header-aligned head of
the frame *behind* the junk - and dropped>0 together with a partial frame is
exactly what an interleaved flood produces. Flushing there turns a recoverable
split frame (#377) into a lost one, and the following tail then counts as junk
too, so it repeats.

Check the real invariant instead: feed_frames consumes or trims on every
iteration, so it cannot return with a whole frame still buffered. If it ever
does, log it and resync rather than letting the buffer grow silently.
fl4p added a commit that referenced this pull request Aug 18, 2026
…ts (#392)

Recovery for a host stack that wedges and answers every connect with
"Operation already in progress". Lives next to the existing per-BMS backoff
rather than in main.py's serial fetch loop, which saw only "some BMS raised"
and so cycled the radio for a single hiccup on any one of them.

It escalates only after repeated error-forced reconnects of the same BMS have
failed to recover it, resets on that BMS's next good sample, skips wired and
virtual BMS, and is rate-limited process-wide because all samplers share the
controller. Goes through bt_power() rather than hciconfig so the ble_stack
guards apply: with bumble the adapter is owned by bumble, and with esphome
there is no local adapter to cycle. Off by default - it drops every BLE
connection, including Home Assistant's own.
fl4p added a commit that referenced this pull request Aug 18, 2026
… BLE-only (#392)

Three defects found reviewing 120a513:

The escalation counter only advanced inside the `bms.is_connected` forced-
disconnect branch, so it never advanced in the case the option exists for: a
host answering every connect with "Operation already in progress" never gets the
link up, so is_connected stays False and the count sat at zero forever. 50
consecutive failures produced no cycle. Count the saturation round whether or
not there was a link to drop. The old tests missed this because they forced
is_connected=True on a fake; the real property is read-only.

Being cancelled between power-off and power-on left the radio down for good, and
main.py cancels the pending fetch loops whenever one returns. Hand the power-on
to a task that outlives the cancellation.

Escalation also fired for any exception the generic handler caught, including
failures raised after fetch() had already returned a sample - a downstream sink
bug would cycle a perfectly healthy radio. Require a BLE-ish error.

Also: bt_power() swallows bluetoothctl failures internally, so the old "power
cycle done" claimed more than was verified; and a rate-limited skip consumed the
per-BMS history, making the BMS earn its way back from zero while still wedged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants