From f075cdcb987bf2fb13c3407aa0a909a28a220975 Mon Sep 17 00:00:00 2001 From: Ian Chua Date: Thu, 8 Oct 2026 19:55:39 +0800 Subject: [PATCH] fix: harden printer-agent filament sync and teardown --- src/slic3r/Utils/CrealityPrintAgent.cpp | 10 +- src/slic3r/Utils/CrealityPrintAgent.hpp | 1 + src/slic3r/Utils/IPrinterAgent.hpp | 13 +- src/slic3r/Utils/MoonrakerPrinterAgent.cpp | 186 ++++++++++++--------- src/slic3r/Utils/MoonrakerPrinterAgent.hpp | 19 ++- src/slic3r/Utils/QidiPrinterAgent.cpp | 117 +++++++------ src/slic3r/Utils/QidiPrinterAgent.hpp | 5 +- src/slic3r/Utils/SnapmakerPrinterAgent.cpp | 113 ++++++++----- tests/slic3rutils/test_printer_agent.cpp | 52 +++--- 9 files changed, 314 insertions(+), 202 deletions(-) diff --git a/src/slic3r/Utils/CrealityPrintAgent.cpp b/src/slic3r/Utils/CrealityPrintAgent.cpp index b77f930456..5be7022484 100644 --- a/src/slic3r/Utils/CrealityPrintAgent.cpp +++ b/src/slic3r/Utils/CrealityPrintAgent.cpp @@ -298,7 +298,7 @@ bool CrealityPrintAgent::fetch_filament_info(std::string dev_id, FilamentSyncMod if (info.dev_ip.empty()) { BOOST_LOG_TRIVIAL(warning) << "CrealityPrintAgent::fetch_filament_info: no device IP, falling back to base agent"; - return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id)); + return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id), sync_mode); } // Build a CrealityPrint helper so we can use its model detection + WS helpers @@ -318,7 +318,7 @@ bool CrealityPrintAgent::fetch_filament_info(std::string dev_id, FilamentSyncMod BOOST_LOG_TRIVIAL(info) << "CrealityPrintAgent: " << host.model_name() << " is not CFS-capable, deferring to base Moonraker agent"; - return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id)); + return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id), sync_mode); } BOOST_LOG_TRIVIAL(info) @@ -333,7 +333,7 @@ bool CrealityPrintAgent::fetch_filament_info(std::string dev_id, FilamentSyncMod BOOST_LOG_TRIVIAL(warning) << "CrealityPrintAgent: CFS query failed (" << parse_err << "), " << "falling back to base agent"; - return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id)); + return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id), sync_mode); } if (box_count == 0) { @@ -342,7 +342,7 @@ bool CrealityPrintAgent::fetch_filament_info(std::string dev_id, FilamentSyncMod // Moonraker exposes. BOOST_LOG_TRIVIAL(info) << "CrealityPrintAgent: no active CFS boxes, deferring to base agent"; - return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id)); + return MoonrakerPrinterAgent::fetch_filament_info(std::move(dev_id), sync_mode); } BOOST_LOG_TRIVIAL(info) @@ -387,7 +387,7 @@ bool CrealityPrintAgent::fetch_filament_info(std::string dev_id, FilamentSyncMod } } - build_ams_payload(box_count, max_slots - 1, trays); + build_ams_payload(box_count, max_slots - 1, trays, sync_mode == FilamentSyncMode::pull); return true; } diff --git a/src/slic3r/Utils/CrealityPrintAgent.hpp b/src/slic3r/Utils/CrealityPrintAgent.hpp index 6c2aded326..142ef82926 100644 --- a/src/slic3r/Utils/CrealityPrintAgent.hpp +++ b/src/slic3r/Utils/CrealityPrintAgent.hpp @@ -43,6 +43,7 @@ public: AgentInfo get_agent_info() override { return get_agent_info_static(); } bool fetch_filament_info(std::string dev_id, FilamentSyncMode sync_mode = FilamentSyncMode::pull) override; + FilamentSyncMode get_filament_sync_mode() const override { return FilamentSyncMode::pull; } // Parse the boxsInfo JSON returned by CrealityPrint::query_boxes_info() into // a flat list of loaded slots, plus the count of CFS boxes the printer reports. diff --git a/src/slic3r/Utils/IPrinterAgent.hpp b/src/slic3r/Utils/IPrinterAgent.hpp index 293b0af2b8..07ce9af33d 100644 --- a/src/slic3r/Utils/IPrinterAgent.hpp +++ b/src/slic3r/Utils/IPrinterAgent.hpp @@ -455,9 +455,16 @@ public: virtual CameraStreamMode get_camera_stream_mode() const { return CameraStreamMode::none; } /** - * Refresh filament info from the printer synchronously. - * Should only be called when get_filament_sync_mode() returns FilamentSyncMode::pull. - * Populates the MachineObject's DevFilaSystem with fetched filament data. + * Refresh filament info from the printer. + * + * When called with FilamentSyncMode::pull (which requires get_filament_sync_mode() to + * return pull) this is a blocking, synchronous call: when it returns true the MachineObject's + * DevFilaSystem has already been populated and the caller may read it immediately. + * + * When called with FilamentSyncMode::subscription — by an implementation's own status loop — + * the refresh may be performed asynchronously: a true return then means the refresh was + * scheduled (or is already in flight), and DevFilaSystem is updated later on the main thread. + * Callers must not assume the data is ready on return. */ virtual bool fetch_filament_info(std::string dev_id, FilamentSyncMode sync_mode = FilamentSyncMode::pull) { return false; } diff --git a/src/slic3r/Utils/MoonrakerPrinterAgent.cpp b/src/slic3r/Utils/MoonrakerPrinterAgent.cpp index 11537ae460..32fff18342 100644 --- a/src/slic3r/Utils/MoonrakerPrinterAgent.cpp +++ b/src/slic3r/Utils/MoonrakerPrinterAgent.cpp @@ -42,9 +42,11 @@ #include #include #include -#include #include +#include + #include +#include #include #include #include @@ -53,7 +55,6 @@ #include #include #include -#include #include #include #include @@ -86,6 +87,12 @@ struct WsEndpoint bool secure = false; }; +// Wrap a bare IPv6 literal in brackets for use as a host / authority (Host header, logs). +std::string bracket_host_for_header(const std::string& host) +{ + return host.find(':') != std::string::npos ? "[" + host + "]" : host; +} + bool parse_ws_endpoint(const std::string& base_url, WsEndpoint& endpoint) { if (base_url.empty()) { @@ -110,7 +117,21 @@ bool parse_ws_endpoint(const std::string& base_url, WsEndpoint& endpoint) endpoint.host = url; endpoint.port = endpoint.secure ? MOONRAKER_DEFAULT_TLS_PORT : MOONRAKER_DEFAULT_PORT; - if (auto colon = url.rfind(':'); colon != std::string::npos && url.find(']') == std::string::npos) { + if (url.rfind('[', 0) == 0) { + // Bracketed IPv6 literal, e.g. [fe80::1] or [fe80::1]:7125. Keep the bare address for the + // resolver/host-name verification; the Host header re-brackets it via bracket_host_for_header. + const auto close = url.find(']'); + if (close == std::string::npos) { + return false; + } + endpoint.host = url.substr(1, close - 1); + if (close + 1 < url.size()) { + if (url[close + 1] != ':') { + return false; + } + endpoint.port = url.substr(close + 2); + } + } else if (auto colon = url.rfind(':'); colon != std::string::npos) { endpoint.host = url.substr(0, colon); endpoint.port = url.substr(colon + 1); } @@ -149,16 +170,18 @@ struct MoonrakerWebsocket::Impl using SecurePtr = std::unique_ptr; explicit Impl(bool secure, std::string api_key, std::string ca_file) - : secure(secure), api_key(std::move(api_key)), ca_file(std::move(ca_file)), ssl_context(net::ssl::context::tls_client) + : secure(secure), api_key(std::move(api_key)), ca_file(std::move(ca_file)) { if (this->secure) { + // Build the TLS context lazily so plaintext connections do not initialize OpenSSL. + ssl_context = std::make_unique(net::ssl::context::tls_client); if (!this->ca_file.empty()) { - ssl_context.load_verify_file(this->ca_file); + ssl_context->load_verify_file(this->ca_file); } else { - ssl_context.set_default_verify_paths(); + ssl_context->set_default_verify_paths(); } - ssl_context.set_verify_mode(net::ssl::verify_peer); - websocket = std::make_unique(ioc, ssl_context); + ssl_context->set_verify_mode(net::ssl::verify_peer); + websocket = std::make_unique(ioc, *ssl_context); } else { websocket = std::make_unique(beast::tcp_stream{ioc}); } @@ -168,7 +191,7 @@ struct MoonrakerWebsocket::Impl std::string api_key; std::string ca_file; net::io_context ioc; - net::ssl::context ssl_context; + std::unique_ptr ssl_context; std::variant websocket; }; @@ -529,6 +552,8 @@ int MoonrakerPrinterAgent::bind_detect(std::string dev_ip, std::string sec_link, // so the name falls back to the IP instead of blank. (matches // feature/printer-agent-port-pristine; the IP is what shipped before the port) // note: dummy id/creds; use_ssl false because Moonraker/print-host is http. + // init_device_info writes device_info; take the same lock every other writer/reader uses. + std::lock_guard lock(connect_mutex); init_device_info(PrinterConnectionParams{ dev_ip, dev_ip, "", "", "", false, "" }); @@ -765,16 +790,21 @@ int MoonrakerPrinterAgent::set_queue_on_main_fn(QueueOnMainFn fn) return BAMBU_NETWORK_SUCCESS; } -void MoonrakerPrinterAgent::build_ams_payload(int ams_count, int max_lane_index, const std::vector& trays) +void MoonrakerPrinterAgent::build_ams_payload(int ams_count, int max_lane_index, const std::vector& trays, bool apply_inline, + ResolveAmsTrayIdsFn resolve_tray_ids) { // This may be called from a background thread (e.g. run_status_stream's read loop, // for subscription-mode agents) as well as from the GUI thread (Sidebar's pull-mode // path). Everything below touches MachineObject/DevFilaSystem, which the GUI thread - // reads without locking — so the actual mutation must run on the main thread. Snapshot + // reads without locking — so the actual mutation must run on the main thread. + // + // In the pull path the caller is already the GUI thread and reads DevFilaSystem as soon + // as this returns, so the payload is applied inline (apply_inline == true) rather than + // posted back through the event queue, which would land after that read. Snapshot // queue_on_main_fn and the two device_info fields we need up front, then defer the rest, // mirroring dispatch_message's existing queue_fn ? queue_fn(x) : x() idiom. QueueOnMainFn queue_fn; - { + if (!apply_inline) { std::lock_guard lock(state_mutex); queue_fn = queue_on_main_fn; } @@ -783,7 +813,14 @@ void MoonrakerPrinterAgent::build_ams_payload(int ams_count, int max_lane_index, std::string dev_id = info.dev_id; std::string model_id = info.model_id; - auto apply = [dev_id, model_id, ams_count, max_lane_index, trays]() { + auto apply = [dev_id, model_id, ams_count, max_lane_index, trays, resolve_tray_ids]() { + // Resolve preset-dependent tray ids on the main thread, not on the background fetch thread + // that produced `trays`. resolve_tray_ids must not capture `this` (this commit may outlive + // the agent once queued). + std::vector resolved = trays; + if (resolve_tray_ids) { + resolve_tray_ids(resolved); + } // Look up MachineObject via DeviceManager auto* dev_manager = GUI::wxGetApp().getDeviceManager(); if (!dev_manager) { @@ -816,7 +853,7 @@ void MoonrakerPrinterAgent::build_ams_payload(int ams_count, int max_lane_index, // Find tray with matching slot_index const AmsTrayData* tray = nullptr; - for (const auto& t : trays) { + for (const auto& t : resolved) { if (t.slot_index == slot_index) { tray = &t; break; @@ -870,7 +907,7 @@ void MoonrakerPrinterAgent::build_ams_payload(int ams_count, int max_lane_index, // Call the parser to populate DevFilaSystem DevFilaSystemParser::ParseV1_0(print_json, obj, obj->GetFilaSystem().get(), false); - BOOST_LOG_TRIVIAL(info) << "MoonrakerPrinterAgent::build_ams_payload: Parsed " << trays.size() << " trays"; + BOOST_LOG_TRIVIAL(info) << "MoonrakerPrinterAgent::build_ams_payload: Parsed " << resolved.size() << " trays"; // Set printer_type so update_sync_status() can match it against the preset's printer type. // Without this, the comparison fails and all sync badges are cleared. @@ -899,11 +936,15 @@ void MoonrakerPrinterAgent::build_ams_payload(int ams_count, int max_lane_index, } }; - if (queue_fn) { - queue_fn(apply); - } else { + if (apply_inline) { + // Pull path: the caller is the GUI thread and reads DevFilaSystem as soon as this + // returns, so apply directly instead of posting back through the event queue. apply(); + } else if (queue_fn) { + queue_fn(apply); } + // else: queue_on_main_fn was cleared (e.g. during GUI shutdown) while this background fetch + // is still running. Skip the mutation rather than touch MachineObject off the main thread. } bool MoonrakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSyncMode sync_mode) @@ -922,7 +963,7 @@ bool MoonrakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync BOOST_LOG_TRIVIAL(info) << "MoonrakerPrinterAgent::fetch_filament_info: Detected Moonraker filament system with " << (max_lane_index + 1) << " lanes"; int ams_count = (max_lane_index + 4) / 4; - build_ams_payload(ams_count, max_lane_index, trays); + build_ams_payload(ams_count, max_lane_index, trays, sync_mode == FilamentSyncMode::pull); return true; } @@ -931,7 +972,7 @@ bool MoonrakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync BOOST_LOG_TRIVIAL(info) << "MoonrakerPrinterAgent::fetch_filament_info: Detected Happy Hare MMU with " << (max_lane_index + 1) << " gates"; int ams_count = (max_lane_index + 4) / 4; - build_ams_payload(ams_count, max_lane_index, trays); + build_ams_payload(ams_count, max_lane_index, trays, sync_mode == FilamentSyncMode::pull); return true; } @@ -1602,7 +1643,9 @@ int MoonrakerPrinterAgent::handle_request(const std::string& dev_id, const std:: // why: reaching here means no case claimed the command, which is the honest verdict for // every control Klipper has no equivalent for. Returning SUCCESS instead made all of them - // look like they worked. Nothing surfaces this code to the user yet. + // look like they worked. The caller (MachineObject::publish_json) maps this code to the + // "not supported on this printer" dialog, so only a command the user actually fired should + // reach here. BOOST_LOG_TRIVIAL(warning) << "MoonrakerPrinterAgent: no translation for " << command_namespace << "." << command_name << ", dropping"; return ORCA_NETWORK_ERR_CMD_NOT_SUPPORTED; @@ -1869,7 +1912,7 @@ bool MoonrakerPrinterAgent::send_ws_rpc(const std::string& method, const nlohman ws.connect(endpoint.host, port, std::chrono::seconds(5)); ws.tls_handshake(endpoint.host); - std::string host_header = endpoint.host; + std::string host_header = bracket_host_for_header(endpoint.host); if (!port.empty() && port != default_port) { host_header += ":" + port; } @@ -1963,60 +2006,51 @@ bool MoonrakerPrinterAgent::fetch_webcam_info(const ConnectionSettings& connecti std::string webcam_name; CameraStreamMode stream_mode = CameraStreamMode::none; std::string error; - // A subclass may name its stream directly (e.g. a fixed webcam path with no - // /server/webcams/list entry); consult it before doing any HTTP. - const std::string override_url = webcam_stream_override(connection.base_url); try { - if (!override_url.empty()) { - camera_url = override_url; - stream_mode = (override_url.rfind("rtsp://", 0) == 0 || override_url.rfind("rtsps://", 0) == 0) - ? CameraStreamMode::rtsp - : CameraStreamMode::http; - } else { - std::string response_body; - bool success = false; - std::string http_error; + std::string response_body; + bool success = false; + std::string http_error; - auto http = Http::get(join_url(connection.base_url, "/server/webcams/list")); - configure_http(http, connection); - if (!connection.api_key.empty()) { - http.header("X-Api-Key", connection.api_key); - } - http.timeout_connect(5) - .timeout_max(10) - .on_complete([&](std::string body, unsigned status_code) { - if (status_code == 200) { - response_body = body; - success = true; - } else { - http_error = "HTTP error: " + std::to_string(status_code); - } - }) - .on_error([&](std::string body, std::string err, unsigned status_code) { - http_error = err; - if (status_code > 0) { - http_error += " (HTTP " + std::to_string(status_code) + ")"; - } - }) - .perform_sync(); - - if (!success) { - error = http_error.empty() ? "Connection failed" : http_error; - } else { - BOOST_LOG_TRIVIAL(debug) << "[Moonraker Diagnostic] " << connection.base_url - << " webcams list: " << response_body.size() << " bytes"; - auto json = nlohmann::json::parse(response_body, nullptr, false, true); - if (json.is_discarded()) { - error = "Invalid JSON response"; + auto http = Http::get(join_url(connection.base_url, "/server/webcams/list")); + configure_http(http, connection); + if (!connection.api_key.empty()) { + http.header("X-Api-Key", connection.api_key); + } + http.timeout_connect(5) + .timeout_max(10) + .on_complete([&](std::string body, unsigned status_code) { + if (status_code == 200) { + response_body = body; + success = true; } else { - MoonrakerWebcamSelection selection; - if (moonraker_parse_webcam_list(json, connection.base_url, selection)) { - camera_url = selection.url; - stream_mode = selection.mode; - webcam_name = selection.name; - } else { - error = selection.error; - } + http_error = "HTTP error: " + std::to_string(status_code); + } + }) + .on_error([&](std::string body, std::string err, unsigned status_code) { + http_error = err; + if (status_code > 0) { + http_error += " (HTTP " + std::to_string(status_code) + ")"; + } + }) + .on_progress([this](Http::Progress, bool& cancel) { cancel = ws_stop.load(); }) + .perform_sync(); + + if (!success) { + error = http_error.empty() ? "Connection failed" : http_error; + } else { + BOOST_LOG_TRIVIAL(debug) << "[Moonraker Diagnostic] " << connection.base_url + << " webcams list: " << response_body.size() << " bytes"; + auto json = nlohmann::json::parse(response_body, nullptr, false, true); + if (json.is_discarded()) { + error = "Invalid JSON response"; + } else { + MoonrakerWebcamSelection selection; + if (moonraker_parse_webcam_list(json, connection.base_url, selection)) { + camera_url = selection.url; + stream_mode = selection.mode; + webcam_name = selection.name; + } else { + error = selection.error; } } } @@ -2219,6 +2253,7 @@ bool MoonrakerPrinterAgent::fetch_object_list(const ConnectionSettings& connecti http_error += " (HTTP " + std::to_string(status) + ")"; } }) + .on_progress([this](Http::Progress, bool& cancel) { cancel = ws_stop.load(); }) .perform_sync(); if (!success) { @@ -2445,7 +2480,7 @@ void MoonrakerPrinterAgent::run_status_stream(std::string dev_id, ConnectionSett ws.connect(endpoint.host, endpoint.port, std::chrono::seconds(10)); ws.tls_handshake(endpoint.host); - std::string host_header = endpoint.host; + std::string host_header = bracket_host_for_header(endpoint.host); if (!endpoint.port.empty() && endpoint.port != (endpoint.secure ? MOONRAKER_DEFAULT_TLS_PORT : MOONRAKER_DEFAULT_PORT)) { host_header += ":" + endpoint.port; } @@ -3183,7 +3218,8 @@ void MoonrakerPrinterAgent::perform_connection_async(const std::string& dev_id, if (connection.use_ssl && !connection.ca_file.empty() && !Http::ca_file_supported()) { BOOST_LOG_TRIVIAL(warning) << "MoonrakerPrinterAgent: a custom CA file is configured but this platform cannot load CA files " - "(Schannel/DarwinSSL); the CA will be ignored and the TLS connection may fail verification."; + "for HTTP requests (Schannel/DarwinSSL); HTTPS verification may fail. WebSocket (wss://) " + "connections still use the CA via OpenSSL."; } try { diff --git a/src/slic3r/Utils/MoonrakerPrinterAgent.hpp b/src/slic3r/Utils/MoonrakerPrinterAgent.hpp index 584fb2b094..bf1a919e75 100644 --- a/src/slic3r/Utils/MoonrakerPrinterAgent.hpp +++ b/src/slic3r/Utils/MoonrakerPrinterAgent.hpp @@ -15,7 +15,6 @@ #include #include #include -#include #include #include @@ -164,8 +163,19 @@ protected: int nozzle_temp = 0; // Optional }; - // Build ams JSON and call parser - void build_ams_payload(int ams_count, int max_lane_index, const std::vector& trays); + // Build ams JSON and call parser. + // apply_inline: when true the MachineObject mutation runs on the calling thread, which + // must therefore be the main thread; when false it is deferred through queue_on_main_fn. + // Pull-mode fetches run synchronously on the GUI thread and are read back immediately, so + // they pass true to avoid the deferred mutation landing after the caller's read. + // + // resolve_tray_ids: optional hook that runs as part of the commit, i.e. on the main thread. + // Agents that would otherwise resolve tray_info_idx against GUI-owned preset state on a + // background fetch thread must do it here to avoid racing the GUI. It must not capture + // `this` (the commit may outlive the agent). + using ResolveAmsTrayIdsFn = std::function&)>; + void build_ams_payload(int ams_count, int max_lane_index, const std::vector& trays, bool apply_inline = false, + ResolveAmsTrayIdsFn resolve_tray_ids = {}); // Methods that derived classes may need to override or access virtual bool init_device_info(const PrinterConnectionParams& params); @@ -267,9 +277,6 @@ private: ConnectionSettings connection, uint64_t generation); - // why: a printer with no /server/webcams/list entry can still name its stream directly; - // subclasses (e.g. printers with a fixed webcam path) can override this instead. - virtual std::string webcam_stream_override(const std::string& base_url) const { return {}; } void refresh_webcam_info() const; bool fetch_webcam_info(const ConnectionSettings& connection, uint64_t generation) const; diff --git a/src/slic3r/Utils/QidiPrinterAgent.cpp b/src/slic3r/Utils/QidiPrinterAgent.cpp index 87d5058f2e..53174bc831 100644 --- a/src/slic3r/Utils/QidiPrinterAgent.cpp +++ b/src/slic3r/Utils/QidiPrinterAgent.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include "libslic3r/Preset.hpp" #include #include @@ -57,7 +58,21 @@ struct InFlightGuard int read_int_or(const nlohmann::json& obj, const std::string& key, int fallback) { auto it = obj.find(key); - return (it != obj.end() && it->is_number_integer()) ? it->get() : fallback; + if (it == obj.end()) { + return fallback; + } + if (it->is_number_integer()) { + return it->get(); + } + // Firmware may report an integral field as a JSON float (e.g. 2.0). Accept it only when it + // has no fractional part, so a genuinely fractional value falls back instead of truncating. + if (it->is_number_float()) { + const double value = it->get(); + if (value == std::floor(value)) { + return static_cast(value); + } + } + return fallback; } } // anonymous namespace @@ -95,18 +110,11 @@ bool QidiPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSyncMode model_name = device_info.model_name; } - // Reserve under the same mutex shutdown() uses, so the flag and the count can't race. - { - std::lock_guard lock(fetch_lifecycle_mutex); - if (shutting_down.load()) - return false; - if (filament_fetch_in_flight.load() > 0) - return true; // a fetch is already running; don't pile on - filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed); - } - - InFlightGuard guard{*this}; - std::thread([this, guard = std::move(guard), connection = std::move(connection), model_id, model_name]() mutable { + // One implementation for both modes; only where it runs differs. pull is the documented + // blocking contract (the GUI thread reads DevFilaSystem right after the call), so it runs + // on the caller with the payload applied inline. subscription runs on a background thread + // so the status loop is not stalled, and the payload is marshalled back to the main thread. + auto work = [this, connection = std::move(connection), model_id, model_name](bool apply_inline) -> bool { try { std::string error; @@ -134,18 +142,57 @@ bool QidiPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSyncMode int box_count = 0; if (!fetch_slot_info(connection, dict, series_id, trays, box_count, error)) { BOOST_LOG_TRIVIAL(warning) << "QidiPrinterAgent::fetch_filament_info: Failed to fetch slot info: " << error; - return; + return false; } - // 4. Build the AMS payload - build_ams_payload(box_count, box_count * 4 - 1, trays); + // 4. Build the AMS payload, resolving preset-dependent ids on the main thread. + auto resolve_ids = [](std::vector& resolved_trays) { + auto* bundle = GUI::wxGetApp().preset_bundle; + if (!bundle) { + return; // keep the Qidi-specific ids computed from device data + } + for (auto& tray : resolved_trays) { + if (!tray.has_filament) { + continue; + } + if (!tray.tray_info_idx.empty() && has_visible_base_preset(bundle->filaments, tray.tray_info_idx)) { + continue; + } + tray.tray_info_idx = bundle->filaments.filament_id_by_type(tray.tray_type); + } + }; + build_ams_payload(box_count, box_count * 4 - 1, trays, apply_inline, resolve_ids); + return true; } catch (const std::exception& e) { // why: an exception escaping a detached thread is std::terminate, and firmware // JSON is untrusted; mirror run_command_worker and swallow it here. BOOST_LOG_TRIVIAL(error) << "QidiPrinterAgent::fetch_filament_info: unhandled exception: " << e.what(); + return false; } catch (...) { BOOST_LOG_TRIVIAL(error) << "QidiPrinterAgent::fetch_filament_info: unhandled exception"; + return false; } + }; + + if (sync_mode == FilamentSyncMode::pull) { + return work(/*apply_inline=*/true); + } + + // Subscription: fire-and-forget. A `true` return means the refresh was scheduled (or is + // already running), not that DevFilaSystem has been updated. Reserve under the same mutex + // shutdown() uses, so the flag and the count can't race. + { + std::lock_guard lock(fetch_lifecycle_mutex); + if (shutting_down.load()) + return false; + if (filament_fetch_in_flight.load() > 0) + return true; // a fetch is already running; don't pile on + filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed); + } + + InFlightGuard guard{*this}; + std::thread([work = std::move(work), guard = std::move(guard)]() mutable { + work(/*apply_inline=*/false); }).detach(); return true; } @@ -198,33 +245,14 @@ bool QidiPrinterAgent::apply_box_mapping(const PrintParams& params) const int QidiPrinterAgent::start_local_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn) { + // Only the LAN print path is implemented by the Moonraker base; the box config is emitted + // here, before the print starts. The cloud/record/sdcard variants inherited from the base + // deliberately return ORCA_NETWORK_ERR_CMD_NOT_SUPPORTED without side effects. if (!apply_box_mapping(params)) return BAMBU_NETWORK_ERR_PRINT_LP_PUBLISH_MSG_FAILED; return MoonrakerPrinterAgent::start_local_print(std::move(params), update_fn, cancel_fn); } -int QidiPrinterAgent::start_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn, OnWaitFn wait_fn) -{ - if (!apply_box_mapping(params)) - return BAMBU_NETWORK_ERR_PRINT_LP_PUBLISH_MSG_FAILED; - return MoonrakerPrinterAgent::start_print(std::move(params), update_fn, cancel_fn, wait_fn); -} - -int QidiPrinterAgent::start_local_print_with_record(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn, OnWaitFn wait_fn) -{ - // A failed box mapping is a send failure, not an upload failure. - if (!apply_box_mapping(params)) - return BAMBU_NETWORK_ERR_PRINT_LP_PUBLISH_MSG_FAILED; - return MoonrakerPrinterAgent::start_local_print_with_record(std::move(params), update_fn, cancel_fn, wait_fn); -} - -int QidiPrinterAgent::start_sdcard_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn) -{ - if (!apply_box_mapping(params)) - return BAMBU_NETWORK_ERR_PRINT_LP_PUBLISH_MSG_FAILED; - return MoonrakerPrinterAgent::start_sdcard_print(std::move(params), update_fn, cancel_fn); -} - bool QidiPrinterAgent::fetch_slot_info(const ConnectionSettings& connection, const QidiFilamentDict& dict, const std::string& series_id, @@ -321,16 +349,9 @@ bool QidiPrinterAgent::fetch_slot_info(const ConnectionSettings& connection, } tray.tray_type = normalize_filament_type(filament_name); - // Try Qidi-specific setting ID first; fall back to visible preset by type - std::string setting_id = build_setting_id(filament_type, vendor_type, tray.tray_type); - auto* bundle = GUI::wxGetApp().preset_bundle; - if (!bundle) { - tray.tray_info_idx = setting_id; - } else if (!setting_id.empty() && has_visible_base_preset(bundle->filaments, setting_id)) { - tray.tray_info_idx = setting_id; - } else { - tray.tray_info_idx = bundle->filaments.filament_id_by_type(tray.tray_type); - } + // Qidi-specific setting id derived from device data only; the preset-dependent + // fallback runs on the main thread in the resolve hook passed to build_ams_payload. + tray.tray_info_idx = build_setting_id(filament_type, vendor_type, tray.tray_type); // Look up color from dictionary auto color_it = dict.colors.find(color_index); diff --git a/src/slic3r/Utils/QidiPrinterAgent.hpp b/src/slic3r/Utils/QidiPrinterAgent.hpp index c2229ec4c9..9dcb409168 100644 --- a/src/slic3r/Utils/QidiPrinterAgent.hpp +++ b/src/slic3r/Utils/QidiPrinterAgent.hpp @@ -30,10 +30,9 @@ public: std::string& error); // Print operations — emit QiDi multi-color box config, then delegate to base. - int start_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn, OnWaitFn wait_fn) override; + // Only the LAN print path is supported by the base; the cloud/record/sdcard variants + // are inherited and return ORCA_NETWORK_ERR_CMD_NOT_SUPPORTED. int start_local_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn) override; - int start_local_print_with_record(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn, OnWaitFn wait_fn) override; - int start_sdcard_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn) override; FilamentSyncMode get_filament_sync_mode() const override; diff --git a/src/slic3r/Utils/SnapmakerPrinterAgent.cpp b/src/slic3r/Utils/SnapmakerPrinterAgent.cpp index c6383b6514..04973990b4 100644 --- a/src/slic3r/Utils/SnapmakerPrinterAgent.cpp +++ b/src/slic3r/Utils/SnapmakerPrinterAgent.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -51,7 +52,21 @@ struct InFlightGuard int read_int_or(const nlohmann::json& obj, const char* key, int fallback) { auto it = obj.find(key); - return (it != obj.end() && it->is_number_integer()) ? it->get() : fallback; + if (it == obj.end()) { + return fallback; + } + if (it->is_number_integer()) { + return it->get(); + } + // Firmware may report an integral field as a JSON float (e.g. 2.0). Accept it only when it + // has no fractional part, so a genuinely fractional value falls back instead of truncating. + if (it->is_number_float()) { + const double value = it->get(); + if (value == std::floor(value)) { + return static_cast(value); + } + } + return fallback; } std::vector read_string_array_or(const nlohmann::json& obj, const char* key) @@ -225,18 +240,11 @@ bool SnapmakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync // device_info meanwhile. const ConnectionSettings connection = get_connection_settings(); - // Reserve under the same mutex shutdown() uses, so the flag and the count can't race. - { - std::lock_guard lock(fetch_lifecycle_mutex); - if (shutting_down.load()) - return false; - if (filament_fetch_in_flight.load() > 0) - return true; // a fetch is already running; don't pile on - filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed); - } - - InFlightGuard guard{*this}; - std::thread([this, guard = std::move(guard), connection]() { + // One implementation for both modes; only where it runs differs. pull is the documented + // blocking contract (the GUI thread reads DevFilaSystem right after the call), so it runs + // on the caller with the payload applied inline. subscription runs on a background thread + // so the status loop is not stalled, and the payload is marshalled back to the main thread. + auto work = [this, connection](bool apply_inline) -> bool { try { const std::string url = join_url(connection.base_url, "/printer/objects/query?print_task_config&filament_detect"); @@ -269,19 +277,19 @@ bool SnapmakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync if (!success) { BOOST_LOG_TRIVIAL(warning) << "SnapmakerPrinterAgent::fetch_filament_info: HTTP request failed: " << http_error; - return; + return false; } auto json = nlohmann::json::parse(response_body, nullptr, false, true); if (json.is_discarded()) { BOOST_LOG_TRIVIAL(warning) << "SnapmakerPrinterAgent::fetch_filament_info: Invalid JSON response"; - return; + return false; } // Navigate to result.status.print_task_config if (!json.contains("result") || !json["result"].contains("status") || !json["result"]["status"].contains("print_task_config")) { BOOST_LOG_TRIVIAL(warning) << "SnapmakerPrinterAgent::fetch_filament_info: Missing print_task_config in response"; - return; + return false; } auto& ptc = json["result"]["status"]["print_task_config"]; @@ -296,7 +304,7 @@ bool SnapmakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync const int slot_count = static_cast(filament_exist.size()); if (slot_count == 0) { BOOST_LOG_TRIVIAL(info) << "SnapmakerPrinterAgent::fetch_filament_info: No filament slots reported"; - return; + return false; } // Read NFC filament_detect data for temperature info (optional) @@ -308,6 +316,9 @@ bool SnapmakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync static const std::string empty_str; static const std::string default_color = "FFFFFFFF"; + // Per-tray vendor names, kept so the preset lookup can run on the main thread. + std::vector vendors(slot_count); + std::vector trays; trays.reserve(slot_count); @@ -319,25 +330,11 @@ bool SnapmakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync if (tray.has_filament) { tray.tray_type = combine_filament_type(safe_at(filament_type, i, empty_str), safe_at(filament_sub_type, i, empty_str)); tray.tray_color = safe_at(filament_color, i, default_color); + vendors[i] = safe_at(filament_vendor, i, empty_str); - auto* bundle = GUI::wxGetApp().preset_bundle; - // Try to find a matching preset for this filament based on vendor, type and color. - // If not found, default to traditional search by type only or generic type mapping. - if (bundle) { - std::string vendor = safe_at(filament_vendor, i, empty_str); - std::string filament_id = find_closest_color_preset_by_vendor_and_type(bundle->filaments, vendor, tray.tray_type, - tray.tray_color); - - if (!filament_id.empty()) { - tray.tray_info_idx = filament_id; - BOOST_LOG_TRIVIAL(warning) - << "Filament sync: Found manufacturer-specific profile for slot " << i << ": " << filament_id; - } else { - tray.tray_info_idx = bundle->filaments.filament_id_by_type(tray.tray_type); - } - } else { - tray.tray_info_idx = map_filament_type_to_generic_id(tray.tray_type); - } + // Generic id derived from device data only; the preset-dependent lookup runs + // on the main thread in the resolve hook passed to build_ams_payload. + tray.tray_info_idx = map_filament_type_to_generic_id(tray.tray_type); // Extract NFC temperature data if available if (nfc_info.is_array() && i < static_cast(nfc_info.size()) && nfc_info[i].is_object()) { @@ -356,22 +353,62 @@ bool SnapmakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync trays.emplace_back(std::move(tray)); } - build_ams_payload(1, slot_count - 1, trays); + // Resolve against GUI-owned presets on the main thread (inside build_ams_payload). + auto resolve_ids = [vendors](std::vector& resolved_trays) { + auto* bundle = GUI::wxGetApp().preset_bundle; + if (!bundle) { + return; // keep the generic ids computed from device data + } + for (size_t i = 0; i < resolved_trays.size() && i < vendors.size(); ++i) { + AmsTrayData& tray = resolved_trays[i]; + if (!tray.has_filament) { + continue; + } + // Prefer a manufacturer/colour-specific preset, else fall back to type. + std::string filament_id = find_closest_color_preset_by_vendor_and_type(bundle->filaments, vendors[i], tray.tray_type, + tray.tray_color); + tray.tray_info_idx = filament_id.empty() ? bundle->filaments.filament_id_by_type(tray.tray_type) : filament_id; + } + }; + build_ams_payload(1, slot_count - 1, trays, apply_inline, resolve_ids); + return true; } catch (const std::exception& e) { // why: an exception escaping a detached thread is std::terminate, and firmware // JSON is untrusted; mirror run_command_worker and swallow it here. BOOST_LOG_TRIVIAL(error) << "SnapmakerPrinterAgent::fetch_filament_info: unhandled exception: " << e.what(); + return false; } catch (...) { BOOST_LOG_TRIVIAL(error) << "SnapmakerPrinterAgent::fetch_filament_info: unhandled exception"; + return false; } - }).detach(); + }; + if (sync_mode == FilamentSyncMode::pull) { + return work(/*apply_inline=*/true); + } + + // Subscription: fire-and-forget. A `true` return means the refresh was scheduled (or is + // already running), not that DevFilaSystem has been updated. Reserve under the same mutex + // shutdown() uses, so the flag and the count can't race. + { + std::lock_guard lock(fetch_lifecycle_mutex); + if (shutting_down.load()) + return false; + if (filament_fetch_in_flight.load() > 0) + return true; // a fetch is already running; don't pile on + filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed); + } + + InFlightGuard guard{*this}; + std::thread([work = std::move(work), guard = std::move(guard)]() mutable { + work(/*apply_inline=*/false); + }).detach(); return true; } std::string SnapmakerPrinterAgent::get_camera_url() const { - return get_connection_settings().base_url + "/server/files/camera/monitor.jpg"; + return join_url(get_connection_settings().base_url, "/server/files/camera/monitor.jpg"); } FilamentSyncMode SnapmakerPrinterAgent::get_filament_sync_mode() const diff --git a/tests/slic3rutils/test_printer_agent.cpp b/tests/slic3rutils/test_printer_agent.cpp index 8e92a71621..7fc6328ca8 100644 --- a/tests/slic3rutils/test_printer_agent.cpp +++ b/tests/slic3rutils/test_printer_agent.cpp @@ -10,7 +10,6 @@ #include #include #include -#include "catch2/catch_approx.hpp" #include "python_test_support.hpp" #include @@ -66,7 +65,7 @@ public: explicit MoonrakerParserProbe(std::string log_dir) : MoonrakerPrinterAgent(std::move(log_dir)) {} }; -TEST_CASE("Moonraker parses nozzle diameter from configfile settings", "[unit][moonraker]") +TEST_CASE("Moonraker parses nozzle diameter from configfile settings", "[MoonrakerPrinterAgent]") { const auto response = nlohmann::json::parse(R"({ "result": { @@ -82,10 +81,10 @@ TEST_CASE("Moonraker parses nozzle diameter from configfile settings", "[unit][m } })"); - CHECK(MoonrakerParserProbe::parse_nozzle_diameter(response) == Catch::Approx(0.6f)); + CHECK_THAT(MoonrakerParserProbe::parse_nozzle_diameter(response), Catch::Matchers::WithinAbs(0.6f, 1e-4f)); } -TEST_CASE("Moonraker parses nozzle diameter from raw config and tolerates missing data", "[unit][moonraker]") +TEST_CASE("Moonraker parses nozzle diameter from raw config and tolerates missing data", "[MoonrakerPrinterAgent]") { const auto raw_config_response = nlohmann::json::parse(R"({ "result": { @@ -102,12 +101,12 @@ TEST_CASE("Moonraker parses nozzle diameter from raw config and tolerates missin })"); const auto missing_response = nlohmann::json::object(); - CHECK(MoonrakerParserProbe::parse_nozzle_diameter(raw_config_response) == Catch::Approx(0.8f)); - CHECK(MoonrakerParserProbe::parse_nozzle_diameter(missing_response) == 0.0f); + CHECK_THAT(MoonrakerParserProbe::parse_nozzle_diameter(raw_config_response), Catch::Matchers::WithinAbs(0.8f, 1e-4f)); + CHECK_THAT(MoonrakerParserProbe::parse_nozzle_diameter(missing_response), Catch::Matchers::WithinAbs(0.0f, 1e-6f)); } // why: an agent without a Bambu-dialect translation must refuse these commands before any network or wx path. -TEST_CASE("unit: default AMS commands report not supported", "[unit][moonraker]") +TEST_CASE("default AMS commands report not supported", "[MoonrakerPrinterAgent]") { MoonrakerPrinterAgent agent(""); @@ -116,7 +115,7 @@ TEST_CASE("unit: default AMS commands report not supported", "[unit][moonraker]" CHECK(agent.command_ams_select_tray("dev", "123", 3, false) == ORCA_NETWORK_ERR_CMD_NOT_SUPPORTED); } -TEST_CASE("unit: Moonraker light name matching", "[unit][moonraker]") +TEST_CASE("Moonraker light name matching", "[MoonrakerPrinterAgent]") { CHECK(moonraker_is_light_name("caselight")); CHECK(moonraker_is_light_name("LED_STRIP")); @@ -126,7 +125,7 @@ TEST_CASE("unit: Moonraker light name matching", "[unit][moonraker]") } TEST_CASE("Moonraker webcam selection skips disabled webcams and prefers the first enabled one", - "[unit][moonraker]") + "[MoonrakerPrinterAgent]") { const auto response = nlohmann::json::parse(R"({ "result": { "webcams": [ @@ -144,7 +143,7 @@ TEST_CASE("Moonraker webcam selection skips disabled webcams and prefers the fir } TEST_CASE("Moonraker webcam selection resolves relative URLs, maps rtsp, and rejects other schemes", - "[unit][moonraker]") + "[MoonrakerPrinterAgent]") { const auto relative = nlohmann::json::parse(R"({ "result": { "webcams": [ { "name": "cam", "snapshot_url": "/webcam/?action=snapshot" } ] } @@ -170,7 +169,7 @@ TEST_CASE("Moonraker webcam selection resolves relative URLs, maps rtsp, and rej CHECK(bad.error == "Unsupported webcam URL"); } -TEST_CASE("Moonraker webcam selection reports no webcam and malformed structure", "[unit][moonraker]") +TEST_CASE("Moonraker webcam selection reports no webcam and malformed structure", "[MoonrakerPrinterAgent]") { const auto empty = nlohmann::json::parse(R"({ "result": { "webcams": [] } })"); MoonrakerWebcamSelection none; @@ -198,7 +197,7 @@ TEST_CASE("Moonraker webcam selection reports no webcam and malformed structure" // roughly once a second, so it must stay a success or it would raise a dialog on a // timer. Only branches that touch neither the network nor wx are exercised. // =========================================================================== -TEST_CASE("unit: Moonraker reports untranslated commands as not supported", "[unit][moonraker]") +TEST_CASE("Moonraker reports untranslated commands as not supported", "[MoonrakerPrinterAgent]") { MoonrakerPrinterAgent agent(""); @@ -217,12 +216,13 @@ TEST_CASE("unit: Moonraker reports untranslated commands as not supported", "[un CHECK(agent.send_message("dev", "{not json", 0, 0) == BAMBU_NETWORK_ERR_INVALID_RESULT); } -// why: IPrinterAgent::fetch_filament_info is the single virtual hook derived agents override -// (MoonrakerPrinterAgent's own override is synchronous, but QidiPrinterAgent's override is -// fire-and-forget: it spawns a detached thread and returns immediately). QidiPrinterAgent is -// `final`, so this probes the same contract with a controllable double instead. -TEST_CASE("unit: a fire-and-forget override of fetch_filament_info is not waited on by the caller", - "[unit][moonraker]") +// why: IPrinterAgent::fetch_filament_info is the single virtual hook derived agents override. +// Pull-mode overrides must be synchronous (the caller reads DevFilaSystem as soon as it +// returns), but subscription overrides may be fire-and-forget: QidiPrinterAgent/Snapmaker +// spawn a detached thread when asked for subscription updates. QidiPrinterAgent is `final`, +// so this probes the subscription contract with a controllable double instead. +TEST_CASE("a fire-and-forget subscription fetch is not waited on by the caller", + "[MoonrakerPrinterAgent]") { class RecordingAgent : public Slic3r::MoonrakerPrinterAgent { @@ -235,8 +235,12 @@ TEST_CASE("unit: a fire-and-forget override of fetch_filament_info is not waited std::shared_ptr> release_gate{std::make_shared>()}; std::shared_ptr> done_promise{std::make_shared>()}; - bool fetch_filament_info(std::string /*dev_id*/, FilamentSyncMode /*sync_mode*/ = FilamentSyncMode::pull) override + bool fetch_filament_info(std::string /*dev_id*/, FilamentSyncMode sync_mode = FilamentSyncMode::pull) override { + // Only the subscription path is allowed to be fire-and-forget. + if (sync_mode != FilamentSyncMode::subscription) + return true; + auto invoked_p = invoked; auto release_gate_p = release_gate; auto done_promise_p = done_promise; @@ -255,7 +259,7 @@ TEST_CASE("unit: a fire-and-forget override of fetch_filament_info is not waited auto done_future = agent->done_promise->get_future(); ScopedPromiseRelease release_gate_guard{agent->release_gate}; - bool immediate_result = agent->fetch_filament_info("test-dev"); + bool immediate_result = agent->fetch_filament_info("test-dev", FilamentSyncMode::subscription); // fetch_filament_info must return before its background work completes — prove // it by confirming the background call is still blocked on the gate right now. @@ -353,7 +357,7 @@ public: // The command worker parks a fetch before it reserves the in-flight slot, forcing // the "wait already observed zero" interleaving deterministically. TEST_CASE("an agent's destruction waits for a fetch started by its worker during teardown", - "[unit][moonraker][Regression]") + "[MoonrakerPrinterAgent][Regression]") { g_deferred_fetch_running.store(0); g_deferred_destroy_returned.store(false); @@ -409,7 +413,7 @@ TEST_CASE("an agent's destruction waits for a fetch started by its worker during // Confirms a duplicate agent id is rejected so a plugin cannot shadow a built-in // or previously registered agent. // =========================================================================== -TEST_CASE("unit: printer-agent registry register / lookup / duplicate-reject", "[registry][unit]") +TEST_CASE("printer-agent registry register / lookup / duplicate-reject", "[PrinterAgent]") { // why: the registry is process-global state shared by the test binary, and // Catch2 may run cases in any order. Use an id that cannot collide with @@ -453,7 +457,7 @@ TEST_CASE("unit: printer-agent registry register / lookup / duplicate-reject", " // fails to load the codecs needed for the filesystem encoding. The shared helper // points PyConfig.home at the python/ runtime staged next to the test executable. -TEST_CASE("integration: orca.printer_agent binding surface", "[integration][Python]") +TEST_CASE("orca.printer_agent binding surface", "[integration][Python]") { py::module_ orca = import_orca_module(); @@ -504,7 +508,7 @@ TEST_CASE("integration: orca.printer_agent binding surface", "[integration][Pyth // them in the lightweight embedded-interpreter test catches binding breakage // before the plugin-loader test needs to run. // =========================================================================== -TEST_CASE("integration: orca plugin-registration API surface + discovery-context guards", "[integration][Python]") +TEST_CASE("orca plugin-registration API surface + discovery-context guards", "[integration][Python]") { py::module_ orca = import_orca_module();