diff --git a/backend/app/api/routes/inventory.py b/backend/app/api/routes/inventory.py index c18192f69..18eadec7a 100644 --- a/backend/app/api/routes/inventory.py +++ b/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 diff --git a/backend/app/services/bambu_ftp.py b/backend/app/services/bambu_ftp.py index 2c1c7517e..3650a59d4 100644 --- a/backend/app/services/bambu_ftp.py +++ b/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__, diff --git a/backend/app/services/bambu_mqtt.py b/backend/app/services/bambu_mqtt.py index d1806afe4..53cadeb5a 100644 --- a/backend/app/services/bambu_mqtt.py +++ b/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", diff --git a/backend/tests/integration/test_ams_slot_sub_brand_none_2987.py b/backend/tests/integration/test_ams_slot_sub_brand_none_2987.py new file mode 100644 index 000000000..25f6532f4 --- /dev/null +++ b/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" diff --git a/backend/tests/unit/services/test_ams_tray_color_case_2987.py b/backend/tests/unit/services/test_ams_tray_color_case_2987.py new file mode 100644 index 000000000..0dba8f3ea --- /dev/null +++ b/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"] == "" diff --git a/backend/tests/unit/services/test_bambu_ftp.py b/backend/tests/unit/services/test_bambu_ftp.py index ad0cf6e57..5b736cd22 100644 --- a/backend/tests/unit/services/test_bambu_ftp.py +++ b/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