MeatPack: Compact only G0-G3 lines.

src/LibBGCode/binarize/meatpack.cpp

@@ -3,6 +3,7 @@
 #include <unordered_map>
 #include <algorithm>
 #include <cassert>
+#include <charconv>
 #include <cstdlib>
 
 namespace MeatPack {
@@ -64,6 +65,49 @@ static std::string_view ltrim(const std::string_view& str)
     return start == std::string_view::npos ? std::string_view() : str.substr(start);
 }
 
+static std::string compact_gcode_line_for_packing(const std::string& line, bool uppercase_e)
+{
+    std::string result = line;
+    if (uppercase_e) {
+        std::replace(result.begin(), result.end(), 'e', 'E');
+    }
+
+    std::replace(result.begin(), result.end(), 'x', 'X');
+    std::replace(result.begin(), result.end(), 'g', 'G');
+    result.erase(std::remove(result.begin(), result.end(), ' '), result.end());
+    if (result.find('*') != std::string::npos) {
+        size_t checksum = 0;
+        result.erase(std::remove(result.begin(), result.end(), '*'), result.end());
+        for (const char line_char : result) {
+            checksum ^= static_cast<uint8_t>(line_char);
+        }
+
+        result += "*" + std::to_string(checksum);
+    }
+
+    result += '\n';
+    return result;
+}
+
+// Only G0 to G3 lines are compacted because the decoder reliably reinserts spaces only before their parameters.
+static std::string compact_gcode_line_if_it_contains_g_command(const std::string& line, bool uppercase_e)
+{
+    const std::string::size_type g_idx = line.find('G');
+    if (g_idx != std::string::npos) {
+        if (g_idx + 1 < line.size() && line[g_idx + 1] >= '0' && line[g_idx + 1] <= '9') {
+            int gcode_number = 0;
+            const std::from_chars_result gcode_number_result =
+                std::from_chars(line.data() + g_idx + 1, line.data() + line.size(), gcode_number);
+            const bool is_g0_to_g3_line = g_idx == 0 && gcode_number_result.ec == std::errc() && gcode_number <= 3;
+            if (is_g0_to_g3_line) {
+                return compact_gcode_line_for_packing(line, uppercase_e);
+            }
+        }
+    }
+
+    return line;
+}
+
 MPBinarizer::LookupTables MPBinarizer::s_lookup_tables = { { 0 }, { 0 }, false, 0 };
 
 MPBinarizer::MPBinarizer(uint8_t flags) : m_flags(flags) {}
@@ -88,47 +132,6 @@ void MPBinarizer::finalize(std::vector<uint8_t>& dst)
 
 bool MPBinarizer::binarize_line(const std::string& line, std::vector<uint8_t>& dst)
 {
-    auto unified_method = [this](const std::string& line) {
-        const std::string::size_type g_idx = line.find('G');
-        if (g_idx != std::string::npos) {
-            if (g_idx + 1 < line.size() && line[g_idx + 1] >= '0' && line[g_idx + 1] <= '9') {
-                if ((m_flags & Flag_OmitWhitespaces) != 0) {
-                    std::string result = line;
-                    std::replace(result.begin(), result.end(), 'e', 'E');
-                    std::replace(result.begin(), result.end(), 'x', 'X');
-                    std::replace(result.begin(), result.end(), 'g', 'G');
-                    result.erase(std::remove(result.begin(), result.end(), ' '), result.end());
-                    if (result.find('*') != std::string::npos) {
-                        size_t checksum = 0;
-                        result.erase(std::remove(result.begin(), result.end(), '*'), result.end());
-                        for (size_t i = 0; i < result.size(); ++i) {
-                            checksum ^= static_cast<uint8_t>(result[i]);
-                        }
-                        result += "*" + std::to_string(checksum);
-                    }
-                    result += '\n';
-                    return result;
-                }
-                else {
-                    std::string result = line;
-                    std::replace(result.begin(), result.end(), 'x', 'X');
-                    std::replace(result.begin(), result.end(), 'g', 'G');
-                    result.erase(std::remove(result.begin(), result.end(), ' '), result.end());
-                    if (result.find('*') != std::string::npos) {
-                        size_t checksum = 0;
-                        result.erase(std::remove(result.begin(), result.end(), '*'), result.end());
-                        for (size_t i = 0; i < result.size(); ++i) {
-                            checksum ^= static_cast<uint8_t>(result[i]);
-                        }
-                        result += "*" + std::to_string(checksum);
-                    }
-                    result += '\n';
-                    return result;
-                }
-            }
-        }
-        return line;
-    };
     auto is_packable = [](char c) {
         return (s_lookup_tables.packable[static_cast<uint8_t>(c)] != 0);
     };
@@ -164,11 +167,15 @@ bool MPBinarizer::binarize_line(const std::string& line, std::vector<uint8_t>& d
             return true;
 
         std::string modifiedLine = std::string(trim(std::string_view(line.substr(0, line.find(';')))));
-        if (modifiedLine.empty())
+        if (modifiedLine.empty()) {
             return true;
-        modifiedLine = unified_method(modifiedLine);
-        if (modifiedLine.back() != '\n')
+        }
+
+        modifiedLine = compact_gcode_line_if_it_contains_g_command(modifiedLine, (m_flags & Flag_OmitWhitespaces) != 0);
+        if (modifiedLine.back() != '\n') {
             modifiedLine.push_back('\n');
+        }
+
         // 0xFF is the decoder's signal byte; it cannot be represented in the packed output.
         if (modifiedLine.find('\xff') != std::string::npos)
             return false;
@@ -429,18 +436,19 @@ void unbinarize(const std::vector<uint8_t>& src, std::string& dst)
             if (c_unbin[i] == 'G' && (curr_unbin_buffer_length == 0 || *std::prev(it_unbin_end, 1) == '\n')) {
                 add_space = true;
                 new_line = true;
-            }
-            else if (c_unbin[i] == '\n')
+            } else if (c_unbin[i] == '\n' || c_unbin[i] == ' ') {
+                // A line containing a space was stored unchanged by the encoder.
                 add_space = false;
+            }
 
-            if (!new_line && add_space && (curr_unbin_buffer_length == 0 || *std::prev(it_unbin_end, 1) != ' ') &&
-                is_gline_parameter(c_unbin[i])) {
+            if (!new_line && add_space && is_gline_parameter(c_unbin[i])) {
                 *it_unbin_end = ' ';
                 ++it_unbin_end;
                 if (it_unbin_end == unbin_buffer.end()) {
                     // the buffer is not big enough, resize it
-                    unbin_buffer.resize(2 * unbin_buffer.size(), 0);
-                    it_unbin_end = unbin_buffer.begin() + curr_unbin_buffer_length + 1;
+                    const size_t unbin_buffer_length = unbin_buffer.size();
+                    unbin_buffer.resize(2 * unbin_buffer_length, 0);
+                    it_unbin_end = unbin_buffer.begin() + unbin_buffer_length;
                 }
             }
 
@@ -449,8 +457,9 @@ void unbinarize(const std::vector<uint8_t>& src, std::string& dst)
                 ++it_unbin_end;
                 if (it_unbin_end == unbin_buffer.end()) {
                     // the buffer is not big enough, resize it
-                    unbin_buffer.resize(2 * unbin_buffer.size(), 0);
-                    it_unbin_end = unbin_buffer.begin() + curr_unbin_buffer_length + 1;
+                    const size_t unbin_buffer_length = unbin_buffer.size();
+                    unbin_buffer.resize(2 * unbin_buffer_length, 0);
+                    it_unbin_end = unbin_buffer.begin() + unbin_buffer_length;
                 }
             }
         }

tests/binarize/CMakeLists.txt

@@ -5,6 +5,7 @@ add_executable(binarize_tests
     heatshrink_roundtrip_tests.cpp
     meatpack_encode_tests.cpp
     meatpack_overflow_tests.cpp
+    meatpack_whitespace_tests.cpp
     metadata_iterator_tests.cpp
 )
 

tests/binarize/meatpack_overflow_tests.cpp

@@ -54,3 +54,26 @@ TEST_CASE("reading a crafted MeatPack gcode block stays in bounds", "[Binarize][
 
     REQUIRE((result == EResult::Success || result == EResult::GCodeDecodingError));
 }
+
+TEST_CASE("Unbinarize keeps the parameter letter after a reinserted space when its output buffer grows", "[Binarize][MeatPack]") {
+    const std::string gcode_line = "G1 X10.5 E1.25\n";
+    for (size_t line_count = 1; line_count <= 64; ++line_count) {
+        CAPTURE(line_count);
+
+        MeatPack::MPBinarizer binarizer(MeatPack::Flag_OmitWhitespaces);
+        std::vector<uint8_t> binarized_data;
+        binarizer.initialize(binarized_data);
+
+        std::string gcode;
+        for (size_t i = 0; i < line_count; ++i) {
+            REQUIRE(binarizer.binarize_line(gcode_line, binarized_data));
+            gcode += gcode_line;
+        }
+
+        binarizer.finalize(binarized_data);
+
+        std::string out;
+        unbinarize(binarized_data, out);
+        CHECK(out == gcode);
+    }
+}

tests/binarize/meatpack_whitespace_tests.cpp

@@ -0,0 +1,123 @@
+#include <catch2/catch_test_macros.hpp>
+
+#include "binarize/meatpack.hpp"
+
+#include <cstdint>
+#include <string>
+#include <vector>
+
+static std::vector<uint8_t> binarize_gcode_line(const std::string& gcode_line, uint8_t meatpack_flags)
+{
+    MeatPack::MPBinarizer binarizer(meatpack_flags);
+    std::vector<uint8_t> binarized_data;
+    binarizer.initialize(binarized_data);
+    REQUIRE(binarizer.binarize_line(gcode_line, binarized_data));
+    binarizer.finalize(binarized_data);
+    return binarized_data;
+}
+
+static std::string unbinarize_to_text(const std::vector<uint8_t>& binarized_data)
+{
+    std::string text;
+    MeatPack::unbinarize(binarized_data, text);
+    return text;
+}
+
+static uint8_t four_bit_code_in_no_spaces_mode(char character)
+{
+    const size_t index = std::string("0123456789.E\nGX").find(character);
+    return index == std::string::npos ? 0xF : static_cast<uint8_t>(index);
+}
+
+// Written according to the MeatPack specification, independently of the library.
+static std::vector<uint8_t> expected_stream(const std::vector<std::string>& packed_lines)
+{
+    constexpr uint8_t EnablePackingCommand = 251;
+    constexpr uint8_t EnableNoSpacesCommand = 247;
+
+    std::vector<uint8_t> stream = { 0xFF, 0xFF, EnablePackingCommand, 0xFF, 0xFF, EnableNoSpacesCommand };
+    for (const std::string& packed_line : packed_lines) {
+        for (size_t i = 0; i < packed_line.size(); i += 2) {
+            const char first = packed_line[i];
+            const char second = (i + 1 < packed_line.size()) ? packed_line[i + 1] : '\n';
+            const uint8_t first_code = four_bit_code_in_no_spaces_mode(first);
+            const uint8_t second_code = four_bit_code_in_no_spaces_mode(second);
+
+            stream.push_back(static_cast<uint8_t>((second_code << 4) | first_code));
+
+            if (first_code == 0xF) {
+                stream.push_back(static_cast<uint8_t>(first));
+            }
+
+            if (second_code == 0xF) {
+                stream.push_back(static_cast<uint8_t>(second));
+            }
+        }
+    }
+
+    return stream;
+}
+
+struct CompactionCase
+{
+    std::string gcode_line;
+    std::string packed_line;
+    std::string packed_by_previous_versions;
+    std::string decoded_from_previous_versions;
+};
+
+static const std::vector<CompactionCase> compaction_cases = {
+    // gcode line                Packed                     Previous versions          Decoded from previous versions
+    { "M104 S200\n",             "M104 S200\n",             "M104 S200\n",             "M104 S200\n" },
+    { "G1 X10 Y10 E1.5 F3000\n", "G1X10Y10E1.5F3000\n\n",   "G1X10Y10E1.5F3000\n\n",   "G1 X10 Y10 E1.5 F3000\n" },
+    { "G01 X1 Y2\n",             "G01X1Y2\n\n",             "G01X1Y2\n\n",             "G01 X1 Y2\n" },
+    { "G2 X10 Y10 I5 J5\n",      "G2X10Y10I5J5\n\n",        "G2X10Y10I5J5\n\n",        "G2 X10 Y10 I5 J5\n" },
+    { "G10\n",                   "G10\n",                   "G10\n\n",                 "G10\n" },
+    { "G28 W\n",                 "G28 W\n",                 "G28W\n\n",                "G28 W\n" },
+    { "G28 XY\n",                "G28 XY\n",                "G28XY\n\n",               "G28 X Y\n" },
+    { "N5 G1 X10\n",             "N5 G1 X10\n",             "N5G1X10\n\n",             "N5G1X10\n" },
+    { "G12 P1 S3 T5\n",          "G12 P1 S3 T5\n",          "G12P1S3T5\n\n",           "G12 P1 S3T5\n" },
+    { "G12 Quick clean\n",       "G12 Quick clean\n",       "G12QuickclEan\n\n",       "G12Quickcl Ean\n" },
+    { "G12 Quick_Clean\n",       "G12 Quick_Clean\n",       "G12Quick_ClEan\n\n",      "G12Quick_ Cl Ean\n" },
+    { "G12 QUICK STOP\n",        "G12 QUICK STOP\n",        "G12QUICKSTOP\n\n",        "G12QU I CK STO P\n" },
+    { "G FS\n",                  "G FS\n",                  "G FS\n",                  "G FS\n" },
+    { "g1 x10 y10\n",            "g1 x10 y10\n",            "g1 x10 y10\n",            "g1 x10 y10\n" },
+};
+
+TEST_CASE("MeatPack compacts only G0-G3 lines", "[Binarize][MeatPack]")
+{
+    for (const CompactionCase& compaction_case : compaction_cases) {
+        CAPTURE(compaction_case.gcode_line);
+        CHECK(binarize_gcode_line(compaction_case.gcode_line, MeatPack::Flag_OmitWhitespaces) == expected_stream({ compaction_case.packed_line }));
+    }
+}
+
+TEST_CASE("MeatPack decodes every line back exactly", "[Binarize][MeatPack]")
+{
+    std::vector<std::string> packed_lines;
+    std::string gcode_lines;
+    for (const CompactionCase& compaction_case : compaction_cases) {
+        CAPTURE(compaction_case.gcode_line);
+        CHECK(unbinarize_to_text(binarize_gcode_line(compaction_case.gcode_line, MeatPack::Flag_OmitWhitespaces)) == compaction_case.gcode_line);
+
+        packed_lines.push_back(compaction_case.packed_line);
+        gcode_lines += compaction_case.gcode_line;
+    }
+
+    CHECK(unbinarize_to_text(expected_stream(packed_lines)) == gcode_lines);
+}
+
+TEST_CASE("MeatPack decodes streams of previous versions as before", "[Binarize][MeatPack]")
+{
+    std::vector<std::string> packed_lines;
+    std::string decoded_lines;
+    for (const CompactionCase& compaction_case : compaction_cases) {
+        CAPTURE(compaction_case.gcode_line);
+        CHECK(unbinarize_to_text(expected_stream({ compaction_case.packed_by_previous_versions })) == compaction_case.decoded_from_previous_versions);
+
+        packed_lines.push_back(compaction_case.packed_by_previous_versions);
+        decoded_lines += compaction_case.decoded_from_previous_versions;
+    }
+
+    CHECK(unbinarize_to_text(expected_stream(packed_lines)) == decoded_lines);
+}