Pass bind_detect's Result to Python by Reference

pybind11 copies a reference argument to an override, so a Python
printer agent that filled in detect wrote to a throwaway object and the
host always saw an empty detectResult. Pass it as a pointer so Python
edits the caller's struct.

src/slic3r/plugin/pluginTypes/printerAgent/PrinterAgentPluginCapabilityTrampoline.hpp

@@ -7,7 +7,31 @@
 #include "IPrinterAgent.hpp"
 #include <slic3r/plugin/PythonPluginInterface.hpp>
 
+#include <type_traits>
+
+// IPrinterAgent reports failure through its return values and its callers do not catch, so nothing
+// the plugin does may leave the trampoline as an exception: a Python raise, a missing override or a
+// wrongly typed return is logged and answered with what NetworkAgent returns when no agent is set.
+#define ORCA_PY_AGENT_CATCH(name) \
+    catch (const std::exception& ex) { this->log_failure(#name, ex.what()); } \
+    catch (...) { this->log_failure(#name, "unknown error"); }
+
+#define ORCA_PY_AGENT_OVERRIDE(ret, name, ...) \
+    try { \
+        ORCA_PY_OVERRIDE_AUDITED([] {}, PYBIND11_OVERRIDE_PURE, ret, PrinterAgentPluginCapability, name, ##__VA_ARGS__); \
+    } ORCA_PY_AGENT_CATCH(name) \
+    return printer_agent_failure<ret>()
+
 namespace Slic3r {
+// NetworkAgent's no-agent answer: -1 for a status code, the empty value (false, "", none) otherwise.
+template<typename T> T printer_agent_failure()
+{
+    if constexpr (std::is_same_v<T, int>)
+        return -1;
+    else if constexpr (!std::is_void_v<T>)
+        return T{};
+}
+
 class PyPrinterAgentPluginCapabilityTrampoline : public PyPluginCommonTrampoline<PrinterAgentPluginCapability>
 {
 public:
@@ -15,207 +39,158 @@ public:
 
     AgentInfo get_agent_info() override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, AgentInfo, PrinterAgentPluginCapability,
-            get_agent_info);
+        ORCA_PY_AGENT_OVERRIDE(AgentInfo, get_agent_info);
     }
 
     int connect_printer(std::string dev_id, std::string dev_ip, std::string username, std::string password, bool use_ssl) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, connect_printer, dev_id,
-            dev_ip, username, password, use_ssl);
+        ORCA_PY_AGENT_OVERRIDE(int, connect_printer, dev_id, dev_ip, username, password, use_ssl);
     }
 
     int disconnect_printer() override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, disconnect_printer);
+        ORCA_PY_AGENT_OVERRIDE(int, disconnect_printer);
     }
 
     int send_message(std::string dev_id, std::string json_str, int qos, int flag) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, send_message, dev_id,
-            json_str, qos, flag);
+        ORCA_PY_AGENT_OVERRIDE(int, send_message, dev_id, json_str, qos, flag);
     }
 
     int send_message_to_printer(std::string dev_id, std::string json_str, int qos, int flag) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, send_message_to_printer,
-            dev_id, json_str, qos, flag);
+        ORCA_PY_AGENT_OVERRIDE(int, send_message_to_printer, dev_id, json_str, qos, flag);
     }
 
     bool start_discovery(bool start, bool sending) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, bool, PrinterAgentPluginCapability, start_discovery, start,
-            sending);
+        ORCA_PY_AGENT_OVERRIDE(bool, start_discovery, start, sending);
     }
 
     int bind_detect(std::string dev_ip, std::string sec_link, detectResult& detect) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, bind_detect, dev_ip,
-            sec_link, detect);
+        // Passed as a pointer: pybind11 copies a reference argument, so the plugin's writes would be lost.
+        ORCA_PY_AGENT_OVERRIDE(int, bind_detect, dev_ip, sec_link, &detect);
     }
 
     std::string get_user_selected_machine() override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, std::string, PrinterAgentPluginCapability,
-            get_user_selected_machine);
+        ORCA_PY_AGENT_OVERRIDE(std::string, get_user_selected_machine);
     }
 
     int set_user_selected_machine(std::string dev_id) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability,
-            set_user_selected_machine, dev_id);
+        ORCA_PY_AGENT_OVERRIDE(int, set_user_selected_machine, dev_id);
     }
 
     int start_send_gcode_to_sdcard(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn, OnWaitFn wait_fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability,
-            start_send_gcode_to_sdcard, params, update_fn, cancel_fn, wait_fn);
+        ORCA_PY_AGENT_OVERRIDE(int, start_send_gcode_to_sdcard, params, update_fn, cancel_fn, wait_fn);
     }
 
     int start_local_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, start_local_print,
-            params, update_fn, cancel_fn);
+        ORCA_PY_AGENT_OVERRIDE(int, start_local_print, params, update_fn, cancel_fn);
     }
 
     FilamentSyncMode get_filament_sync_mode() const override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, FilamentSyncMode, PrinterAgentPluginCapability,
-            get_filament_sync_mode);
+        ORCA_PY_AGENT_OVERRIDE(FilamentSyncMode, get_filament_sync_mode);
     }
 
     bool fetch_filament_info(std::string dev_id) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, bool, PrinterAgentPluginCapability, fetch_filament_info, dev_id);
+        ORCA_PY_AGENT_OVERRIDE(bool, fetch_filament_info, dev_id);
     }
 
     int check_cert() override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, check_cert);
+        ORCA_PY_AGENT_OVERRIDE(int, check_cert);
     }
 
     void install_device_cert(std::string dev_id, bool lan_only) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, void, PrinterAgentPluginCapability, install_device_cert, dev_id,
-            lan_only);
+        ORCA_PY_AGENT_OVERRIDE(void, install_device_cert, dev_id, lan_only);
     }
 
     int ping_bind(std::string ping_code) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, ping_bind, ping_code);
+        ORCA_PY_AGENT_OVERRIDE(int, ping_bind, ping_code);
     }
 
     int bind(std::string dev_ip, std::string dev_id, std::string dev_model, std::string sec_link, std::string timezone, bool improved, OnUpdateStatusFn update_fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, bind, dev_ip, dev_id,
-            dev_model, sec_link, timezone, improved, update_fn);
+        ORCA_PY_AGENT_OVERRIDE(int, bind, dev_ip, dev_id, dev_model, sec_link, timezone, improved, update_fn);
     }
 
     int unbind(std::string dev_id) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, unbind, dev_id);
+        ORCA_PY_AGENT_OVERRIDE(int, unbind, dev_id);
     }
 
     int start_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn, OnWaitFn wait_fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, start_print, params,
-            update_fn, cancel_fn, wait_fn);
+        ORCA_PY_AGENT_OVERRIDE(int, start_print, params, update_fn, cancel_fn, wait_fn);
     }
 
     int start_local_print_with_record(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn, OnWaitFn wait_fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability,
-            start_local_print_with_record, params, update_fn, cancel_fn, wait_fn);
+        ORCA_PY_AGENT_OVERRIDE(int, start_local_print_with_record, params, update_fn, cancel_fn, wait_fn);
     }
 
     int start_sdcard_print(PrintParams params, OnUpdateStatusFn update_fn, WasCancelledFn cancel_fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, start_sdcard_print, params,
-            update_fn, cancel_fn);
+        ORCA_PY_AGENT_OVERRIDE(int, start_sdcard_print, params, update_fn, cancel_fn);
     }
 
     int get_hms_snapshot(std::string dev_id, std::string file_name, std::function<void(std::string, int)> callback) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, get_hms_snapshot, dev_id,
-            file_name, callback);
+        ORCA_PY_AGENT_OVERRIDE(int, get_hms_snapshot, dev_id, file_name, callback);
     }
 
     int set_server_callback(OnServerErrFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_server_callback, fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_server_callback, fn);
     }
 
     int set_on_ssdp_msg_fn(OnMsgArrivedFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_on_ssdp_msg_fn, fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_on_ssdp_msg_fn, fn);
     }
 
     int set_on_printer_connected_fn(OnPrinterConnectedFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_on_printer_connected_fn,
-            fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_on_printer_connected_fn, fn);
     }
 
     int set_on_subscribe_failure_fn(GetSubscribeFailureFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_on_subscribe_failure_fn,
-            fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_on_subscribe_failure_fn, fn);
     }
 
     int set_on_message_fn(OnMessageFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_on_message_fn, fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_on_message_fn, fn);
     }
 
     int set_on_user_message_fn(OnMessageFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_on_user_message_fn, fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_on_user_message_fn, fn);
     }
 
     int set_on_local_connect_fn(OnLocalConnectedFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_on_local_connect_fn, fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_on_local_connect_fn, fn);
     }
 
     int set_on_local_message_fn(OnMessageFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_on_local_message_fn, fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_on_local_message_fn, fn);
     }
 
     int set_queue_on_main_fn(QueueOnMainFn fn) override
     {
-        ORCA_PY_OVERRIDE_AUDITED(
-            [] {}, PYBIND11_OVERRIDE_PURE, int, PrinterAgentPluginCapability, set_queue_on_main_fn, fn);
+        ORCA_PY_AGENT_OVERRIDE(int, set_queue_on_main_fn, fn);
     }
 
     // request_bind_ticket returns its ticket through a std::string* out-param, which pybind11
@@ -223,24 +198,33 @@ public:
     // returns a (result, ticket) tuple, which we unpack into the int result and the out-param.
     int request_bind_ticket(std::string* ticket) override
     {
-        ORCA_PY_AUDIT_SCOPE();
-        ::Slic3r::PluginCapabilityInterface::RefCounter _orca_ref_counter(*this);
-        ::Slic3r::PythonGILState gil;
-        if (!gil)
-            throw std::runtime_error("Python interpreter is shutting down");
-        pybind11::function override =
-            pybind11::get_override(static_cast<const PrinterAgentPluginCapability*>(this), "request_bind_ticket");
-        if (!override)
-            pybind11::pybind11_fail("Tried to call pure virtual function \"PrinterAgentPluginCapability::request_bind_ticket\"");
         try {
-            pybind11::tuple result = override().cast<pybind11::tuple>();
-            if (ticket)
-                *ticket = result[1].cast<std::string>();
-            return result[0].cast<int>();
-        } catch (pybind11::error_already_set& err) {
-            ::Slic3r::log_python_exception_keep(err);
-            throw;
-        }
+            ORCA_PY_AUDIT_SCOPE();
+            ::Slic3r::PluginCapabilityInterface::RefCounter _orca_ref_counter(*this);
+            ::Slic3r::PythonGILState gil;
+            if (!gil)
+                throw std::runtime_error("Python interpreter is shutting down");
+            pybind11::function override =
+                pybind11::get_override(static_cast<const PrinterAgentPluginCapability*>(this), "request_bind_ticket");
+            if (!override)
+                pybind11::pybind11_fail("Tried to call pure virtual function \"PrinterAgentPluginCapability::request_bind_ticket\"");
+            try {
+                pybind11::tuple result = override().cast<pybind11::tuple>();
+                if (ticket)
+                    *ticket = result[1].cast<std::string>();
+                return result[0].cast<int>();
+            } catch (pybind11::error_already_set& err) {
+                ::Slic3r::log_python_exception_keep(err);
+                throw;
+            }
+        } ORCA_PY_AGENT_CATCH(request_bind_ticket)
+        return printer_agent_failure<int>();
+    }
+
+private:
+    void log_failure(const char* operation, const char* error) const
+    {
+        BOOST_LOG_TRIVIAL(error) << "Printer agent plugin '" << this->audit_plugin_key() << "': " << operation << " failed: " << error;
     }
 };
 } // namespace Slic3r

tests/slic3rutils/CMakeLists.txt

@@ -13,6 +13,7 @@ add_executable(${_TEST_NAME}_tests
     test_plugin_capabilities_in_use.cpp
     test_plugin_install.cpp
     test_plugin_lifecycle.cpp
+    test_plugin_printer_agent.cpp
     test_slicing_pipeline_bindings.cpp
     test_slicing_pipeline_config.cpp
     test_plugin_sort.cpp

tests/slic3rutils/test_plugin_printer_agent.cpp

@@ -0,0 +1,161 @@
+#include <catch2/catch_all.hpp>
+
+#include <slic3r/plugin/PluginManager.hpp>
+#include <slic3r/plugin/PythonInterpreter.hpp>
+#include <slic3r/plugin/PythonPluginBridge.hpp>
+#include <slic3r/plugin/PythonPluginInterface.hpp>
+#include <slic3r/Utils/IPrinterAgent.hpp>
+
+#include <pybind11/embed.h>
+#include <pybind11/pybind11.h>
+
+#include <memory>
+#include <string>
+
+namespace py = pybind11;
+using namespace Slic3r;
+
+namespace {
+
+// Same idiom as ScopedPluginManager in test_plugin_lifecycle.cpp: the trampolines refuse to call
+// into Python unless PythonInterpreter::instance() reports initialized.
+struct ScopedPluginManager
+{
+    bool initialized = PluginManager::instance().initialize();
+
+    ~ScopedPluginManager()
+    {
+        PluginManager::instance().shutdown();
+        PythonInterpreter::instance().shutdown();
+    }
+};
+
+// The host reaches a printer agent plugin through IPrinterAgent, so the tests do too. The Python
+// instance carries the overrides, so it has to outlive every call, as PluginInstanceHandle ensures
+// in production.
+struct Agent
+{
+    py::object                     instance;
+    std::shared_ptr<IPrinterAgent> agent;
+
+    IPrinterAgent* operator->() const { return agent.get(); }
+    IPrinterAgent& operator*() const { return *agent; }
+};
+
+Agent make_agent(const std::string& body)
+{
+    (void) PythonPluginBridge::instance(); // force the embedded module registration into the binary
+    py::dict globals;
+    globals["orca"] = py::module_::import("orca");
+
+    py::exec("class Agent(orca.printer_agent.PrinterAgentBase):\n"
+             "    def get_name(self): return 'agent'\n" + body, globals);
+    py::object instance   = globals["Agent"]();
+    auto       capability = instance.cast<std::shared_ptr<PluginCapabilityInterface>>();
+    capability->set_audit_plugin_key("agent_plugin");
+    return {instance, std::dynamic_pointer_cast<IPrinterAgent>(capability)};
+}
+
+// One operation per return type and dispatch shape; the rest share their macro.
+const std::string OPERATIONS[] = {"get_agent_info", "disconnect_printer", "start_discovery", "get_user_selected_machine",
+                                  "get_filament_sync_mode", "install_device_cert", "start_local_print", "request_bind_ticket"};
+
+// What NetworkAgent answers when no printer agent is set.
+void check_answers_like_no_agent(IPrinterAgent& agent)
+{
+    std::string ticket = "untouched";
+
+    CHECK(agent.get_agent_info().id.empty());
+    CHECK(agent.disconnect_printer() == -1);
+    CHECK_FALSE(agent.start_discovery(true, false));
+    CHECK(agent.get_user_selected_machine().empty());
+    CHECK(agent.get_filament_sync_mode() == FilamentSyncMode::none);
+    CHECK_NOTHROW(agent.install_device_cert("dev", true));
+    CHECK(agent.start_local_print(PrintParams{}, nullptr, nullptr) == -1);
+    CHECK(agent.request_bind_ticket(&ticket) == -1);
+    CHECK(ticket == "untouched");
+}
+
+std::string define_all(const std::string& signature_tail, const std::string& statement)
+{
+    std::string body;
+    for (const std::string& operation : OPERATIONS)
+        body += "    def " + operation + "(self" + signature_tail + "): " + statement + "\n";
+    return body;
+}
+
+} // namespace
+
+TEST_CASE("A printer agent operation that raises answers like a missing agent", "[PluginPrinterAgent][Python]")
+{
+    ScopedPluginManager plugin_system; // declared first: destroyed last
+    if (!plugin_system.initialized)
+        SKIP("Bundled Python interpreter unavailable: " + PythonInterpreter::instance().last_error());
+    py::gil_scoped_acquire gil; // released before plugin_system's destructor shuts Python down
+
+    auto agent = make_agent(define_all(", *args", "raise RuntimeError('boom')"));
+    REQUIRE(agent.agent);
+
+    check_answers_like_no_agent(*agent);
+
+    // The interpreter stays usable.
+    CHECK(py::eval("1 + 1").cast<int>() == 2);
+}
+
+TEST_CASE("A printer agent that omits its operations answers like a missing agent", "[PluginPrinterAgent][Python]")
+{
+    ScopedPluginManager plugin_system;
+    if (!plugin_system.initialized)
+        SKIP("Bundled Python interpreter unavailable: " + PythonInterpreter::instance().last_error());
+    py::gil_scoped_acquire gil;
+
+    auto agent = make_agent("");
+    REQUIRE(agent.agent);
+
+    check_answers_like_no_agent(*agent);
+}
+
+TEST_CASE("A printer agent operation returning the wrong type answers like a missing agent", "[PluginPrinterAgent][Python]")
+{
+    ScopedPluginManager plugin_system;
+    if (!plugin_system.initialized)
+        SKIP("Bundled Python interpreter unavailable: " + PythonInterpreter::instance().last_error());
+    py::gil_scoped_acquire gil;
+
+    auto agent = make_agent(define_all(", *args", "return object()"));
+    REQUIRE(agent.agent);
+
+    check_answers_like_no_agent(*agent);
+}
+
+TEST_CASE("A working printer agent's answers reach the host unchanged", "[PluginPrinterAgent][Python]")
+{
+    ScopedPluginManager plugin_system;
+    if (!plugin_system.initialized)
+        SKIP("Bundled Python interpreter unavailable: " + PythonInterpreter::instance().last_error());
+    py::gil_scoped_acquire gil;
+
+    auto agent = make_agent("    def get_agent_info(self): return orca.printer_agent.AgentInfo('id', 'name', '1', 'description')\n"
+                            "    def disconnect_printer(self): return 7\n"
+                            "    def start_discovery(self, start, sending): return start and not sending\n"
+                            "    def get_user_selected_machine(self): return 'machine'\n"
+                            "    def get_filament_sync_mode(self): return orca.printer_agent.FilamentSyncMode.Pull\n"
+                            "    def request_bind_ticket(self): return (3, 'ticket')\n"
+                            "    def bind_detect(self, dev_ip, sec_link, detect):\n"
+                            "        detect.dev_id = dev_ip\n"
+                            "        return 0\n");
+    REQUIRE(agent.agent);
+
+    std::string  ticket;
+    detectResult detect;
+
+    CHECK(agent->get_agent_info().id == "id");
+    CHECK(agent->disconnect_printer() == 7);
+    CHECK(agent->start_discovery(true, false));
+    CHECK(agent->get_user_selected_machine() == "machine");
+    CHECK(agent->get_filament_sync_mode() == FilamentSyncMode::pull);
+    CHECK(agent->request_bind_ticket(&ticket) == 3);
+    CHECK(ticket == "ticket");
+    CHECK(agent->bind_detect("192.168.0.2", "secure", detect) == 0);
+    CHECK(detect.dev_id == "192.168.0.2");
+}