Browse Source

fix(inventory): break the Reset-Slot deadlock on A1 Mini BMCU / P1S Standard AMS (#1322)

  The original #1322 fix widened empty-slot detection to (state == 11 OR
  tray_type != ""), which closed the configured-slot reconfig case but
  didn't help the "Reset Slot on printer screen with spool still inserted"
  flow. On these firmwares the AMS reports state=3, tray_type="" after a
  Reset Slot regardless of whether a spool is physically present, so the
  empty-detection still decided "empty", skipped MQTT, marked pending —
  and on_ams_change replay never re-fired because the AMS never reported
  any state change either.

  RosdasHH traced the path: tray_state=3 falls into the else: branch,
  slot_is_empty = not (fingerprint_type and fingerprint_type.strip()),
  fingerprint_type is "", so slot_is_empty=True, MQTT is skipped, and the
  slot stays unconfigured forever. He verified empirically that removing
  the gate makes the firmware accept the push when a spool is physically
  present.

  Drop the tray_type fallback entirely. Only state in {9, 10} (firmware's
  explicit "no spool" / "spool present but no feed") short-circuits the
  MQTT publish. Every other state — including 3 (default-idle, ambiguous)
  and missing-state (older firmwares) — attempts the publish. Bambu's
  "firmware silently drops on empty slots" behavior makes the worst case
  a no-op for a truly-empty slot, and on_ams_change replay still serves
  as the safety net for state=9/10 slots whose spools get inserted later.

  pending_config is now (slot_is_definitely_empty OR not configured) so a
  printer-offline / no-client publish failure correctly flags the
  assignment for replay instead of falsely showing "configured".
maziggy 1 week ago
parent
commit
dca05ce6b1

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 37 - 28
backend/app/api/routes/inventory.py

@@ -1270,36 +1270,40 @@ async def assign_spool(
 
 
     # 4. Auto-configure AMS slot via MQTT.
     # 4. Auto-configure AMS slot via MQTT.
     #
     #
-    # Skip the publish entirely when the target slot is empty: Bambu firmware
-    # silently drops ams_filament_setting / extrusion_cali_sel for unloaded
-    # slots (there is no filament context for the cali_idx to attach to). The
-    # SpoolAssignment row is preserved with an empty fingerprint_type, which
-    # acts as the "pending config" marker — when the spool is physically
-    # inserted later, on_ams_change re-fires the full configuration. This is
-    # the SpoolBuddy primary workflow: weigh-then-assign before insertion.
+    # Only suppress the publish when the firmware's *explicit* empty signal
+    # (state ∈ {9, 10}) is set — "no spool" / "spool present but no feed".
+    # Every other state, including state=3 (the default idle on A1 Mini BMCU /
+    # P1S Standard AMS for both loaded and unconfigured slots) and missing
+    # state (older firmwares), is treated as the user's assertion that a
+    # spool is in the slot and we attempt the MQTT push.
     #
     #
-    # Empty-detection: priority order is the firmware's explicit signals
-    # first, then a tray_type fallback for firmwares that don't use the full
-    # state enum meaningfully.
-    #   - state == 9  → empty (firmware says "no spool")
-    #   - state == 10 → empty (firmware says "spool present but no feed")
-    #   - state == 11 → loaded (firmware says "filament in extruder")
-    #   - any other state value, including 3 (A1 Mini BMCU / P1S Standard AMS
-    #     always report 3) or None (older firmwares omit the field), falls
-    #     back to tray_type: configured ⇒ loaded, empty ⇒ empty (#1322).
-    # The tray_type fallback for state==3 fixes the reported bug. The 9/10
-    # branch keeps existing behavior intact even if the MQTT relay's
-    # auto-clearing of tray_type ever fails to fire — the firmware's
-    # explicit "empty" signal stays authoritative over stale metadata.
-    if tray_state == 9 or tray_state == 10:
-        slot_is_empty = True
-    elif tray_state == 11:
-        slot_is_empty = False
-    else:
-        slot_is_empty = not (fingerprint_type and fingerprint_type.strip())
+    # The pre-existing "skip when slot looks empty" guard read state=3 +
+    # tray_type="" as "empty" and skipped MQTT. On these firmwares that
+    # combination is the post-"Reset Slot" state with the spool still
+    # physically inserted — there is NO AMS signal that distinguishes it
+    # from a truly-empty slot, so the guard created a deadlock: MQTT never
+    # fired, the AMS never reported any change (because nothing changed
+    # physically), and on_ams_change replay therefore never re-fired the
+    # config either. Reporter (#1322 follow-up by @RosdasHH) verified
+    # empirically that removing the guard makes the slot configure
+    # correctly because Bambu firmware DOES accept the push for a
+    # physically-loaded slot, even when tray_type is "" and state is 3.
+    #
+    # Trade-off for the truly-empty slot case: firmware drops the push
+    # silently (per Bambu's documented behavior), the SpoolAssignment row
+    # still has empty fingerprint_type because nothing in the assign path
+    # updates that column, and on_ams_change at main.py:1031-1054 still
+    # fires the deferred config when a spool eventually appears. So the
+    # SpoolBuddy weigh-then-assign-before-insert workflow continues to
+    # work — just without the optimization of skipping a no-op MQTT call.
+    #
+    # state ∈ {9, 10} stays as an explicit short-circuit so we don't churn
+    # a doomed MQTT push when the firmware has positively confirmed "no
+    # spool" — and to keep the on_ams_change replay path as the single
+    # source of truth for those slots.
+    slot_is_definitely_empty = tray_state == 9 or tray_state == 10
     configured = False
     configured = False
-    pending_config = slot_is_empty
-    if not slot_is_empty:
+    if not slot_is_definitely_empty:
         try:
         try:
             configured = await apply_spool_to_slot_via_mqtt(
             configured = await apply_spool_to_slot_via_mqtt(
                 db=db,
                 db=db,
@@ -1313,6 +1317,11 @@ async def assign_spool(
             )
             )
         except Exception as e:
         except Exception as e:
             logger.warning("MQTT auto-configure failed for spool %d: %s", spool.id, e)
             logger.warning("MQTT auto-configure failed for spool %d: %s", spool.id, e)
+    # pending_config is the "config not landed yet" UI marker. True when the
+    # firmware said empty, OR when MQTT couldn't actually publish (printer
+    # offline, no client, transient failure). on_ams_change replay re-fires
+    # the config in either case once the AMS reports a non-empty fingerprint.
+    pending_config = slot_is_definitely_empty or not configured
 
 
     # Return assignment with spool data
     # Return assignment with spool data
     result = await db.execute(
     result = await db.execute(

+ 56 - 36
backend/tests/integration/test_inventory_assign.py

@@ -590,21 +590,25 @@ class TestAssignSpoolLiveCaliIdx:
 
 
 
 
 class TestAssignSpoolEmptySlotPreConfig:
 class TestAssignSpoolEmptySlotPreConfig:
-    """SpoolBuddy primary workflow: weigh-then-assign before the spool is in the AMS.
-
-    Bambu firmware silently drops ams_filament_setting / extrusion_cali_sel for
-    unloaded slots — there's no filament context for the cali_idx to attach to.
-    The endpoint persists the SpoolAssignment row with an empty fingerprint_type
-    (the "pending config" marker) and skips the MQTT publish; on_ams_change
-    re-fires the full configuration when filament is later inserted.
+    """Assign path under ambiguous / explicit-empty AMS state.
+
+    Updated for the #1322 follow-up: only the firmware's *explicit* empty
+    signal (state ∈ {9, 10}) skips MQTT. Anything else — including the
+    SpoolBuddy weigh-then-assign-before-insert case where state/tray_type
+    can't tell us whether a spool is loaded — attempts MQTT. The deferred-
+    config workflow still works because on_ams_change at main.py:1031-1054
+    re-fires when an AMS push eventually reports the loaded slot.
     """
     """
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
-    async def test_empty_slot_skips_mqtt_but_persists_assignment(
+    async def test_empty_tray_type_without_state_still_fires_mqtt(
         self, async_client: AsyncClient, printer_factory, spool_factory
         self, async_client: AsyncClient, printer_factory, spool_factory
     ):
     ):
-        """Assigning to an empty slot skips MQTT and returns pending_config=True."""
+        """tray_type='' with no state field: AMS can't tell us whether a
+        spool is loaded. Trust the user's Assign click and fire MQTT —
+        firmware accepts it when a spool is physically there, drops it
+        silently otherwise (no harm)."""
         printer = await printer_factory(name="H2D")
         printer = await printer_factory(name="H2D")
         spool = await spool_factory(slicer_filament="GFL05", material="PLA")
         spool = await spool_factory(slicer_filament="GFL05", material="PLA")
 
 
@@ -612,7 +616,6 @@ class TestAssignSpoolEmptySlotPreConfig:
         mock_client.ams_set_filament_setting.return_value = True
         mock_client.ams_set_filament_setting.return_value = True
         mock_client.extrusion_cali_sel.return_value = True
         mock_client.extrusion_cali_sel.return_value = True
 
 
-        # Slot found but empty (tray_type=""): the SpoolBuddy scenario
         status = _make_mock_status(ams_data=[{"id": 2, "tray": [{"id": 3, "tray_type": ""}]}])
         status = _make_mock_status(ams_data=[{"id": 2, "tray": [{"id": 3, "tray_type": ""}]}])
 
 
         with patch("backend.app.services.printer_manager.printer_manager") as mock_pm:
         with patch("backend.app.services.printer_manager.printer_manager") as mock_pm:
@@ -625,27 +628,27 @@ class TestAssignSpoolEmptySlotPreConfig:
             )
             )
 
 
         assert response.status_code == 200
         assert response.status_code == 200
+        mock_client.ams_set_filament_setting.assert_called_once()
         body = response.json()
         body = response.json()
-        assert body["pending_config"] is True
-        assert body["configured"] is False
-        # Critical: no MQTT was published (firmware would drop it)
-        mock_client.ams_set_filament_setting.assert_not_called()
-        mock_client.extrusion_cali_sel.assert_not_called()
+        assert body["pending_config"] is False
+        assert body["configured"] is True
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
-    async def test_empty_slot_no_ams_data_skips_mqtt(self, async_client: AsyncClient, printer_factory, spool_factory):
-        """No AMS data at all (printer offline, no telemetry yet) → still pre-config."""
+    async def test_no_ams_data_with_no_client_marks_pending(
+        self, async_client: AsyncClient, printer_factory, spool_factory
+    ):
+        """No AMS data + no MQTT client (printer offline, no telemetry):
+        publish can't happen, so configured=False and pending_config=True so
+        on_ams_change replay picks it up when the printer comes online."""
         printer = await printer_factory(name="X1C")
         printer = await printer_factory(name="X1C")
         spool = await spool_factory(slicer_filament="GFL05", material="PLA")
         spool = await spool_factory(slicer_filament="GFL05", material="PLA")
 
 
-        mock_client = MagicMock()
-
-        # No AMS data — fingerprint_type stays None, treated as empty
+        # No AMS data — fingerprint_type stays None.
         status = _make_mock_status(ams_data=[])
         status = _make_mock_status(ams_data=[])
 
 
         with patch("backend.app.services.printer_manager.printer_manager") as mock_pm:
         with patch("backend.app.services.printer_manager.printer_manager") as mock_pm:
-            mock_pm.get_client.return_value = mock_client
+            mock_pm.get_client.return_value = None  # Printer offline, no MQTT client.
             mock_pm.get_status.return_value = status
             mock_pm.get_status.return_value = status
 
 
             response = await async_client.post(
             response = await async_client.post(
@@ -654,8 +657,9 @@ class TestAssignSpoolEmptySlotPreConfig:
             )
             )
 
 
         assert response.status_code == 200
         assert response.status_code == 200
-        assert response.json()["pending_config"] is True
-        mock_client.ams_set_filament_setting.assert_not_called()
+        body = response.json()
+        assert body["pending_config"] is True
+        assert body["configured"] is False
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
@@ -994,15 +998,23 @@ class TestAssignSpoolEmptyDetection:
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
-    async def test_state_missing_falls_back_to_tray_type_empty(
+    async def test_state_missing_with_empty_tray_type_still_fires_mqtt(
         self, async_client: AsyncClient, printer_factory, spool_factory
         self, async_client: AsyncClient, printer_factory, spool_factory
     ):
     ):
-        """Older firmware without state field + empty tray_type → pending."""
+        """Older firmware without state field + empty tray_type still fires MQTT.
+
+        The AMS doesn't tell us whether a spool is physically loaded in this
+        case (no state, no tray_type), so the assign click is the user's
+        assertion that a spool is there. Firmware silently drops the push on
+        a truly empty slot — no harm done, and on_ams_change replay handles
+        the deferred-config case (#1322 follow-up).
+        """
         printer = await printer_factory()
         printer = await printer_factory()
         spool = await spool_factory(slicer_filament="PFUS9ac902733670a9", material="PLA")
         spool = await spool_factory(slicer_filament="PFUS9ac902733670a9", material="PLA")
 
 
         mock_client = MagicMock()
         mock_client = MagicMock()
         mock_client.ams_set_filament_setting.return_value = True
         mock_client.ams_set_filament_setting.return_value = True
+        mock_client.extrusion_cali_sel.return_value = True
 
 
         tray_data = {"id": 3, "tray_type": "", "tray_color": ""}
         tray_data = {"id": 3, "tray_type": "", "tray_color": ""}
         status = _make_mock_status(ams_data=[{"id": 2, "tray": [tray_data]}])
         status = _make_mock_status(ams_data=[{"id": 2, "tray": [tray_data]}])
@@ -1017,10 +1029,10 @@ class TestAssignSpoolEmptyDetection:
             )
             )
 
 
         assert response.status_code == 200
         assert response.status_code == 200
-        # Legacy fallback: empty tray_type + no state → treated as empty.
-        mock_client.ams_set_filament_setting.assert_not_called()
+        mock_client.ams_set_filament_setting.assert_called_once()
         body = response.json()
         body = response.json()
-        assert body["pending_config"] is True
+        assert body["pending_config"] is False
+        assert body["configured"] is True
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
@@ -1059,19 +1071,26 @@ class TestAssignSpoolEmptyDetection:
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
-    async def test_state_never_eleven_firmware_with_empty_tray_marks_pending(
+    async def test_post_reset_slot_with_state_3_still_fires_mqtt(
         self, async_client: AsyncClient, printer_factory, spool_factory
         self, async_client: AsyncClient, printer_factory, spool_factory
     ):
     ):
-        """Same firmwares as above, but the slot is truly unconfigured
-        (tray_type=''). Neither signal points to 'loaded', so this should
-        still pending-config — the user has to configure or insert filament
-        before MQTT can fire. Pins that the disjunction didn't accidentally
-        flip empty slots into the loaded branch."""
+        """A1 Mini BMCU / P1S Standard AMS post-"Reset Slot" with spool still
+        inserted: state=3, tray_type="". The AMS gives us no signal to tell
+        this apart from a truly-empty slot. We trust the user's Assign click
+        and fire MQTT — firmware accepts the push because a spool is
+        physically there (#1322 follow-up by @RosdasHH).
+
+        Replaces the previous "marks_pending" assertion which was the bug:
+        that gate created a deadlock because the AMS would never report a
+        state change (nothing physically changed), so on_ams_change replay
+        never re-fired the deferred config either.
+        """
         printer = await printer_factory()
         printer = await printer_factory()
         spool = await spool_factory(slicer_filament="PFUS9ac902733670a9", material="PLA")
         spool = await spool_factory(slicer_filament="PFUS9ac902733670a9", material="PLA")
 
 
         mock_client = MagicMock()
         mock_client = MagicMock()
         mock_client.ams_set_filament_setting.return_value = True
         mock_client.ams_set_filament_setting.return_value = True
+        mock_client.extrusion_cali_sel.return_value = True
 
 
         tray_data = {"id": 3, "state": 3, "tray_type": "", "tray_color": "00000000", "tray_info_idx": ""}
         tray_data = {"id": 3, "state": 3, "tray_type": "", "tray_color": "00000000", "tray_info_idx": ""}
         status = _make_mock_status(ams_data=[{"id": 2, "tray": [tray_data]}])
         status = _make_mock_status(ams_data=[{"id": 2, "tray": [tray_data]}])
@@ -1086,9 +1105,10 @@ class TestAssignSpoolEmptyDetection:
             )
             )
 
 
         assert response.status_code == 200
         assert response.status_code == 200
-        mock_client.ams_set_filament_setting.assert_not_called()
+        mock_client.ams_set_filament_setting.assert_called_once()
         body = response.json()
         body = response.json()
-        assert body["pending_config"] is True
+        assert body["pending_config"] is False
+        assert body["configured"] is True
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration

Some files were not shown because too many files changed in this diff