fix(vp): deep-merge ams on bridge cache so P1S/A1 partial pushes don't nuke AMS (#1387)

Reporter vmhomelab ran a Print Queue VP against a P1S, opened
  BambuStudio, and saw only the External Spool. Toggling Auto-Dispatch
  (which restarts the VP) made AMS briefly appear, then it reverted to
  defaults. Proxy Mode worked fine.

  The earlier #1371 sticky-keys fix only handled one of two firmware
  incremental-push shapes: it preserved cached `ams` when the incoming
  push OMITTED the key entirely. P1S firmware (01.09.01.00) instead
  sends incrementals with the `ams` key present but the inner `ams.ams`
  array stripped — `{ams_status: 1, humidity: 2}` rather than
  `{ams: [...], ams_status: 1}`. To the existing "key present? leave it"
  check that read as "no need to preserve," so the bridge cache got
  overwritten with the stripped blob, the slicer's next 1 Hz read saw
  `ams` with no unit list, and BambuStudio fell back to its "no AMS"
  default render. Toggling Auto-Dispatch restarted the VP and got a
  fresh pushall through; the next P1S incremental stripped it again.

  H2D rarely trips this because its incrementals typically don't carry
  `ams` at all, so #1371 alone was enough — which is why H2D users
  (including the project owner) didn't see the bug while P1S/A1 users do.

  Fix: deep-merge the `ams` key inside the bridge cache. Mirrors the
  structure Bambuddy itself already does in
  `bambu_mqtt.py::_handle_ams_data` — scalar fields take the new value,
  but the `ams.ams` array is merged unit-by-unit by `id`, each unit's
  `tray` array is merged tray-by-tray by `id`, and units / trays the
  incremental doesn't mention survive intact from the cached full
  state. A tray-targeted incremental during a print
  (`{ams: [{id: 0, tray: [{id: 0, state: 11}]}]}`) now updates that one
  tray's state without dropping the other trays' tray_type / tray_color.

  Helper added as `_merge_ams_dict` next to `_ip_to_uint32_le`, called
  from the existing sticky-keys block when both prev and new carry the
  `ams` key as dicts. Other sticky keys (vt_tray, net, ipcam,
  lights_report, ams_extruder_map, mapping) keep the prior absent-only
  preservation; only `ams` has the multi-shape partial problem worth
  the merge complexity.
This commit is contained in:
maziggy
2026-05-17 09:45:29 +02:00
parent 8e4f815b37
commit 1bb0d4856d
3 changed files with 304 additions and 2 deletions
+1
View File
@@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file.
## [0.2.5b1] - Unreleased
### Fixed
- **Virtual Printer (queue / immediate / review modes): AMS data flickered or disappeared in BambuStudio between pushalls on P1S/A1 targets (#1387)** — Reporter vmhomelab ran a Print Queue VP against a P1S, opened BambuStudio, and saw the External Spool only — no AMS. Toggling Auto-Dispatch (which triggers a VP restart) made AMS briefly appear, then it reverted to defaults. Proxy Mode worked fine. The earlier #1371 sticky-keys fix only handled one of two Bambu firmware incremental-push shapes: it preserved cached AMS when the incoming push *omitted* the `ams` key entirely (H2D's common incremental shape). The reporter's P1S firmware (01.09.01.00) instead sends incrementals with the `ams` key present but the inner `ams.ams` array stripped — `{ams_status: 1, humidity: 2}` instead of `{ams: [...], ams_status: 1}`. To the previous sticky-keys check that read as "key present, leave new state alone," so the bridge cache got overwritten with the stripped blob; the slicer's next 1 Hz read saw `ams` with no unit list and fell back to the "no AMS" default render. Toggling Auto-Dispatch restarted the VP and got a fresh pushall in; the next P1S incremental stripped it again. (H2D rarely hits this — its incrementals typically don't carry `ams` at all, so #1371 alone was enough there. The reporter's same-VP-architecture pinging both an H2D and a P1S would observe the H2D works while the P1S doesn't, which is exactly the split that surfaced this.) Fix is a deep-merge applied to the `ams` key inside the bridge cache, mirroring the structure Bambuddy itself already does in `bambu_mqtt.py::_handle_ams_data` (which is why Bambuddy's own AMS display stays coherent on the same firmware): scalar fields like `ams_status` and `humidity` take the new value, but the `ams.ams` array is merged unit-by-unit on `id`, each unit's `tray` array is merged tray-by-tray on `id`, and units / trays the incremental doesn't mention survive intact from the cached full state. A tray-targeted incremental during a print like `{ams: [{id: 0, tray: [{id: 0, state: 11}]}]}` now updates that one tray's state without nuking the other three trays' tray_type/tray_color. Helper added as `_merge_ams_dict` in `backend/app/services/virtual_printer/mqtt_bridge.py` next to `_ip_to_uint32_le`, called from the existing sticky-keys block. Three new regression tests under `TestPushStatusCache` in `backend/tests/unit/test_vp_mqtt_bridge.py` cover the status-only partial (the reporter's exact reproduction), the multi-AMS unit-level merge, and the multi-tray merge. The existing `test_incoming_ams_update_replaces_cached_ams` still passes — fresh full updates still take effect, the merge only protects the cache from stripped incrementals. 32 tests total in that file, all green. Verified the cross-subnet topology from the report (printer / Bambuddy / slicer each on a different /24) is incidental: the symptom is the same regardless of subnet once the partial-shape arrives; the latency just makes the "empty cache when slicer first connects" race more visible. ProxyMode is unaffected because Proxy is raw byte-forwarding rather than a cached-as-base mirror — it never had this class of bug.
- **Quick Stats showed Filament Cost = 0 and empty Time Accuracy on pre-upgrade data after the 0.2.4.1 stats rewrite (#1390)** — Reporter IndividualGhost1905 upgraded to 0.2.4.1 (which shipped the per-event aggregation rewrite from #1378) and saw the Stats page split between consistent values (Total Prints / Print Time / Filament Used / Energy / Success Rate matched the archive list) and zero-or-empty ones (Filament Cost, Time Accuracy). Inconsistency was a migration gap: #1378 added six columns to `print_log_entries` — `archive_id`, `cost`, `energy_kwh`, `energy_cost`, `failure_reason`, `created_by_id` — but **didn't backfill any of them**. So every pre-upgrade log entry kept NULL on all six. The new Quick Stats query sums `PrintLogEntry.cost` (gets 0 for legacy data); the time-accuracy query joins `PrintArchive ON archive_id` (drops every legacy run from the average). Counts and per-row fields that already existed pre-#1378 (`status`, `duration_seconds`, `filament_used_grams`) kept working — which is why some panels looked right and others didn't. Fix is a two-step backfill in `run_migrations` next to the existing column-add block (DML, runs inside `begin_nested()` not `_safe_execute` since the latter is documented "DDL only"): step 1 links each orphan log entry to its archive via `print_name + printer_id` (highest archive `id` wins on tiebreak — newest matches the overwrite-then-stop shape that pre-#1378 reprints left behind); step 2 copies `archive.cost / energy_kwh / energy_cost` onto the latest matching log entry per archive, **but only for archives where no log entry yet carries a cost**. That second clause is the idempotency anchor and also the double-count guard for users running this migration after #1378 has already written cost-bearing rows for new runs — those archives are left untouched. Earlier reprints stay NULL, matching the "first/latest writes, rest stay NULL" convention #1378 introduced. Sum across the legacy reprint chain reproduces sum-of-archive-cost exactly, so the Quick Stats Filament Cost column matches the pre-upgrade total instead of dropping to zero. SQL is plain ANSI — correlated UPDATE with `LIMIT 1` in the SET subquery, `WHERE id IN (SELECT MAX(id) ... GROUP BY archive_id HAVING SUM(CASE WHEN cost IS NOT NULL THEN 1 ELSE 0 END) = 0)` — verified end-to-end on both SQLite (4 unit tests in `test_print_log_backfill_migration.py`) and `postgres:16-alpine + asyncpg` (live container reproduction). For the other widgets the reporter listed (Printer Stats, Filament Trends, By Material, Success by Material, Color Distribution) — those still iterate the archives list on the frontend rather than calling /stats, so they read consistent pre-upgrade data and aren't part of this fix; the inconsistency the reporter saw between Quick Stats and those widgets resolves itself once the backfill brings Quick Stats in line.
- **Spoolman: spool "Color Name" edits silently never saved — Bambuddy was writing to a field Spoolman doesn't have (#1357)** — Reporter pgladel edited a spool's Color Name in Spoolman mode, hit Save, and saw the value snap back to the subtype on the next read. Martin shipped #1319 in May to handle "form round-trips the synth value back as if it were user input" — that fix's read/form-prefill half was correct (the `color_name_is_synthesized` flag, the blank-on-synth form init), but the **write half assumed Spoolman has a `color_name` field on Filament**. It doesn't. Verified against the live `FilamentUpdateParameters` schema on Spoolman 0.23.1: `name`, `vendor_id`, `material`, `price`, `density`, `diameter`, `weight`, `spool_weight`, `article_number`, `comment`, `settings_extruder_temp`, `settings_bed_temp`, `color_hex`, `multi_color_hexes`, `multi_color_direction`, `external_id`, `extra` — that's the lot. No `color_name`. Spoolman's PATCH happily returns 200 for `{"color_name": "Red"}` and just **silently discards the unknown key**. So `find_or_create_filament` was either patching a void or creating filament after filament with the same field-that-doesn't-stick (which is what produced the reporter's "BB also created a bunch of new filaments" trail of duplicates on each save attempt). The fix takes the same route as the existing BambuStudio slicer-preset storage: persist color_name on `spool.extra.bambu_color_name` as a JSON-encoded string, register the extra field via `ensure_extra_field` before write (Spoolman 400s on unknown extra keys), and read it back in `_map_spoolman_spool` with priority `spool.extra.bambu_color_name → filament.color_name (forward-compat for any future Spoolman release that adds it) → subtype synth`. Also dropped the now-dead `color_name` passing through `find_or_create_filament` and `create_filament` — Spoolman would discard it anyway and keeping the dead pipe risked the same confusion the next time someone reads this code. The previous "match by name then patch color_name" loop is gone; what survives is the name-match resilience added earlier this turn so an AMS-sync-created filament named `"Glow"` still matches the user-driven edit's composed `"PLA Glow"`, which prevents the duplicate-filament trail. The frontend form's `color_name_is_synthesized` handling is unchanged — that part already worked. Tests rewritten across the three affected suites (`test_spoolman_inventory_methods.py`, `test_spoolman_inventory_helpers.py`, `test_spoolman_inventory_api.py`) to pin the new contract: filament patch never carries `color_name`, route writes to `bambu_color_name` extra, read prefers extra over filament-field over synth. Verified end-to-end against the live Spoolman instance at the reporter's setup (PATCH /filament with color_name → field absent from response; PATCH /spool with extra.bambu_color_name → field present in response).
- **Add Smart Plug (HA mode) — search dropdown let users pick entities the schema would reject, surfacing as a cryptic regex error on Save (#1388)** — Reporter MartinNYHC opened the Add Smart Plug dialog, typed a search prefix matching a multi-entity HA device (a Shelly-style outlet exposing one `switch.*` and several `sensor.*` / `binary_sensor.*` siblings under the same friendly-name prefix), clicked one of the entities, filled in the optional power/energy sensors, and clicked Save. The backend returned 422 with the raw Pydantic message `String should match pattern '^(switch|light|input_boolean|script)\.[a-z0-9_]+$'`. After the dropdown closed and the search cleared, the entity-list refetch (with no search param) returned the default-domain-filtered list — which didn't include the user's pick — so `selectedEntity = haEntities.find(...)` was undefined, the field rendered as visually empty (placeholder shown), but `haEntityId` still held the bad value the user had selected. Root cause was at `backend/app/services/homeassistant.py::list_entities`: when a search query was present, the function bypassed the domain filter entirely and returned matches across every HA domain — including ones the `SmartPlugBase.ha_entity_id` regex at `backend/app/schemas/smart_plug.py:17` could never accept. Offering a clickable choice the user can't save is broken UX; the fact that the error message then said `switch|light|input_boolean|script` made it look like a schema problem rather than a search-permissiveness problem. Fix: the allowed-domains filter (`{"switch", "light", "input_boolean", "script"}`, kept in sync with the schema regex) now always runs, and search composes on top of it as an additional substring match against `entity_id` or `friendly_name`. Whitespace-only search strings are treated as no search. Verified the smart-plug code path is unchanged between 0.2.4 and 0.2.4.1 — this bug was latent since the script-domain commit in February 2026 and was only noticed now because the reporter hadn't reopened the modal in months. 5 new regression tests in `backend/tests/unit/services/test_homeassistant_list_entities.py` cover the no-search baseline, the search-still-domain-filters case (the actual #1388 reproduction), the entity_id-or-friendly_name substring match, case-insensitivity, and the whitespace-only edge case.
@@ -76,6 +76,110 @@ def _ip_to_uint32_le(ip_str: str) -> int:
return parts[0] | (parts[1] << 8) | (parts[2] << 16) | (parts[3] << 24)
def _merge_ams_dict(prev_ams: dict, new_ams: dict) -> dict:
"""Merge a new ``ams`` blob from an incremental push onto the previous one.
Bambu firmware sends three shapes for the ``ams`` field on push_status:
1. Full pushall (after a printer reconnect or explicit pushall request):
``{ams: [{id, tray: [{id, tray_type, ...}, ...]}, ...], ams_status, ams_exist_bits, ...}``
— every unit + every tray populated.
2. Status-only incremental: ``{ams_status: 1}`` or ``{humidity: 30}`` —
no ``ams`` array at all. Bambuddy logs these as "AMS partial update
(no tray data)" (#784 vintage).
3. Tray-targeted incremental during a print: ``{ams: [{id: 0, tray:
[{id: 0, state: 11}]}]}`` — only the units / trays whose state
changed.
Replacing the cached ``ams`` wholesale on shapes (2) and (3) is what
made the slicer "lose" AMS between pushalls and trip the symptom in
#1387: the slicer would see a stripped ``ams_status``-only blob and
fall back to its "no AMS" default render. This merge mirrors the
deep-merge logic in ``bambu_mqtt.py::_handle_ams_data`` at the bridge
layer so the slicer-facing cache always carries the latest known
coherent state.
Strategy:
- Shallow-merge top-level scalars: keys in ``new`` win; keys only
in ``prev`` are preserved.
- For the ``ams`` array (list of units): match by ``id``. Units
only in ``prev`` survive. Units in ``new`` overlay onto their
``prev`` counterpart; same recursion applies to each unit's
``tray`` array by tray ``id``.
"""
merged = dict(prev_ams)
for k, v in new_ams.items():
if k != "ams":
merged[k] = v
prev_units = prev_ams.get("ams") if isinstance(prev_ams.get("ams"), list) else []
new_units = new_ams.get("ams") if isinstance(new_ams.get("ams"), list) else None
if new_units is None:
# Shape (2): no ``ams`` array in the incremental — keep prev's units.
if prev_units:
merged["ams"] = prev_units
return merged
prev_by_id = {u.get("id"): u for u in prev_units if isinstance(u, dict) and u.get("id") is not None}
merged_units: list = []
seen_ids: set = set()
for new_unit in new_units:
if not isinstance(new_unit, dict):
merged_units.append(new_unit)
continue
uid = new_unit.get("id")
prev_unit = prev_by_id.get(uid) if uid is not None else None
if prev_unit is None:
merged_units.append(new_unit)
if uid is not None:
seen_ids.add(uid)
continue
# Shallow-merge unit fields; preserve prev's trays not present in new.
merged_unit = dict(prev_unit)
for k, v in new_unit.items():
if k != "tray":
merged_unit[k] = v
new_trays = new_unit.get("tray") if isinstance(new_unit.get("tray"), list) else None
if new_trays is None:
# Unit-level partial — keep prev's tray list intact.
pass
else:
prev_trays = prev_unit.get("tray") if isinstance(prev_unit.get("tray"), list) else []
prev_trays_by_id = {t.get("id"): t for t in prev_trays if isinstance(t, dict) and t.get("id") is not None}
merged_trays: list = []
seen_tray_ids: set = set()
for new_tray in new_trays:
if not isinstance(new_tray, dict):
merged_trays.append(new_tray)
continue
tid = new_tray.get("id")
prev_tray = prev_trays_by_id.get(tid) if tid is not None else None
if prev_tray is None:
merged_trays.append(new_tray)
else:
merged_tray = dict(prev_tray)
merged_tray.update(new_tray)
merged_trays.append(merged_tray)
if tid is not None:
seen_tray_ids.add(tid)
# Preserve prev trays not mentioned in the incremental.
for tid, prev_tray in prev_trays_by_id.items():
if tid not in seen_tray_ids:
merged_trays.append(prev_tray)
merged_unit["tray"] = merged_trays
merged_units.append(merged_unit)
if uid is not None:
seen_ids.add(uid)
# Preserve prev units not mentioned in the incremental.
for uid, prev_unit in prev_by_id.items():
if uid not in seen_ids:
merged_units.append(prev_unit)
merged["ams"] = merged_units
return merged
class MQTTBridge:
"""Per-VP MQTT fan-out between a real printer and slicers connected to a VP."""
@@ -296,8 +400,22 @@ class MQTTBridge:
prev = self._latest_print_state
if prev is not None:
for sticky_key in _SLICER_VISIBLE_STICKY_KEYS:
if sticky_key not in new_state and sticky_key in prev:
new_state[sticky_key] = prev[sticky_key]
if sticky_key not in new_state:
if sticky_key in prev:
new_state[sticky_key] = prev[sticky_key]
continue
# Key IS in new_state — but firmware sends partial blobs
# (status-only / tray-targeted) under the same key on
# incremental updates, which would overwrite the cached
# full blob and break the slicer's AMS render (#1387).
# For `ams` specifically the deep-merge mirrors what
# Bambuddy already does internally in `_handle_ams_data`.
if (
sticky_key == "ams"
and isinstance(new_state.get("ams"), dict)
and isinstance(prev.get("ams"), dict)
):
new_state["ams"] = _merge_ams_dict(prev["ams"], new_state["ams"])
self._latest_print_state = new_state
return
+183
View File
@@ -306,6 +306,189 @@ class TestPushStatusCache:
await bridge.stop()
@pytest.mark.asyncio
async def test_partial_ams_status_update_preserves_unit_list(self):
"""#1387: Bambu firmware also sends `ams` updates where the key is
present but the inner `ams` array is missing — e.g. just
``{ams_status: 1}`` or a humidity change. Before the deep-merge fix
the bridge would overwrite the cached AMS with this stripped blob,
the slicer would read it on the next 1 Hz push, and BambuStudio
would drop the unit list and fall back to its "no AMS" render
(only the external spool visible — the reporter's exact symptom).
Now the partial update only mutates the fields it carries; the
cached unit list survives.
"""
server = _make_server()
bridge = _make_bridge(server)
await bridge.start()
# 1. Pushall with full AMS state.
bridge._on_printer_raw(
f"device/{H2D_SERIAL}/report",
json.dumps(
{
"print": {
"command": "push_status",
"ams": {
"ams": [
{
"id": "0",
"humidity": "1",
"tray": [{"id": "0", "tray_type": "PLA", "tray_color": "FF0000FF"}],
}
],
"tray_exist_bits": "1",
"ams_status": "0",
},
}
}
).encode(),
)
await asyncio.sleep(0.01)
# 2. Partial AMS update — only `ams_status` and `humidity` changed.
# No `ams.ams` array, so prev's unit list must be preserved.
bridge._on_printer_raw(
f"device/{H2D_SERIAL}/report",
json.dumps(
{
"print": {
"command": "push_status",
"ams": {"ams_status": "1", "humidity": "2"},
}
}
).encode(),
)
await asyncio.sleep(0.01)
cached = bridge.get_latest_print_state()
# Scalar fields take the new values.
assert cached["ams"]["ams_status"] == "1"
assert cached["ams"]["humidity"] == "2"
# Unit + tray data preserved from the pushall.
assert cached["ams"]["tray_exist_bits"] == "1"
assert len(cached["ams"]["ams"]) == 1
assert cached["ams"]["ams"][0]["tray"][0]["tray_type"] == "PLA"
assert cached["ams"]["ams"][0]["tray"][0]["tray_color"] == "FF0000FF"
await bridge.stop()
@pytest.mark.asyncio
async def test_partial_ams_unit_update_preserves_other_units(self):
"""#1387: when multiple AMS units are configured (e.g. H2D with two
AMS), an incremental push during a print typically only carries the
unit / tray that changed state. Naive replacement of `ams.ams` wipes
the other unit. The bridge merges unit-by-unit by id, preserving
units the incremental doesn't mention.
"""
server = _make_server()
bridge = _make_bridge(server)
await bridge.start()
# 1. Pushall with two AMS units configured.
bridge._on_printer_raw(
f"device/{H2D_SERIAL}/report",
json.dumps(
{
"print": {
"command": "push_status",
"ams": {
"ams": [
{"id": "0", "tray": [{"id": "0", "tray_type": "PLA"}]},
{"id": "1", "tray": [{"id": "0", "tray_type": "PETG"}]},
],
"tray_exist_bits": "3",
},
}
}
).encode(),
)
await asyncio.sleep(0.01)
# 2. Tray-targeted incremental: unit 0 / tray 0 state changed.
# Unit 1 is not in the update — must survive.
bridge._on_printer_raw(
f"device/{H2D_SERIAL}/report",
json.dumps(
{
"print": {
"command": "push_status",
"ams": {"ams": [{"id": "0", "tray": [{"id": "0", "state": "11"}]}]},
}
}
).encode(),
)
await asyncio.sleep(0.01)
cached = bridge.get_latest_print_state()
units = {u["id"]: u for u in cached["ams"]["ams"]}
# Unit 0 keeps its tray_type from the pushall + picks up the new state.
assert units["0"]["tray"][0]["tray_type"] == "PLA"
assert units["0"]["tray"][0]["state"] == "11"
# Unit 1 survives the incremental.
assert "1" in units
assert units["1"]["tray"][0]["tray_type"] == "PETG"
await bridge.stop()
@pytest.mark.asyncio
async def test_partial_ams_tray_update_preserves_other_trays(self):
"""Same shape as the unit-level test but at the tray level. AMS
unit 0 has four trays; the incremental only mentions tray 0.
Trays 1-3 must survive intact."""
server = _make_server()
bridge = _make_bridge(server)
await bridge.start()
bridge._on_printer_raw(
f"device/{H2D_SERIAL}/report",
json.dumps(
{
"print": {
"command": "push_status",
"ams": {
"ams": [
{
"id": "0",
"tray": [
{"id": "0", "tray_type": "PLA", "tray_color": "FF0000FF"},
{"id": "1", "tray_type": "PETG", "tray_color": "00FF00FF"},
{"id": "2", "tray_type": "ABS", "tray_color": "0000FFFF"},
{"id": "3", "tray_type": "TPU", "tray_color": "FFFF00FF"},
],
}
],
},
}
}
).encode(),
)
await asyncio.sleep(0.01)
bridge._on_printer_raw(
f"device/{H2D_SERIAL}/report",
json.dumps(
{
"print": {
"command": "push_status",
"ams": {"ams": [{"id": "0", "tray": [{"id": "0", "state": "11"}]}]},
}
}
).encode(),
)
await asyncio.sleep(0.01)
cached = bridge.get_latest_print_state()
trays = {t["id"]: t for t in cached["ams"]["ams"][0]["tray"]}
assert trays["0"]["tray_type"] == "PLA"
assert trays["0"]["state"] == "11"
# Trays not mentioned in the incremental survive intact.
assert trays["1"]["tray_type"] == "PETG"
assert trays["2"]["tray_type"] == "ABS"
assert trays["3"]["tray_type"] == "TPU"
await bridge.stop()
@pytest.mark.asyncio
async def test_incoming_ams_update_replaces_cached_ams(self):
"""Counterpart to the #1371 fix: preservation only kicks in when the