From d5d940d6b47eb151df12da2ae19846003e0cbe3b Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Tue, 29 Sep 2026 22:44:22 -0400 Subject: [PATCH] Cover the per-carriage pressure advance and temperature emissions In a parallel mode no tool changes occur, so two loops address each active carriage explicitly: pressure advance before the print, and the second-layer drop off the initial-layer temperature. Neither had coverage, and both fail silently -- a carriage missing from one emits nothing at all, so it holds the initial-layer temperature for the whole job, or runs on whatever pressure advance the firmware was last given. The new case pins that the carriages addressed are exactly the ones the mode declares active, each with the values of the slot physical_extruder_map routes its head to. The test needs filament_self_index, set here on imex_7x4_printer() so the whole file has it. Production authors that key 1..n; its all-1s default collapses every per-filament vector to filament 1's value through get_config_index_base(), which leaves a per-slot assertion comparing a value against itself. Also bounds the second-layer loop on filament_diameter alone. The bound belongs in slot space, and filament_diameter is the one per-filament vector never expanded per variant; the previous min() against nozzle_temperature mixed the two index spaces without changing the result. The pressure advance loop already bounds this way, so the two now read alike. Co-Authored-By: Claude Opus 5 (1M context) --- src/libslic3r/GCode.cpp | 20 +++---- tests/fff_print/test_imex_mode_gcode.cpp | 69 ++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/libslic3r/GCode.cpp b/src/libslic3r/GCode.cpp index c7c2721ed1..78b4300d87 100644 --- a/src/libslic3r/GCode.cpp +++ b/src/libslic3r/GCode.cpp @@ -4026,10 +4026,10 @@ void GCode::_do_export(Print& print, GCodeOutputStream &file, ThumbnailsGenerato (int) initial_extruder_id < (int) m_config.physical_extruder_map.values.size()) ? m_config.physical_extruder_map.values[(int) initial_extruder_id] : -1; - // enable_pressure_advance and pressure_advance are variant-expanded, so their - // length is columns, not filament slots. Bound in slot space -- filament_diameter - // is one entry per slot -- exactly as the second-layer temperature loop does, then - // translate the slot to its column with get_filament_config_index(). + // enable_pressure_advance and pressure_advance are indexed by COLUMN, not by + // filament slot. filament_diameter is one entry per slot and is never + // variant-expanded, so it is the slot-space bound; translate the slot to its column + // with get_filament_config_index(), as the second-layer temperature loop below does. const int num_pa_filament_slots = (int) print.config().filament_diameter.values.size(); for (int tool_idx : get_imex_active_tools(print)) { // Unlike the second-layer temperature loop, the primary is skipped here: @@ -6065,11 +6065,11 @@ LayerResult GCode::process_layer( // Mutually exclusive with the `else` below, so a head skipped here gets no // transition at all. `tool_idx` is physical; the printing head uses this layer's // own filament, the parallel carriages resolve through the head map. - // nozzle_temperature is variant-expanded, so its length is columns, not slots: - // bound in slot space, or an out-of-slot logical reaches get_filament_config_index - // and comes back as filament 0. - const int num_filament_columns = std::min((int) print.config().nozzle_temperature.values.size(), - (int) print.config().filament_diameter.values.size()); + // nozzle_temperature is indexed by column: bound in slot space, or an out-of-slot + // logical reaches get_filament_config_index and comes back as filament 0. + // filament_diameter is the slot-space yardstick -- one entry per filament slot, + // never variant-expanded -- and is the bound the pressure advance loop uses too. + const int num_filament_slots = (int) print.config().filament_diameter.values.size(); // Bounds-checked, not get_at() -- see IMEXHelpers.hpp. A clamp would hand the // "initial" branch below to whichever secondary sits on pem[0], giving it the wrong // filament's transition temperature and never its own. @@ -6083,7 +6083,7 @@ LayerResult GCode::process_layer( ? (int)first_extruder_id : resolve_filament_for_head( m_imex_head_filament_map, m_config.physical_extruder_map, tool_idx); - if (logical < 0 || logical >= num_filament_columns) continue; + if (logical < 0 || logical >= num_filament_slots) continue; // Variant-expanded printers column each filament; index as the `else` does. int temperature = print.config().nozzle_temperature.get_at(get_filament_config_index(logical)); if (temperature > 0) diff --git a/tests/fff_print/test_imex_mode_gcode.cpp b/tests/fff_print/test_imex_mode_gcode.cpp index d09feb93c4..04643864e7 100644 --- a/tests/fff_print/test_imex_mode_gcode.cpp +++ b/tests/fff_print/test_imex_mode_gcode.cpp @@ -50,6 +50,25 @@ static bool has_line(const std::string &gcode, const std::string &expected) return find_exact_line(gcode, expected) != std::string::npos; } +// Any command line -- not a comment -- containing `token`. Used where the assertion is that a +// carriage was addressed at all, rather than with a particular value: the config block the +// exporter appends restates every setting as "; key = value", so the comment lines have to be +// excluded or a search for a setting's own name matches itself. +static bool has_command_containing(const std::string &gcode, const std::string &token) +{ + std::size_t pos = 0; + while (pos < gcode.size()) { + const std::size_t eol = gcode.find('\n', pos); + const std::string line = gcode.substr(pos, (eol == std::string::npos ? gcode.size() : eol) - pos); + if (!line.empty() && line.front() != ';' && line.find(token) != std::string::npos) + return true; + if (eol == std::string::npos) + break; + pos = eol + 1; + } + return false; +} + // Mode scripts. Free of placeholders so that a test can tell "the script was emitted" apart // from "the script was expanded"; the expansion case below supplies its own template. static const char *kCopyScript = "SET_DUAL_CARRIAGE MODE=COPY"; @@ -79,6 +98,12 @@ static void imex_7x4_printer(DynamicPrintConfig &config) "Direct Drive Standard" }, { "extruder_printable_height", "0,0,0,0,0,0,0" }, { "physical_extruder_map", "0,0,0,0,1,2,3" }, + // Authored 1..n, as every production path does: PresetBundle writes it in + // full_fff_config(), and config load synthesizes it when absent. The all-1s default is + // not a neutral placeholder -- get_config_index_base() keys the slot-to-column lookup + // on it, and with every entry equal each per-filament vector collapses to filament 1's + // value, leaving a per-slot assertion comparing a value against itself. + { "filament_self_index", "1,2,3,4,5,6,7" }, { "is_imex", "1" }, { "imex_mode_names", "primary;copy;iq-copy" }, { "imex_mode_active_tools", "0:P;0:P,1:C;0:P,1:C,2:C,3:C" }, @@ -315,3 +340,47 @@ TEST_CASE("IMEX mode placeholders are defined on an ordinary single-extruder pri CHECK(has_line(gcode, ";IMEX_MODE_INDEX:0")); CHECK(has_line(gcode, ";IMEX_MODE_GCODE:")); } + +// In a parallel mode no tool changes occur, so every active carriage has to be addressed +// explicitly: once before the print for pressure advance, and again at the second layer for the +// drop from the initial-layer temperature. Nothing else in the suite covers either emission, and +// a carriage that falls out of one of those loops emits nothing at all rather than emitting +// something wrong -- it silently holds the initial-layer temperature, or whatever pressure +// advance the firmware was last told, for the whole job. So the property worth pinning is that +// the set of carriages addressed is exactly the set the mode declares active. +TEST_CASE("Every carriage a parallel mode declares active is addressed, and no other", + "[ImexModeGcode][IMEX]") +{ + DynamicPrintConfig config = multifilament_config(7); + imex_7x4_printer(config); + all_regions_on_filament(config, 1); + set_mode_gcodes(config, { "", kCopyScript, kQuadScript }); + config.set_deserialize_strict({ + { "imex_parallel_mode", "copy" }, + { "enable_pressure_advance", "1,1,1,1,1,1,1" }, + // Distinct per slot, so each assertion below names one filament and no other. copy + // mode's secondary rides physical 1, which physical_extruder_map routes to the fifth + // slot -- 0.05 and 244, not the primary's 0.01 and 240. + { "pressure_advance", "0.010,0.020,0.030,0.040,0.050,0.060,0.070" }, + { "nozzle_temperature_initial_layer", "235,235,235,235,235,235,235" }, + { "nozzle_temperature", "240,241,242,243,244,245,246" }, + }); + + const std::string gcode = slice({ cube(20) }, config); + + // imex_mode_active_tools declares copy as "0:P,1:C", so exactly these two carriages run, and + // each transitions to the temperature of the filament its own head is routed to. The other + // two heads this printer has must stay untouched. + CHECK(has_command_containing(gcode, "M104 S240 T0 ; set IMEX tool temperature")); + CHECK(has_command_containing(gcode, "M104 S244 T1 ; set IMEX tool temperature")); + CHECK_FALSE(has_command_containing(gcode, "T2 ; set IMEX tool temperature")); + CHECK_FALSE(has_command_containing(gcode, "T3 ; set IMEX tool temperature")); + + // The companion loop, which skips the primary because set_extruder() has already emitted it + // on the ordinary path. The qualified line is therefore the one that belongs to this loop: + // Klipper names the first carriage as the unnumbered "extruder" and the rest by index, so + // dropping the secondary removes `extruder1` and leaves `extruder` in place. + CHECK(has_command_containing(gcode, "SET_PRESSURE_ADVANCE ADVANCE=0.05 EXTRUDER=extruder1;")); + CHECK(has_command_containing(gcode, "SET_PRESSURE_ADVANCE ADVANCE=0.01 EXTRUDER=extruder;")); + CHECK_FALSE(has_command_containing(gcode, "EXTRUDER=extruder2;")); +}