Fix wrong AMS unit displayed with dual AMS on P2S (#420)

P2S firmware sends local slot IDs (0-3) in tray_now, not global tray IDs, when multiple AMS units are connected. The UI highlighted the wrong AMS unit (e.g., AMS-A instead of AMS-B). Usage tracking was unaffected because it uses the MQTT mapping field. Added disambiguation in _handle_ams_data for single-nozzle printers with ams_exist_bits indicating >1 AMS: cross-references tray_now with the MQTT mapping field (snow-encoded) to resolve the correct global tray ID. Falls back to raw value when no mapping is available or ambiguous. No behavior change for single-AMS printers or dual-nozzle (H2D) printers.
2026-10-09 07:25:44 +02:00 · 2026-02-21 11:07:24 +01:00
parent 16fa84bc16
commit 7c5741b23c
4 changed files with 281 additions and 3 deletions
@@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file.
 ## [0.2.1b2] - Unreleased

 ### Fixed
+- **Wrong AMS Unit Displayed With Dual AMS on P2S** ([#420](https://github.com/maziggy/bambuddy/issues/420)) — On P2S printers with two AMS units, the UI highlighted the wrong AMS when printing from the second unit (e.g., printing from AMS-B slot 2 but AMS-A slot 2 was shown as active). The P2S firmware sends local slot IDs (0-3) in `tray_now`, not global tray IDs — contrary to the previous assumption that all single-nozzle printers report global IDs. Filament usage tracking was unaffected because it uses the MQTT `mapping` field (snow-encoded with correct AMS hardware IDs). The display now cross-references `tray_now` with the MQTT mapping field to resolve the correct AMS unit when multiple AMS units are detected via `ams_exist_bits`. Falls back to the raw value when no mapping is available (e.g., manual filament load outside of a print) or when the mapping is ambiguous.
 - **PCTG Filament Misidentified as PC** ([#478](https://github.com/maziggy/bambuddy/issues/478)) — Selecting "Generic PCTG" as a filament profile defaulted to PC material. The spool form's material parser listed PC before PCTG and used substring matching (`indexOf`), so "PCTG" matched "PC" first. The AMS slot configuration and local profiles views were also missing PCTG from their known material types. Additionally, the temperature range logic used `includes('PC')` which matched PCTG and assigned PC temperatures (260-300°C) instead of PETG-range temperatures (220-260°C). Fixed by reordering PCTG before PC in the spool form parser, adding PCTG to all material type arrays, and adding an exact-match temperature case for PCTG.
 - **Phantom Prints From Lingering SD Card Files** ([#477](https://github.com/maziggy/bambuddy/issues/477)) — Prints could restart without user input hours after completing, because uploaded gcode files survived on the printer's SD card and were auto-started on firmware restart. Three bugs allowed files to linger. First, the post-print SD card cleanup retry loop always broke after the first attempt regardless of success, because `delete_file_async` catches errors internally and returns `False` instead of raising — the `except` retry branch never executed. Fixed by only breaking on successful delete and retrying with a 2-second delay on failure. Second, when `start_print()` failed after uploading a file (in both the background dispatcher and print scheduler), the uploaded file was never cleaned up since `on_print_complete` never fires for a print that never started. Now deletes the uploaded file on a best-effort basis when `start_print()` returns `False`. Third, cleanup failure logging was at `DEBUG` level, making failures invisible in normal operation — escalated to `WARNING`.
 - **Non-Actionable HMS Errors Triggering Notifications** ([#470](https://github.com/maziggy/bambuddy/issues/470)) — Infrastructure and auth-related HMS error codes (like `0500_0007` "MQTT command verification failed") were triggering printer error notifications even though they don't indicate actual print problems. For example, a device with incorrect bind settings sending unauthorized MQTT commands caused repeated false-alarm nozzle/extruder error notifications with camera snapshots of perfectly fine prints. Now suppresses notifications for known non-actionable error codes: `0500_0007` (MQTT auth failure), `0500_4001` (Bambu Cloud connection failure), and `0500_400E` (print cancelled by user).
@@ -26,6 +27,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **Include Beta Updates Setting** — New toggle in Settings → Updates to opt in to beta/prerelease update notifications. Default: off (stable only). The update checker now fetches `/releases` instead of `/releases/latest` and filters by `parse_version()` prerelease detection (not GitHub's `prerelease` flag, which may not be set correctly). Users on the Docker `latest` tag will no longer see notifications for beta releases they can't install.

 ### Improved
+- **P2S Dual-AMS tray_now Test Coverage** — Added 14 integration tests for multi-AMS tray_now disambiguation on single-nozzle printers (resolving AMS-B slots via mapping field, AMS-A passthrough, multi-color mapping, ambiguous/missing mapping fallbacks, last_loaded_tray tracking). Added 9 unit tests for `_resolve_local_slot_from_mapping` (snow decoding, unmapped entry filtering, ambiguity detection, AMS-HT slot matching). All 66 tray_now-related tests pass.
 - **Bulk Spool & Stock Test Coverage** — Added 13 backend unit tests covering `SpoolBulkCreate` schema validation (quantity bounds, field preservation, stock vs configured distinction) and bulk endpoint logic (correct spool count, single quantity, identical fields). Added 10 frontend tests covering `validateForm` with `quickAdd` flag (6 tests for relaxed vs full validation) and `SpoolFormModal` UI behavior (4 tests for quick-add toggle visibility, PA Profile tab hiding, quantity field rendering).
 - **Filament Cost Tracking Test Coverage** — Added 2 backend unit tests for archive cost aggregation (zero-cost guard preserves existing costs, positive-cost updates archive correctly). Added 2 frontend unit tests for spool form cost_per_kg persistence. Fixed missing `archive_id` database migration, SQLAlchemy `is None` → `.is_(None)` in where clauses, duplicate archive cost write, and unconditional zero-cost overwrite.
 - **Spool Assignment Snapshot Test Coverage** — Added 7 backend unit tests covering spool assignment snapshotting at print start, snapshot-preferred spool lookup in both 3MF and AMS delta paths, fallback to live query for pre-upgrade sessions, and the core mid-print unlink scenario from #459.
@@ -875,6 +875,34 @@ class BambuMQTTClient:
        if "filament_tangle_detect" in xcam_data:
            self.state.print_options.filament_tangle_detect = bool(xcam_data.get("filament_tangle_detect"))

+    @staticmethod
+    def _resolve_local_slot_from_mapping(local_slot: int, mapping_raw: list | None) -> int | None:
+        """Resolve a local AMS slot ID to a global tray ID using the MQTT mapping field.
+
+        The MQTT mapping field is an array of snow-encoded values:
+        each entry = ams_hw_id * 256 + slot_id (65535 = unmapped).
+
+        Finds entries where the local slot matches, then computes the global tray ID.
+        Returns the global ID if exactly one AMS matches, or None if ambiguous/unavailable.
+        """
+        if not isinstance(mapping_raw, list) or not mapping_raw:
+            return None
+
+        candidates: set[int] = set()
+        for value in mapping_raw:
+            if not isinstance(value, int) or value >= 65535:
+                continue
+            ams_hw_id = value >> 8
+            slot = value & 0xFF
+            if 0 <= ams_hw_id <= 3 and (slot & 0x03) == local_slot:
+                candidates.add(ams_hw_id * 4 + local_slot)
+            elif 128 <= ams_hw_id <= 135 and local_slot == 0:
+                candidates.add(ams_hw_id)
+
+        if len(candidates) == 1:
+            return candidates.pop()
+        return None
+
    def _handle_ams_data(self, ams_data):
        """Handle AMS data changes for Spoolman integration.

@@ -928,7 +956,8 @@ class BambuMQTTClient:

                # H2D dual-nozzle printers report only slot number (0-3), not global tray ID
                # Use active_extruder + ams_extruder_map to determine which AMS the slot belongs to
-                # Single-nozzle printers (X1C, P2S, etc.) always report global IDs, even with multiple AMS
+                # Single-nozzle printers with multiple AMS (e.g. P2S) also report local slot IDs (#420)
+                # — disambiguated below using MQTT mapping field
                ams_map = self.state.ams_extruder_map
                if self._is_dual_nozzle and 0 <= parsed_tray_now <= 3:
                    # First, check if we have a pending target that matches this slot
@@ -1050,6 +1079,37 @@ class BambuMQTTClient:
                                    f"using slot {parsed_tray_now}"
                                )
                                self.state.tray_now = parsed_tray_now
+                elif not self._is_dual_nozzle and 0 <= parsed_tray_now <= 3:
+                    # Single-nozzle printer with tray_now in 0-3 range.
+                    # P2S (and possibly other models) with multiple AMS units sends LOCAL slot IDs
+                    # in tray_now, not global tray IDs (#420). Use the MQTT mapping field
+                    # (snow-encoded) to resolve the correct AMS unit.
+                    ams_exist_raw = ams_data.get("ams_exist_bits", "0")
+                    try:
+                        ams_exist = int(ams_exist_raw, 16) if isinstance(ams_exist_raw, str) else int(ams_exist_raw)
+                    except (ValueError, TypeError):
+                        ams_exist = 0
+                    num_ams = bin(ams_exist).count("1")
+
+                    if num_ams > 1:
+                        # Multiple AMS on single-nozzle — tray_now is likely a local slot ID.
+                        # Cross-reference with MQTT mapping field to find the correct AMS unit.
+                        mapping_raw = self.state.raw_data.get("mapping")
+                        resolved = self._resolve_local_slot_from_mapping(parsed_tray_now, mapping_raw)
+                        if resolved is not None:
+                            if resolved != parsed_tray_now:
+                                logger.debug(
+                                    f"[{self.serial_number}] Multi-AMS tray_now: "
+                                    f"local slot {parsed_tray_now} -> global ID {resolved} (from mapping)"
+                                )
+                            self.state.tray_now = resolved
+                        else:
+                            # No mapping available (not printing, or ambiguous) — use as-is.
+                            # This matches the old behavior and is correct for AMS 0.
+                            self.state.tray_now = parsed_tray_now
+                    else:
+                        # Single AMS — local slot 0-3 equals global ID
+                        self.state.tray_now = parsed_tray_now
                else:
                    # tray_now > 3 means it's already a global ID, or 255 means unloaded
                    # Note: Do NOT clear pending_tray_target on tray_now=255 here.
@@ -1212,13 +1212,15 @@ class TestRequestTopicAmsMapping:
 # ---------------------------------------------------------------------------


-def _ams_payload(tray_now, ams_units=None, tray_exist_bits=None):
+def _ams_payload(tray_now, ams_units=None, tray_exist_bits=None, ams_exist_bits=None):
    """Build minimal print.ams payload for tray_now disambiguation tests."""
    ams = {"tray_now": str(tray_now)}
    if ams_units is not None:
        ams["ams"] = ams_units
    if tray_exist_bits is not None:
        ams["tray_exist_bits"] = tray_exist_bits
+    if ams_exist_bits is not None:
+        ams["ams_exist_bits"] = ams_exist_bits
    return {"print": {"ams": ams}}


@@ -1301,7 +1303,7 @@ class TestTrayNowSingleNozzleX1E:


 class TestTrayNowSingleNozzleP2S:
-    """Single-nozzle, 2 AMS — global IDs 4-7 for AMS 1 pass through directly."""
+    """Single-nozzle, 2 AMS — tray_now > 3 passes through as global ID."""

    @pytest.fixture
    def mqtt_client(self):
@@ -1328,6 +1330,195 @@ class TestTrayNowSingleNozzleP2S:
        assert mqtt_client.state.tray_now == 6


+# ---------------------------------------------------------------------------
+# 2b. Single-nozzle P2S — multi-AMS local slot disambiguation (#420)
+# ---------------------------------------------------------------------------
+
+
+class TestTrayNowP2SMultiAmsDisambiguation:
+    """P2S firmware sends local slot IDs (0-3) in tray_now even with dual AMS.
+
+    When ams_exist_bits indicates >1 AMS unit and tray_now is 0-3, the backend
+    should use the MQTT mapping field (snow-encoded) to resolve the correct
+    global tray ID.
+    """
+
+    @pytest.fixture
+    def mqtt_client(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        client = BambuMQTTClient(
+            ip_address="192.168.1.100",
+            serial_number="TEST_P2S_DUAL",
+            access_code="12345678",
+        )
+        return client
+
+    def test_resolves_ams1_slot1_from_mapping(self, mqtt_client):
+        """tray_now=1 with mapping=[257] → global ID 5 (AMS1-T1).
+
+        257 snow-decoded: ams_hw_id=1, slot=1 → global 1*4+1=5.
+        """
+        # Set mapping field in raw_data (as the MQTT handler would)
+        mqtt_client.state.raw_data["mapping"] = [257]
+        mqtt_client._process_message(
+            _ams_payload(1, ams_exist_bits="3")  # '3' = 0b11 → AMS 0 and 1
+        )
+        assert mqtt_client.state.tray_now == 5
+
+    def test_resolves_ams1_slot0_from_mapping(self, mqtt_client):
+        """tray_now=0 with mapping=[256] → global ID 4 (AMS1-T0).
+
+        256 snow-decoded: ams_hw_id=1, slot=0 → global 1*4+0=4.
+        """
+        mqtt_client.state.raw_data["mapping"] = [256]
+        mqtt_client._process_message(_ams_payload(0, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 4
+
+    def test_resolves_ams1_slot3_from_mapping(self, mqtt_client):
+        """tray_now=3 with mapping=[259] → global ID 7 (AMS1-T3).
+
+        259 snow-decoded: ams_hw_id=1, slot=3 → global 1*4+3=7.
+        """
+        mqtt_client.state.raw_data["mapping"] = [259]
+        mqtt_client._process_message(_ams_payload(3, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 7
+
+    def test_ams0_slot_unchanged_when_mapping_confirms_ams0(self, mqtt_client):
+        """tray_now=1 with mapping=[1] → stays 1 (AMS0-T1).
+
+        1 snow-decoded: ams_hw_id=0, slot=1 → global 0*4+1=1.
+        """
+        mqtt_client.state.raw_data["mapping"] = [1]
+        mqtt_client._process_message(_ams_payload(1, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 1
+
+    def test_multicolor_resolves_ams1_from_multi_entry_mapping(self, mqtt_client):
+        """Multi-color print: mapping=[0, 257] → tray_now=1 resolves to AMS1-T1 (5).
+
+        Entry 0: ams_hw_id=0, slot=0 (local 0) — doesn't match tray_now=1.
+        Entry 257: ams_hw_id=1, slot=1 (local 1) — matches tray_now=1 → global 5.
+        """
+        mqtt_client.state.raw_data["mapping"] = [0, 257]
+        mqtt_client._process_message(_ams_payload(1, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 5
+
+    def test_multicolor_four_slot_mapping(self, mqtt_client):
+        """mapping=[65535, 65535, 65535, 257] → tray_now=1 resolves to global 5.
+
+        Only entry 257 has local slot=1, other entries are unmapped (65535).
+        Reproduces exact data from issue #420 support package.
+        """
+        mqtt_client.state.raw_data["mapping"] = [65535, 65535, 65535, 257]
+        mqtt_client._process_message(_ams_payload(1, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 5
+
+    def test_ambiguous_mapping_falls_back_to_local_slot(self, mqtt_client):
+        """Two AMS units with same local slot in mapping → ambiguous, keep local slot.
+
+        mapping=[1, 257]: both have local slot 1 (AMS0-T1 and AMS1-T1).
+        Cannot disambiguate → fall back to tray_now=1.
+        """
+        mqtt_client.state.raw_data["mapping"] = [1, 257]
+        mqtt_client._process_message(_ams_payload(1, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 1
+
+    def test_no_mapping_falls_back_to_local_slot(self, mqtt_client):
+        """No mapping field available → fall back to raw tray_now."""
+        # No mapping in raw_data (e.g. manual filament load, not during print)
+        mqtt_client._process_message(_ams_payload(1, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 1
+
+    def test_empty_mapping_falls_back_to_local_slot(self, mqtt_client):
+        """Empty mapping list → fall back to raw tray_now."""
+        mqtt_client.state.raw_data["mapping"] = []
+        mqtt_client._process_message(_ams_payload(1, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 1
+
+    def test_single_ams_passthrough(self, mqtt_client):
+        """Single AMS (ams_exist_bits='1') → tray_now 0-3 is direct global ID."""
+        mqtt_client._process_message(_ams_payload(2, ams_exist_bits="1"))
+        assert mqtt_client.state.tray_now == 2
+
+    def test_no_ams_exist_bits_passthrough(self, mqtt_client):
+        """No ams_exist_bits in payload → fall back to raw tray_now."""
+        mqtt_client._process_message(_ams_payload(1))
+        assert mqtt_client.state.tray_now == 1
+
+    def test_tray_now_255_unaffected_by_multi_ams(self, mqtt_client):
+        """tray_now=255 (unloaded) passes through regardless of AMS count."""
+        mqtt_client.state.raw_data["mapping"] = [257]
+        mqtt_client._process_message(_ams_payload(255, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 255
+
+    def test_tray_now_above_3_unaffected(self, mqtt_client):
+        """tray_now > 3 is already a global ID and passes through directly."""
+        mqtt_client._process_message(_ams_payload(6, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 6
+
+    def test_last_loaded_tray_uses_resolved_global_id(self, mqtt_client):
+        """last_loaded_tray should reflect the resolved global ID, not local slot."""
+        mqtt_client.state.raw_data["mapping"] = [257]
+        mqtt_client.state.state = "RUNNING"
+        mqtt_client._process_message(_ams_payload(1, ams_exist_bits="3"))
+        assert mqtt_client.state.tray_now == 5
+        assert mqtt_client.state.last_loaded_tray == 5
+
+
+class TestResolveLocalSlotFromMapping:
+    """Unit tests for _resolve_local_slot_from_mapping static method."""
+
+    def test_single_match_ams0(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(1, [1]) == 1
+
+    def test_single_match_ams1(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        # 257 = 1*256 + 1 → AMS1 slot1 → global 5
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(1, [257]) == 5
+
+    def test_single_match_ams2(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        # 514 = 2*256 + 2 → AMS2 slot2 → global 10
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(2, [514]) == 10
+
+    def test_unmapped_entries_skipped(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(1, [65535, 65535, 65535, 257]) == 5
+
+    def test_no_match_returns_none(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        # mapping has slot 0 only, looking for slot 2
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(2, [0]) is None
+
+    def test_ambiguous_returns_none(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        # Both AMS0 slot1 (1) and AMS1 slot1 (257) → ambiguous
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(1, [1, 257]) is None
+
+    def test_none_mapping_returns_none(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(1, None) is None
+
+    def test_empty_mapping_returns_none(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(1, []) is None
+
+    def test_ams_ht_slot0_match(self):
+        from backend.app.services.bambu_mqtt import BambuMQTTClient
+
+        # AMS-HT id=128: snow = 128*256 + 0 = 32768
+        assert BambuMQTTClient._resolve_local_slot_from_mapping(0, [32768]) == 128
+
+
 # ---------------------------------------------------------------------------
 # 3. H2D Pro — initial state detection
 # ---------------------------------------------------------------------------
@@ -0,0 +1,25 @@
+  ┌─────────────────┬─────────────────┬────────────────┬────────────────────────────────────┬─────────────────────┐
+  │      Model      │ _is_dual_nozzle │ ams_exist_bits │             Branch hit             │      Behavior       │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ X1C/X1E (1 AMS) │ False           │ "1"            │ New elif → num_ams=1 → passthrough │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ X1C (2 AMS)     │ False           │ "3"            │ New elif → disambiguation          │ Fixed (same as P2S) │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ P1S/P1P (1 AMS) │ False           │ "1"            │ New elif → num_ams=1 → passthrough │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ A1/A1 Mini      │ False           │ "1" or missing │ New elif → num_ams≤1 → passthrough │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ P2S (1 AMS)     │ False           │ "1"            │ New elif → num_ams=1 → passthrough │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ P2S (2 AMS)     │ False           │ "3"            │ New elif → disambiguation          │ Fixed               │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ H2D/H2D Pro     │ True            │ any            │ Branch 1 (H2D logic)               │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ H2C             │ True            │ any            │ Branch 1 (H2D logic)               │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ tray_now=255    │ any             │ any            │ Branch 3 (passthrough)             │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ tray_now=254    │ any             │ any            │ Branch 3 (passthrough)             │ Unchanged           │
+  ├─────────────────┼─────────────────┼────────────────┼────────────────────────────────────┼─────────────────────┤
+  │ tray_now 4-15   │ any (False)     │ any            │ Branch 3 (passthrough)             │ Unchanged           │
+  └─────────────────┴─────────────────┴────────────────┴────────────────────────────────────┴─────────────────────┘