clean up

src/libslic3r/GCode/WipeTower.cpp

@@ -1345,6 +1345,8 @@ public:
     {
         std::string buffer;
         if (wait_for_moves)
+            // Not flush_planner_queue_command(): this BBL precool path wants M400, which every
+            // flavor it reaches understands, not the zero dwell the other flavors flush with.
             buffer += "M400\n";
         buffer += "M104";
         if (target_extruder != -1)

src/libslic3r/GCode/WipeTower.hpp

@@ -26,16 +26,11 @@ enum GCodeFlavor : unsigned char;
 Polylines construct_gap_for_skip_points(
     const Polygon& polygon, const std::vector<Vec2f>& skip_points, float wt_width, float gap_length, Polygon& insert_skip_polygon);
 
-// Returns the command that makes the firmware finish its queued moves around an M104/M109
+// Klipper acts on commands the instant it parses them, and its G4 reads only P (milliseconds),
-// or custom-G-code boundary. Klipper acts on commands the instant it parses them, and its G4
+// so the zero-second and seconds-valued dwells every other flavor uses neither synchronize nor
-// reads only P, so the zero dwell other flavors use synchronizes nothing there — M400 does.
+// pause there. Both defined in WipeTower.cpp, shared by WipeTower and WipeTower2.
-// Defined in WipeTower.cpp, shared by WipeTower and WipeTower2.
+const char* flush_planner_queue_command(GCodeFlavor flavor); // finish queued moves, e.g. around M104/M109
-const char* flush_planner_queue_command(GCodeFlavor flavor);
+std::string wait_command(GCodeFlavor flavor, float seconds);  // pause for `seconds`
-
-// Returns the command that pauses for `seconds`. Klipper's G4 reads only P, in
-// milliseconds, and ignores S, so the seconds form the other flavors use would dwell zero
-// there. Defined in WipeTower.cpp, shared by WipeTower and WipeTower2.
-std::string wait_command(GCodeFlavor flavor, float seconds);
 
 class WipeTower
 {

src/libslic3r/GCode/WipeTower2.cpp

@@ -386,8 +386,8 @@ public:
     }
 
     WipeTowerWriter2& switch_filament_monitoring(bool enable) {
-        m_gcode += flush_planner_queue_command(m_gcode_flavor);
+        flush_planner_queue();
-        m_gcode += std::string("M591 ") + (enable ? "R" : "S0") + "\n";
+        m_gcode += enable ? "M591 R\n" : "M591 S0\n";
         return *this;
     }
 
@@ -626,7 +626,7 @@ public:
 	// Set extruder temperature, don't wait by default.
 	WipeTowerWriter2& set_extruder_temp(int temperature, bool wait = false)
 	{
-        m_gcode += flush_planner_queue_command(m_gcode_flavor);
+        flush_planner_queue();
         m_gcode += "M" + std::to_string(wait ? 109 : 104) + " S" + std::to_string(temperature) + "\n";
         return *this;
     }

tests/fff_print/test_wipe_tower.cpp

@@ -1,7 +1,9 @@
 #include <catch2/catch_all.hpp>
 
 #include <string>
+#include <vector>
 
+#include "libslic3r/GCode/GCodeProcessor.hpp"
 #include "libslic3r/GCode/WipeTower.hpp"
 #include "libslic3r/PrintConfig.hpp"
 
@@ -10,13 +12,22 @@
 using namespace Slic3r;
 using namespace Slic3r::Test;
 
-// Pins the enum's size: the two GENERATE lists below hand-list every non-Klipper flavor, so a
+// Taken from the config enum map rather than hand-listed, so a flavor added to GCodeFlavor later
-// 14th `GCodeFlavor` value would silently go untested unless this fails the build first.
+// is covered here without editing this file.
-static_assert(int(gcfNoExtrusion) == 12, "GCodeFlavor grew: add the new value to the GENERATE lists in this file");
+static std::vector<GCodeFlavor> non_klipper_flavors()
+{
+    std::vector<GCodeFlavor> flavors;
+    for (const auto &[name, value] : ConfigOptionEnum<GCodeFlavor>::get_enum_values())
+        if (GCodeFlavor(value) != gcfKlipper)
+            flavors.push_back(GCodeFlavor(value));
+    return flavors;
+}
+
+static std::string flavor_name(GCodeFlavor flavor)
+{
+    return ConfigOptionEnum<GCodeFlavor>::get_enum_names()[int(flavor)];
+}
 
-// The wipe tower flushes the firmware's motion queue around an M104/M109 or custom-G-code
-// boundary. Klipper acts on those the moment it parses them, and its G4 reads only P, so the
-// zero dwell every other flavor uses is not a flush there.
 TEST_CASE("Klipper flushes the wipe tower planner queue with M400", "[WipeTower]")
 {
     CHECK(std::string(flush_planner_queue_command(gcfKlipper)) == "M400\n");
@@ -24,16 +35,11 @@ TEST_CASE("Klipper flushes the wipe tower planner queue with M400", "[WipeTower]
 
 TEST_CASE("Other flavors flush the wipe tower planner queue with a zero dwell", "[WipeTower]")
 {
-    const GCodeFlavor flavor = GENERATE(gcfMarlinLegacy, gcfRepRapFirmware, gcfRepetier,
+    const GCodeFlavor flavor = GENERATE(from_range(non_klipper_flavors()));
-                                        gcfMarlinFirmware, gcfRepRapSprinter, gcfTeacup,
+    INFO("gcode flavor: " << flavor_name(flavor));
-                                        gcfMakerWare, gcfSailfish, gcfMach3, gcfMachinekit,
-                                        gcfSmoothie, gcfNoExtrusion);
-    INFO("gcode flavor enum value: " << int(flavor));
     CHECK(std::string(flush_planner_queue_command(flavor)) == "G4 S0\n");
 }
 
-// A timed pause is emitted in seconds for most firmware. Klipper's G4 reads only P, in
-// milliseconds, and ignores S, so the seconds form would pause for no time at all there.
 // 1.5s is exactly representable as a float, so neither form can drift when rounded.
 TEST_CASE("Klipper waits in the wipe tower with a millisecond dwell", "[WipeTower]")
 {
@@ -42,44 +48,39 @@ TEST_CASE("Klipper waits in the wipe tower with a millisecond dwell", "[WipeTowe
 
 TEST_CASE("Other flavors wait in the wipe tower with a seconds dwell", "[WipeTower]")
 {
-    const GCodeFlavor flavor = GENERATE(gcfMarlinLegacy, gcfRepRapFirmware, gcfRepetier,
+    const GCodeFlavor flavor = GENERATE(from_range(non_klipper_flavors()));
-                                        gcfMarlinFirmware, gcfRepRapSprinter, gcfTeacup,
+    INFO("gcode flavor: " << flavor_name(flavor));
-                                        gcfMakerWare, gcfSailfish, gcfMach3, gcfMachinekit,
-                                        gcfSmoothie, gcfNoExtrusion);
-    INFO("gcode flavor enum value: " << int(flavor));
     CHECK(wait_command(flavor, 1.5f) == "G4 S1.500\n");
 }
 
-// The two helpers above are only unit-tested in isolation. Nothing yet confirms that a
+// The cases above only exercise the helpers in isolation. The one below slices a real
-// Klipper `gcode_flavor` actually reaches the wipe tower writer and lands in the exported
+// two-filament print, so it also covers the binding constraint of both changes: that the
-// G-code, which is the binding constraint of both changes above ("only gcfKlipper changes").
+// configured `gcode_flavor` reaches the wipe tower writer and lands in the exported G-code.
-// These slice a real two-filament print and check that.
 
-// The G-code between each "WIPE_TOWER_START"/"WIPE_TOWER_END" tag pair the wipe tower writes
+// The G-code inside each WIPE_TOWER_START/WIPE_TOWER_END pair, concatenated, so an M400 emitted
-// around its toolchange chunks, concatenated. Isolates the region the flush/dwell helpers can
+// outside the tower (e.g. GCodeProcessor's pre-heat injector) cannot create a false match.
-// emit into from ordinary object G-code, where an unrelated M400 (e.g. GCodeProcessor's
-// pre-heat injector, gated off here since neither test sets enable_pre_heating) would
-// otherwise create a false match.
 static std::string wipe_tower_regions(const std::string &gcode)
 {
+    const std::string &start_tag = GCodeProcessor::reserved_tag(GCodeProcessor::ETags::Wipe_Tower_Start);
+    const std::string &end_tag   = GCodeProcessor::reserved_tag(GCodeProcessor::ETags::Wipe_Tower_End);
     std::string regions;
     size_t pos = 0;
     while (true) {
-        size_t start = gcode.find("WIPE_TOWER_START", pos);
+        size_t start = gcode.find(start_tag, pos);
         if (start == std::string::npos)
             break;
-        size_t end = gcode.find("WIPE_TOWER_END", start);
+        size_t end = gcode.find(end_tag, start);
         if (end == std::string::npos)
             break;
-        regions += gcode.substr(start, end - start);
+        regions.append(gcode, start, end - start);
         pos = end + 1;
     }
     return regions;
 }
 
 // A per-layer toolchange between the wall and infill filaments, same shape as
-// test_multifilament.cpp's "Each feature prints with its assigned filament", so the wipe
+// test_multifilament.cpp's "Each feature prints with its assigned filament", so the wipe tower
-// tower actually runs its toolchange path (and so `flush_planner_queue()`) on every layer.
+// runs its toolchange path (and so `flush_planner_queue()`) on every layer.
 static DynamicPrintConfig wipe_tower_toolchange_config(const std::string &gcode_flavor)
 {
     return multifilament_config(2, {
@@ -90,41 +91,33 @@ static DynamicPrintConfig wipe_tower_toolchange_config(const std::string &gcode_
         { "outer_wall_filament_id",     2 },
         { "inner_wall_filament_id",     2 },
         { "enable_prime_tower",         true },
+        { "layer_height",               0.3 },
         { "gcode_flavor",               gcode_flavor },
     });
 }
 
-// Slices a 20mm cube under `config`. Not just `Test::slice(...)`: a brand-new Print's first
+// Slices a 10mm cube under `config`. Not plain Test::slice: a brand-new Print's first `apply()`
-// `apply()` call still has no per-feature regions built, so it undercounts the filaments in
+// counts one filament in use, and DynamicPrintConfig::normalize_fdm_2's single-filament rule then
-// use and lets DynamicPrintConfig::normalize_fdm_2's "single filament" rule turn
+// clears `enable_prime_tower`. A second apply, once init_print's regions have settled, sees both
-// `enable_prime_tower` back off before the wipe tower ever runs. Applying the same config a
+// filaments and the tower survives.
-// second time, once init_print's first apply has settled those regions, lets that count see
-// both filaments so the prime tower stays on.
 static std::string slice_with_prime_tower(const DynamicPrintConfig &config)
 {
     Print print;
     Model model;
-    init_print({ cube(20) }, print, model, config);
+    init_print({ cube(10) }, print, model, config);
     print.apply(model, config);
     return gcode(print);
 }
 
-TEST_CASE("Klipper's wipe tower toolchanges flush the planner queue with M400 in exported G-code", "[WipeTower]")
+TEST_CASE("The wipe tower's toolchange planner flush follows the gcode flavor", "[WipeTower]")
 {
-    const std::string gcode = slice_with_prime_tower(wipe_tower_toolchange_config("klipper"));
+    auto [flavor, expected, unexpected] = GENERATE(table<std::string, std::string, std::string>({
-    REQUIRE_THAT(gcode, Catch::Matchers::ContainsSubstring("WIPE_TOWER_START"));
+        { "klipper", "M400",  "G4 S0" },
-
+        { "marlin",  "G4 S0", "M400"  } }));
-    const std::string tower = wipe_tower_regions(gcode);
+    DYNAMIC_SECTION(flavor) {
-    CHECK_THAT(tower, Catch::Matchers::ContainsSubstring("M400"));
+        const std::string tower = wipe_tower_regions(slice_with_prime_tower(wipe_tower_toolchange_config(flavor)));
-    CHECK_THAT(tower, !Catch::Matchers::ContainsSubstring("G4 S0"));
+        REQUIRE_FALSE(tower.empty());
-}
+        CHECK_THAT(tower, Catch::Matchers::ContainsSubstring(expected));
-
+        CHECK_THAT(tower, !Catch::Matchers::ContainsSubstring(unexpected));
-TEST_CASE("Marlin's wipe tower toolchanges keep the zero-dwell flush in exported G-code", "[WipeTower]")
+    }
-{
-    const std::string gcode = slice_with_prime_tower(wipe_tower_toolchange_config("marlin"));
-    REQUIRE_THAT(gcode, Catch::Matchers::ContainsSubstring("WIPE_TOWER_START"));
-
-    const std::string tower = wipe_tower_regions(gcode);
-    CHECK_THAT(tower, Catch::Matchers::ContainsSubstring("G4 S0"));
-    CHECK_THAT(tower, !Catch::Matchers::ContainsSubstring("M400"));
 }