Просмотр исходного кода

Send AMS tray colours as uppercase hex (issue #2987)

Assigning a spool to an AMS slot unassigned it again seconds later, and
the slot's colour changed at the same time. It presented as Bambu Studio
and Bambuddy fighting over the slot. The reporter's log shows Bambuddy
losing to itself.

P1S firmware 01.10.00.00 reads every lowercase hex letter in an AMS
tray_color as a zero, and hides it completely: the command response
echoes back the value that was sent and reports result "success", so
only the next AMS push says what was really stored. The spool-assign
path sent spool.rgba verbatim and that column stores lowercase. From the
bundle:

  sent 09ff00ff  ->  AMS reports 09000000
  sent ff5100ff  ->  AMS reports 00510000
  sent 090000FF  ->  AMS reports 090000FF

That is the visible colour change, and it is also what deleted the
assignment. The auto-unlink sweep asks whether the slot still matches
the spool assigned to it; the mangled colour no longer did, so the
assignment Bambuddy had made four seconds earlier was removed.
colors_similar('09000000', '09FF00FF') is False, which is the whole of
it.

Re-assigning could not recover, because the Configure Slot dialog seeds
its colour from whatever the printer currently reports. It wrote the
mangled colour back and cemented it, which is the loop the report
describes in its steps 4 and 5.

Colours are now uppercased where the command is assembled rather than in
each of the four routes that configure a slot. A caller that forgets is
exactly how this arrived. Nothing else changes: no padding, no invented
alpha, no six-to-eight widening, and tray_type and tray_sub_brands keep
their case, where it carries meaning -- "PLA Matte" is a product line,
"PLA MATTE" is not.

Two paths deliberately left alone. The developer-mode probe re-sends the
colour the printer itself just reported so that the probe is inert;
uppercasing there would turn it into a write. And the Virtual Printer
forwards the slicer's own command verbatim -- Studio could in principle
hit the same firmware bug, but nothing here evidences that it sends
lowercase, and rewriting a slicer payload inside a transparent proxy is
not a change to make on a hunch.

Two more defects from the same log.

A spool with a brand and no subtype was configured with the string
"None" in its name: the branded branch interpolated spool.subtype
without checking it while the unbranded branch guarded it, so
"Sunlu PLA Matte None" went on the wire and into Studio's display.

And the FTP log is readable again. A 426 whose bytes Bambuddy has
already verified against the printer is how Bambu FTPS normally ends a
transfer, not a fault, so it drops from WARNING to INFO. It fired 54
times in this one bundle, every one followed by a completed upload, and
it was burying the 26 TLS handshake failures in the same log that
actually cost the reporter two prints. A 426 whose bytes do not verify
is still an error and still fails the upload.

The handshake failures themselves are printer-side FTPS cool-off under
load and are not touched here.
maziggy 1 неделя назад
Родитель
Сommit
0d21239e18

Разница между файлами не показана из-за своего большого размера
+ 0 - 0
CHANGELOG.md


+ 6 - 7
backend/app/api/routes/inventory.py

@@ -124,13 +124,12 @@ async def apply_spool_to_slot_via_mqtt(
     # may actually hold ("PLA+", "HTPLA") stays in tray_sub_brands below, which
     # is where Bambu puts it too (issue #2902).
     tray_type = printer_filament_type(spool.material)
-    tray_sub_brands = (
-        f"{spool.brand} {spool.material} {spool.subtype}".strip()
-        if spool.brand
-        else f"{spool.material} {spool.subtype}"
-        if spool.subtype
-        else spool.material
-    )
+    # Join only the parts that exist. The previous shape interpolated
+    # `spool.subtype` into the branded string without checking it, so a spool
+    # with a brand and no subtype went to the printer as
+    # "Sunlu PLA Matte None" -- the string "None", on the wire (#2987). The
+    # unbranded branch guarded subtype; the branded one did not.
+    tray_sub_brands = " ".join(p for p in (spool.brand, spool.material, spool.subtype) if p) or spool.material
     tray_color = spool.rgba or "FFFFFFFF"
 
     _generic_id_values = _GENERIC_ID_VALUES

+ 9 - 2
backend/app/services/bambu_ftp.py

@@ -976,7 +976,13 @@ class BambuFTPClient:
                     logger.debug("Post-error SIZE check failed: %s", size_err)
                     server_size = None
                 if server_size is not None and server_size == file_size:
-                    logger.warning(
+                    # INFO, not WARNING: a 426 whose bytes verify is the normal
+                    # way Bambu FTPS ends a transfer, not a fault. It fired 54
+                    # times in one support bundle and every one was followed by
+                    # a completed upload, which buried the 26 handshake failures
+                    # in the same log that actually cost the user two prints
+                    # (#2987). The unverified branch below is still an error.
+                    logger.info(
                         "FTP STOR returned %s for %s but file is intact on the "
                         "printer (%s bytes match) — proceeding: %s",
                         type(e).__name__,
@@ -1110,7 +1116,8 @@ class BambuFTPClient:
                     logger.debug("Post-error SIZE check failed: %s", size_err)
                     server_size = None
                 if server_size is not None and server_size == len(data):
-                    logger.warning(
+                    # INFO for the same reason as upload_file above (#2987).
+                    logger.info(
                         "FTP STOR returned %s for %s but file is intact on the "
                         "printer (%s bytes match) — proceeding: %s",
                         type(e).__name__,

+ 31 - 1
backend/app/services/bambu_mqtt.py

@@ -111,6 +111,34 @@ def normalize_am_unit_id(ams_id: int) -> int:
     return A2L_LITE_NORMALIZED_AMS_ID if ams_id == A2L_LITE_PHYSICAL_AMS_ID else ams_id
 
 
+def wire_tray_color(tray_color: str | None) -> str:
+    """Normalise a colour to the form AMS firmware actually parses: UPPERCASE hex.
+
+    P1S firmware 01.10.00.00 parses every lowercase hex letter in ``tray_color``
+    as a zero, and does it silently: the command response echoes the value you
+    sent and reports ``result: "success"``, so only the next AMS push shows what
+    was really stored. Measured on the reporter's machine (#2987), where the
+    spool's own ``rgba`` is stored lowercase and went out verbatim:
+
+        sent 09ff00ff  ->  AMS reports 09000000
+        sent ff5100ff  ->  AMS reports 00510000
+        sent 090000FF  ->  AMS reports 090000FF
+
+    A mangled colour is not merely cosmetic. The auto-unlink sweep compares the
+    tray against the spool it is assigned to, so the tray Bambuddy just wrote no
+    longer matches the spool that asked for it and the assignment is deleted
+    seconds after being made -- and re-assigning through the slot modal writes
+    the mangled colour back, because the modal seeds itself from the tray.
+
+    Applied here, at the one place the command is built, rather than in each of
+    the four callers: a caller that forgets is exactly how this arrived.
+
+    A leading ``#`` is stripped -- the wire format carries bare hex -- and a
+    blank stays blank, which is how a slot is cleared.
+    """
+    return (tray_color or "").strip().lstrip("#").upper()
+
+
 def a2l_lite_wire_ids(ams_id: int, tray_id: int) -> tuple[int, int, int] | None:
     """Translate a normalised A2L slot back to the physical wire form.
 
@@ -7680,7 +7708,9 @@ class BambuMQTTClient:
                 "tray_info_idx": tray_info_idx,
                 "tray_type": tray_type,
                 "tray_sub_brands": tray_sub_brands,
-                "tray_color": tray_color,
+                # UPPERCASE, always: lowercase hex is silently read as zeros by
+                # P1S firmware and acknowledged as a success (#2987).
+                "tray_color": wire_tray_color(tray_color),
                 "nozzle_temp_min": nozzle_temp_min,
                 "nozzle_temp_max": nozzle_temp_max,
                 "sequence_id": "0",

+ 105 - 0
backend/tests/integration/test_ams_slot_sub_brand_none_2987.py

@@ -0,0 +1,105 @@
+"""A spool with a brand and no subtype must not send the string "None" (#2987).
+
+``tray_sub_brands`` was built by interpolating brand, material and subtype into
+one f-string whenever the spool had a brand -- without checking that the subtype
+existed. The unbranded branch guarded it; the branded one did not. The
+reporter's Sunlu roll has no subtype, so the slot was configured as
+
+    "tray_sub_brands": "Sunlu PLA Matte None"
+
+which is what the printer stored and what Bambu Studio then displayed.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+from httpx import AsyncClient
+from sqlalchemy.ext.asyncio import AsyncSession
+
+from backend.app.models.printer import Printer
+from backend.app.models.spool import Spool
+
+
+def _mqtt_mock():
+    client = MagicMock()
+    client.ams_set_filament_setting.return_value = True
+    client.extrusion_cali_sel.return_value = True
+    return client
+
+
+def _status():
+    status = MagicMock()
+    status.raw_data = {"ams": {"ams": []}}
+    status.nozzles = [MagicMock(nozzle_diameter="0.4")]
+    status.ams_extruder_map = None
+    status.kprofiles = []
+    return status
+
+
+async def _assign(async_client, db_session, serial, **spool_kwargs):
+    printer = Printer(name="P1S", serial_number=serial, ip_address="192.168.1.78", access_code="12345678")
+    db_session.add(printer)
+    spool = Spool(rgba="09ff00ff", label_weight=1000, weight_used=0, **spool_kwargs)
+    db_session.add(spool)
+    await db_session.commit()
+    await db_session.refresh(printer)
+    await db_session.refresh(spool)
+
+    client = _mqtt_mock()
+    with patch("backend.app.services.printer_manager.printer_manager") as pm:
+        pm.get_client.return_value = client
+        pm.get_status.return_value = _status()
+        response = await async_client.post(
+            "/api/v1/inventory/assignments",
+            json={"spool_id": spool.id, "printer_id": printer.id, "ams_id": 0, "tray_id": 2},
+        )
+    assert response.status_code == 200
+    client.ams_set_filament_setting.assert_called_once()
+    return client.ams_set_filament_setting.call_args.kwargs
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_a_branded_spool_without_a_subtype_sends_no_none(async_client: AsyncClient, db_session: AsyncSession):
+    """The reported case."""
+    sent = await _assign(async_client, db_session, "SB2987A", brand="Sunlu", material="PLA Matte", subtype=None)
+
+    assert sent["tray_sub_brands"] == "Sunlu PLA Matte"
+    assert "None" not in sent["tray_sub_brands"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_a_branded_spool_with_a_subtype_still_carries_all_three(
+    async_client: AsyncClient, db_session: AsyncSession
+):
+    """The case that already worked, pinned so the fix does not drop the subtype."""
+    sent = await _assign(async_client, db_session, "SB2987B", brand="Sunlu", material="PLA", subtype="Silk")
+
+    assert sent["tray_sub_brands"] == "Sunlu PLA Silk"
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_an_unbranded_spool_with_a_subtype_is_unchanged(async_client: AsyncClient, db_session: AsyncSession):
+    sent = await _assign(async_client, db_session, "SB2987C", brand=None, material="PLA", subtype="Silk")
+
+    assert sent["tray_sub_brands"] == "PLA Silk"
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_a_spool_with_only_a_material_is_unchanged(async_client: AsyncClient, db_session: AsyncSession):
+    sent = await _assign(async_client, db_session, "SB2987D", brand=None, material="PLA", subtype=None)
+
+    assert sent["tray_sub_brands"] == "PLA"
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_an_empty_string_subtype_is_treated_as_absent(async_client: AsyncClient, db_session: AsyncSession):
+    """The old branded branch stripped a trailing space away, so "" already
+    behaved; None was the only broken input and both must stay correct."""
+    sent = await _assign(async_client, db_session, "SB2987E", brand="Sunlu", material="PLA Matte", subtype="")
+
+    assert sent["tray_sub_brands"] == "Sunlu PLA Matte"

+ 113 - 0
backend/tests/unit/services/test_ams_tray_color_case_2987.py

@@ -0,0 +1,113 @@
+"""AMS tray colours must go to the printer in UPPERCASE hex (#2987).
+
+P1S firmware 01.10.00.00 reads every lowercase hex letter in ``tray_color`` as a
+zero, and says nothing about it: the command response echoes the value that was
+sent and reports ``result: "success"``. Only the next AMS push shows what was
+really stored. From the reporter's support bundle, where the spool's ``rgba``
+column holds lowercase and went out verbatim:
+
+    sent 09ff00ff  ->  AMS reports 09000000
+    sent ff5100ff  ->  AMS reports 00510000
+    sent 090000FF  ->  AMS reports 090000FF
+
+The damage is not cosmetic. The auto-unlink sweep asks whether the tray still
+matches the spool assigned to it, so the tray Bambuddy had just written stopped
+matching the spool that asked for it and the assignment was deleted seconds
+later -- which is the disappearing assignment the issue reports.
+"""
+
+import json
+from unittest.mock import MagicMock
+
+import pytest
+
+from backend.app.services.bambu_mqtt import BambuMQTTClient, wire_tray_color
+
+
+def _client(model: str = "P1S") -> BambuMQTTClient:
+    client = BambuMQTTClient(ip_address="10.0.0.1", serial_number="P1S2987", access_code="c", model=model)
+    client._client = MagicMock()
+    client.state.connected = True
+    return client
+
+
+def _sent(client: BambuMQTTClient) -> dict:
+    return json.loads(client._client.publish.call_args[0][1])["print"]
+
+
+def _set(client: BambuMQTTClient, tray_color: str, **overrides):
+    kwargs = {
+        "ams_id": 0,
+        "tray_id": 2,
+        "tray_info_idx": "GFSNL03",
+        "tray_type": "PLA Matte",
+        "tray_sub_brands": "Sunlu PLA Matte",
+        "tray_color": tray_color,
+        "nozzle_temp_min": 200,
+        "nozzle_temp_max": 240,
+    }
+    kwargs.update(overrides)
+    assert client.ams_set_filament_setting(**kwargs)
+    return _sent(client)
+
+
+class TestWireTrayColor:
+    @pytest.mark.parametrize(
+        ("stored", "expected"),
+        [
+            pytest.param("09ff00ff", "09FF00FF", id="the-reported-spool"),
+            pytest.param("ff5100ff", "FF5100FF", id="the-second-spool-in-the-bundle"),
+            pytest.param("090000FF", "090000FF", id="already-uppercase-is-untouched"),
+            pytest.param("AbCdEf12", "ABCDEF12", id="mixed-case"),
+            pytest.param("  09ff00ff  ", "09FF00FF", id="surrounding-whitespace"),
+            pytest.param("#09ff00ff", "09FF00FF", id="css-style-hash-is-stripped"),
+        ],
+    )
+    def test_the_wire_form_is_uppercase_bare_hex(self, stored: str, expected: str):
+        assert wire_tray_color(stored) == expected
+
+    @pytest.mark.parametrize("blank", ["", "   ", None])
+    def test_a_blank_colour_stays_blank(self, blank):
+        """Clearing a slot sends an empty colour; it must not become "NONE"."""
+        assert wire_tray_color(blank) == ""
+
+    def test_only_the_case_changes(self):
+        """No padding, no alpha invented, no six-to-eight widening -- the
+        firmware's own format is whatever the caller resolved, and guessing at
+        it here would be a second bug wearing the first one's clothes."""
+        assert wire_tray_color("09ff00") == "09FF00"
+        assert wire_tray_color("09ff00ff00") == "09FF00FF00"
+
+
+class TestWhatReachesThePrinter:
+    def test_a_lowercase_colour_is_uppercased_on_the_wire(self):
+        """The reported case, end to end through the publisher."""
+        assert _set(_client(), "09ff00ff")["tray_color"] == "09FF00FF"
+
+    def test_an_uppercase_colour_is_unchanged(self):
+        assert _set(_client(), "090000FF")["tray_color"] == "090000FF"
+
+    def test_every_other_field_is_left_alone(self):
+        """Only the colour is normalised. tray_type and tray_sub_brands are
+        free text the printer stores verbatim, and case carries meaning there --
+        "PLA Matte" is a product line, "PLA MATTE" is not."""
+        sent = _set(_client(), "09ff00ff")
+        assert sent["tray_type"] == "PLA Matte"
+        assert sent["tray_sub_brands"] == "Sunlu PLA Matte"
+        assert sent["tray_info_idx"] == "GFSNL03"
+        assert (sent["nozzle_temp_min"], sent["nozzle_temp_max"]) == (200, 240)
+
+    def test_the_external_spool_is_normalised_too(self):
+        """ams_id 255 takes a different branch to build its wire ids, so the
+        colour has to be normalised where the command is assembled rather than
+        inside any one of them."""
+        assert _set(_client("X1C"), "09ff00ff", ams_id=255, tray_id=0)["tray_color"] == "09FF00FF"
+
+    def test_an_ams_ht_is_normalised_too(self):
+        assert _set(_client(), "09ff00ff", ams_id=128, tray_id=0)["tray_color"] == "09FF00FF"
+
+    def test_an_a2l_lite_slot_is_normalised_too(self):
+        assert _set(_client("A2L"), "09ff00ff", ams_id=6, tray_id=2)["tray_color"] == "09FF00FF"
+
+    def test_a_blank_colour_reaches_the_printer_blank(self):
+        assert _set(_client(), "")["tray_color"] == ""

+ 56 - 0
backend/tests/unit/services/test_bambu_ftp.py

@@ -487,6 +487,62 @@ class TestUpload:
         assert result is True, "intact file (SIZE match) tolerates 426 noise"
         client.disconnect()
 
+    def test_upload_426_with_intact_file_logs_at_info_not_warning(
+        self, ftp_client_factory, ftp_server, tmp_path, caplog
+    ):
+        """A verified-intact 426 is how Bambu FTPS normally ends a transfer,
+        not a fault, so it must not be a WARNING (#2987).
+
+        It fired 54 times in one support bundle, every one followed by a
+        completed upload, and buried the 26 TLS handshake failures in the same
+        log that actually cost the reporter two prints. The truncated case below
+        is still an error -- this only moves the one we have already verified.
+        """
+        import ftplib  # nosec B402 — tests need the real ftplib to construct mock 426 responses
+        import logging
+
+        local = tmp_path / "test.bin"
+        local.write_bytes(b"data" * 256)  # 1024 bytes
+        client = ftp_client_factory()
+        client.connect()
+
+        def raise_426():
+            raise ftplib.error_temp("426 Failure reading network stream.")
+
+        client._ftp.voidresp = raise_426
+        client._ftp.size = lambda _path: 1024
+
+        with caplog.at_level(logging.INFO, logger="backend.app.services.bambu_ftp"):
+            assert client.upload_file(local, "/cache/test.bin") is True
+        client.disconnect()
+
+        intact = [r for r in caplog.records if "file is intact on the" in r.getMessage()]
+        assert intact, "the proceed path must still say why it proceeded"
+        assert [r.levelno for r in intact] == [logging.INFO] * len(intact)
+
+    def test_upload_426_with_truncated_file_still_logs_an_error(self, ftp_client_factory, ftp_server, tmp_path, caplog):
+        """The half that must stay loud: bytes that did not verify."""
+        import ftplib  # nosec B402 — tests need the real ftplib to construct mock 426 responses
+        import logging
+
+        local = tmp_path / "test.bin"
+        local.write_bytes(b"data" * 256)
+        client = ftp_client_factory()
+        client.connect()
+
+        def raise_426():
+            raise ftplib.error_temp("426 Failure reading network stream.")
+
+        client._ftp.voidresp = raise_426
+        client._ftp.size = lambda _path: 100
+
+        with caplog.at_level(logging.INFO, logger="backend.app.services.bambu_ftp"):
+            assert client.upload_file(local, "/cache/test.bin") is False
+        client.disconnect()
+
+        rejected = [r for r in caplog.records if "rejected by printer" in r.getMessage()]
+        assert rejected and all(r.levelno == logging.ERROR for r in rejected)
+
     def test_upload_426_with_truncated_file_returns_false(self, ftp_client_factory, ftp_server, tmp_path):
         """The original #1401 fix is preserved: when SIZE confirms the file
         isn't on the server at full size (or SIZE itself fails), the upload

Некоторые файлы не были показаны из-за большого количества измененных файлов