From 5629dd29e9e4c8b78105364f96ee8d56fe23bf22 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Thu, 3 Sep 2026 00:55:35 -0400 Subject: [PATCH] Restore the pre-IMEX pressure advance output for non-IMEX printers Closes review comment 6. The per-tool pressure advance work changed set_pressure_advance() for users who are not using the feature. RepRapFirmware lost its D qualifier when no tool index was supplied: upstream emits M572 D0 S unconditionally, and a bare M572 applies to whatever tool is currently selected and errors when there is none, so PA started depending on tool-selection state for every RRF user. The D is back, defaulting to 0, and D is reached only from the IMEX paths. The same rewrite had also changed the comment separator from "; Override" to " ; Override" on the Klipper, RRF, Marlin 2.x and Marlin Legacy branches, so every non-IMEX print of those flavors carried a one-byte diff. Restored. Upstream is internally inconsistent here -- BBL and Repetier do use the spaced form -- and the point is to match it exactly rather than to tidy it. Emitted output for all six flavors with no tool index is now byte-identical to upstream. Verified on a real slice: a Klipper profile emits "SET_PRESSURE_ADVANCE ADVANCE=0.02; Override pressure advance value", an exact string match, with no EXTRUDER= qualifier. The tests were pinning the regressed form and are inverted. Also records at the imex key registrations why they are kept out of the g-code config block, matching the house convention at the other banned keys. Co-Authored-By: Claude Opus 5 (1M context) --- src/libslic3r/GCodeWriter.cpp | 18 +++++++++--------- src/libslic3r/PrintConfig.cpp | 7 ++++++- tests/fff_print/test_gcodewriter.cpp | 15 +++++++-------- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/libslic3r/GCodeWriter.cpp b/src/libslic3r/GCodeWriter.cpp index 1cbc5e4c58..8250cc09d8 100644 --- a/src/libslic3r/GCodeWriter.cpp +++ b/src/libslic3r/GCodeWriter.cpp @@ -522,14 +522,14 @@ std::string GCodeWriter::set_pressure_advance(double pa, int tool) const gcode << " EXTRUDER=extruder" << tool; else if (tool == 0) gcode << " EXTRUDER=extruder"; - gcode << " ; Override pressure advance value\n"; + gcode << "; Override pressure advance value\n"; } else if (FLAVOR_IS(gcfRepRapFirmware)) { - // RRF: M572 without D applies to the current tool; with D targets a specific extruder. - // Use D only when an explicit tool index is provided (IMEX parallel modes). - gcode << "M572"; - if (tool >= 0) - gcode << " D" << tool; - gcode << " S" << std::setprecision(4) << pa << " ; Override pressure advance value\n"; + // RRF: M572 D targets a specific extruder; a bare M572 applies to whatever tool is + // currently selected and errors when there isn't one. Callers with no tool index keep + // the historical D0 rather than the bare form: PA would otherwise depend on + // tool-selection state for every RRF user, none of whom are asking for IMEX. + gcode << "M572 D" << (tool >= 0 ? tool : 0) + << " S" << std::setprecision(4) << pa << "; Override pressure advance value\n"; } else if (FLAVOR_IS(gcfRepetier)) { // Repetier M233: X is quadratic (K), Y is linear (L). // Applying the value to both parameters simultaneously. @@ -539,10 +539,10 @@ std::string GCodeWriter::set_pressure_advance(double pa, int tool) const gcode << "M900 K" << std::setprecision(4) << pa; if (tool >= 0) gcode << " T" << tool; - gcode << " ; Override pressure advance value\n"; + gcode << "; Override pressure advance value\n"; } else { // Marlin Legacy and everything else: single-extruder M900, no tool parameter - gcode << "M900 K" << std::setprecision(4) << pa << " ; Override pressure advance value\n"; + gcode << "M900 K" << std::setprecision(4) << pa << "; Override pressure advance value\n"; } return gcode.str(); } diff --git a/src/libslic3r/PrintConfig.cpp b/src/libslic3r/PrintConfig.cpp index 634164a005..fc946211a5 100644 --- a/src/libslic3r/PrintConfig.cpp +++ b/src/libslic3r/PrintConfig.cpp @@ -6604,7 +6604,12 @@ void PrintConfigDef::init_fff_params() def->mode = comAdvanced; def->set_default_value(new ConfigOptionBool(true)); - // IDEX/IQEX (independent X extruder) — parallel printing support for IDEX/IQEX printers + // IDEX/IQEX (independent X extruder) — parallel printing support for IDEX/IQEX printers. + // Every key in this group is read at slice time from m_config, and the two per-plate process + // keys (imex_parallel_mode, imex_head_filament_map) round-trip through the 3MF's + // model_settings.config plate metadata, so none of them needs to appear in the exported + // g-code. They all carry non-nil defaults, so emitting them would add a line to every + // printer's dump. Kept out of the g-code config block (banned_keys). def = this->add("is_imex", coBool); def->label = L("IMEX Printer"); def->tooltip = L("Enable IMEX parallel printing for printers with multiple independent carriages (IDEX, IQEX, and similar)."); diff --git a/tests/fff_print/test_gcodewriter.cpp b/tests/fff_print/test_gcodewriter.cpp index a1969a2f89..1761e41f4f 100644 --- a/tests/fff_print/test_gcodewriter.cpp +++ b/tests/fff_print/test_gcodewriter.cpp @@ -845,14 +845,14 @@ SCENARIO("set_pressure_advance emits Klipper form with optional EXTRUDER=extrude std::string out = writer.set_pressure_advance(0.05); THEN("Output contains SET_PRESSURE_ADVANCE ADVANCE=0.05 with no EXTRUDER qualifier") { REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("SET_PRESSURE_ADVANCE")); - REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("ADVANCE=0.05")); + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("ADVANCE=0.05; Override")); REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("EXTRUDER=")); } } WHEN("set_pressure_advance is called with tool=0") { std::string out = writer.set_pressure_advance(0.04, 0); THEN("Output targets EXTRUDER=extruder (no trailing index)") { - REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("EXTRUDER=extruder ")); + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("EXTRUDER=extruder;")); REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("EXTRUDER=extruder0")); } } @@ -865,21 +865,20 @@ SCENARIO("set_pressure_advance emits Klipper form with optional EXTRUDER=extrude } } -SCENARIO("set_pressure_advance emits RepRapFirmware form with optional D", "[GCodeWriter][PressureAdvance]") { +SCENARIO("set_pressure_advance emits RepRapFirmware form with D", "[GCodeWriter][PressureAdvance]") { GIVEN("An RRF-flavored GCodeWriter") { GCodeWriter writer; writer.config.gcode_flavor.value = gcfRepRapFirmware; WHEN("set_pressure_advance is called without a tool index") { std::string out = writer.set_pressure_advance(0.07); - THEN("Output is bare M572 S... with no D qualifier") { - REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M572 S0.07")); - REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" D")); + THEN("Output keeps the historical D0 rather than depending on the selected tool") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M572 D0 S0.07")); } } WHEN("set_pressure_advance is called with tool=0") { std::string out = writer.set_pressure_advance(0.08, 0); - THEN("Output contains D0 (explicit tool 0, not the current-tool fallback)") { + THEN("Output contains D0, same as the tool-less form") { REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M572 D0 S0.08")); } } @@ -900,7 +899,7 @@ SCENARIO("set_pressure_advance emits Marlin 2.x form with optional T", "[GCod WHEN("set_pressure_advance is called without a tool index") { std::string out = writer.set_pressure_advance(0.10); THEN("Output is bare M900 K... with no T qualifier") { - REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.1 ")); + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.1; Override")); REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T")); } }