From 1f062db75abe5e643562f2a434d9d1efca7f87b1 Mon Sep 17 00:00:00 2001 From: Mitch Bradley Date: Tue, 11 Jun 2024 12:21:35 -1000 Subject: [PATCH] Fix options parser (#1240) * clang-format cleanup - no substantive changes * Eliminate redundant ] at end of $SS lines * Cleaner code for string_view IOStream << operator * Fix pin options error report format glitch ... while changing PinOptionsParser to use string_view instead of explicit start and end pointers for substrings. * Fixed typo * Fixed test suite and a bug that it found --- FluidNC/esp32/StartupLog.cpp | 3 + FluidNC/src/MyIOStream.h | 4 +- FluidNC/src/Pin.cpp | 2 +- FluidNC/src/Pins/GPIOPinDetail.cpp | 10 ++- FluidNC/src/Pins/I2SOPinDetail.cpp | 12 ++- FluidNC/src/Pins/PinOptionsParser.cpp | 111 +++++++------------------ FluidNC/src/Pins/PinOptionsParser.h | 37 ++++----- FluidNC/src/Protocol.cpp | 3 +- FluidNC/src/string_util.h | 1 + FluidNC/tests/PinOptionsParserTest.cpp | 70 +++++----------- platformio.ini | 2 +- 11 files changed, 89 insertions(+), 166 deletions(-) diff --git a/FluidNC/esp32/StartupLog.cpp b/FluidNC/esp32/StartupLog.cpp index b16d01c74..a1e3517c8 100644 --- a/FluidNC/esp32/StartupLog.cpp +++ b/FluidNC/esp32/StartupLog.cpp @@ -46,6 +46,9 @@ void StartupLog::dump(Channel& out) { } line += c; } + if (!line.empty() && line.back() == ']') { + line.pop_back(); + } log_stream(out, line); } } diff --git a/FluidNC/src/MyIOStream.h b/FluidNC/src/MyIOStream.h index 8e155391d..9513d0c10 100644 --- a/FluidNC/src/MyIOStream.h +++ b/FluidNC/src/MyIOStream.h @@ -23,9 +23,7 @@ inline Print& operator<<(Print& lhs, const char* v) { } inline Print& operator<<(Print& lhs, const std::string_view& v) { - for (const char* p = v.cbegin(); p < v.cend(); ++p) { - lhs.print(*p); - } + lhs.write(reinterpret_cast(v.data()), v.length()); return lhs; } diff --git a/FluidNC/src/Pin.cpp b/FluidNC/src/Pin.cpp index 639309d52..e5fca2c09 100644 --- a/FluidNC/src/Pin.cpp +++ b/FluidNC/src/Pin.cpp @@ -70,7 +70,7 @@ const char* Pin::parse(std::string_view pin_str, Pins::PinDetail*& pinImplementa } // Build an options parser: - Pins::PinOptionsParser parser(pin_str.cbegin(), pin_str.cend()); + Pins::PinOptionsParser parser(pin_str); // Build this pin: if (string_util::equal_ignore_case(prefix, "gpio")) { diff --git a/FluidNC/src/Pins/GPIOPinDetail.cpp b/FluidNC/src/Pins/GPIOPinDetail.cpp index 0973f3e73..49bc8c42c 100644 --- a/FluidNC/src/Pins/GPIOPinDetail.cpp +++ b/FluidNC/src/Pins/GPIOPinDetail.cpp @@ -115,7 +115,7 @@ namespace Pins { } else if (opt.is("high")) { // Default: Active HIGH. } else { - Assert(false, "Bad GPIO option passed to pin %d: %s", int(index), opt()); + Assert(false, "Bad GPIO option passed to pin %d: %.*s", int(index), static_cast(opt().length()), opt().data()); } } _claimed[index] = true; @@ -124,9 +124,13 @@ namespace Pins { _readWriteMask = int(_attributes.has(PinAttributes::ActiveLow)); } - PinAttributes GPIOPinDetail::getAttr() const { return _attributes; } + PinAttributes GPIOPinDetail::getAttr() const { + return _attributes; + } - PinCapabilities GPIOPinDetail::capabilities() const { return _capabilities; } + PinCapabilities GPIOPinDetail::capabilities() const { + return _capabilities; + } void IRAM_ATTR GPIOPinDetail::write(int high) { if (high != _lastWrittenValue) { diff --git a/FluidNC/src/Pins/I2SOPinDetail.cpp b/FluidNC/src/Pins/I2SOPinDetail.cpp index 02fad8e6a..26904f921 100644 --- a/FluidNC/src/Pins/I2SOPinDetail.cpp +++ b/FluidNC/src/Pins/I2SOPinDetail.cpp @@ -22,16 +22,18 @@ namespace Pins { } else if (opt.is("high")) { // Default: Active HIGH. } else { - Assert(false, "Unsupported I2SO option '%s'", opt()); + Assert(false, "Unsupported I2SO option '%.*s'", static_cast(opt().length()), opt().data()); } } _claimed[index] = true; // readWriteMask is xor'ed with the value to invert it if active low - _readWriteMask = _attributes.has(PinAttributes::ActiveLow); + _readWriteMask = int(_attributes.has(PinAttributes::ActiveLow)); } - PinCapabilities I2SOPinDetail::capabilities() const { return PinCapabilities::Output | PinCapabilities::I2S; } + PinCapabilities I2SOPinDetail::capabilities() const { + return PinCapabilities::Output | PinCapabilities::I2S; + } // The write will not happen immediately; the data is queued for // delivery to the serial shift register chain via DMA and a FIFO @@ -77,7 +79,9 @@ namespace Pins { i2s_out_write(_index, value.has(PinAttributes::InitialOn) ^ _readWriteMask); } - PinAttributes I2SOPinDetail::getAttr() const { return _attributes; } + PinAttributes I2SOPinDetail::getAttr() const { + return _attributes; + } std::string I2SOPinDetail::toString() { std::string s("I2SO."); diff --git a/FluidNC/src/Pins/PinOptionsParser.cpp b/FluidNC/src/Pins/PinOptionsParser.cpp index e94e82933..7a16dd3e5 100644 --- a/FluidNC/src/Pins/PinOptionsParser.cpp +++ b/FluidNC/src/Pins/PinOptionsParser.cpp @@ -2,95 +2,52 @@ // Use of this source code is governed by a GPLv3 license that can be found in the LICENSE file. #include "PinOptionsParser.h" +#include "../string_util.h" #include #include #include +#include namespace Pins { - PinOption::PinOption(const char* start, const char* end) : _start(start), _end(end), _key(start), _value(start) { tokenize(); } - - // Copy the value into a null-terminated string, converting to lower case - const char* PinOption::value() const { - static char str[100]; - - int valuelen = _valueend - _value; - if (valuelen > 100) { - valuelen = 99; - } - const char* p = _value; - int i; - for (i = 0; i < valuelen; i++) { - str[i] = ::tolower(*p++); - } - str[i] = '\0'; - return str; + PinOption::PinOption(const std::string_view options) : _options(options) { + tokenize(); } void PinOption::tokenize() { - if (_start != _end) { - _key = _start; - - auto i = _start; - for (; i != _end && (*i) != ':' && (*i) != ';' && (*i) != '='; ++i) {} - - if (i == _end) { - // [start, end> is a key; value is nul - _value = _end; - _valueend = _end; - _keyend = _end; - _start = i; - } else if (*i == '=') { - // Parsing a key-value pair. - // - // Mark end of the key, which is now in [start, end> - _keyend = i; - ++i; - - _value = i; - - // Parse the value: - for (; i != _end && (*i) != ':' && (*i) != ';'; ++i) {} + if (_options.length() == 0) { + _option = _key = _value = {}; + return; + } + auto pos = _options.find_first_of(":;"); + _option = _options.substr(0, pos); + if (pos == std::string_view::npos) { + _option = _options; + _options.remove_prefix(_options.size()); + } else { + _option = _options.substr(0, pos); + _options.remove_prefix(pos + 1); + } - _valueend = i; - if (i != _end) { - _start = i + 1; - } else { - _start = i; - } - } else { // must be ':' or ';' - // [start, i> is a key; value is nul - _keyend = _value = _valueend = i; - _start = i + 1; - } + pos = _option.find_first_of('='); + if (pos == std::string_view::npos) { + _key = _option; + _value = {}; } else { - // Both key and value are nul. - _key = _value = _end; - _keyend = _valueend = _end; + _key = _option.substr(0, pos); + _value = _option.substr(pos + 1); } } bool PinOption::is(const char* option) const { - const char* k = _key; - while (*option && k != _keyend) { - if (::tolower(*k++) != ::tolower(*option++)) { - return false; - } - } - // If we get here, we have reached the end of either option or key - // and the initial substrings match ignoring case. - // If we are at the end of both, we have a match - return !*option && k == _keyend; + return string_util::equal_ignore_case(_key, option); } int PinOption::iValue() const { // Parse to integer - return ::atoi(value()); - } - - double PinOption::dValue() const { - // Parse to integer - return ::atof(value()); + int num; + auto [ptr, ec] = std::from_chars(_value.data(), _value.data() + _value.length(), num); + return num; } PinOption& PinOption ::operator++() { @@ -98,17 +55,5 @@ namespace Pins { return *this; } - PinOptionsParser::PinOptionsParser(const char* buffer, const char* endBuffer) : _buffer(buffer), _bufferEnd(endBuffer) { - // trim whitespaces: - while (buffer != endBuffer && ::isspace(*buffer)) { - ++buffer; - } - if (buffer != endBuffer) { - while (buffer - 1 != endBuffer && ::isspace(endBuffer[-1])) { - --endBuffer; - } - } - _buffer = buffer; - _bufferEnd = endBuffer; - } + PinOptionsParser::PinOptionsParser(std::string_view options) : _options(string_util::trim(options)) {} } diff --git a/FluidNC/src/Pins/PinOptionsParser.h b/FluidNC/src/Pins/PinOptionsParser.h index 46d7c288b..7575cf2f8 100644 --- a/FluidNC/src/Pins/PinOptionsParser.h +++ b/FluidNC/src/Pins/PinOptionsParser.h @@ -2,6 +2,7 @@ // Use of this source code is governed by a GPLv3 license that can be found in the LICENSE file. #pragma once +#include namespace Pins { // Pin options are passed as PinOption object. This is a simple C++ forward iterator, @@ -23,45 +24,41 @@ namespace Pins { class PinOption { friend class PinOptionsParser; - const char* _start; - const char* _end; + std::string_view _options; + std::string_view _option; + std::string_view _key; + std::string_view _value; - const char* _key; - const char* _keyend; - const char* _value; - const char* _valueend; - - PinOption(const char* start, const char* end); + PinOption(const std::string_view options); void tokenize(); public: - inline const char* operator()() const { return _key; } - bool is(const char* option) const; + bool is(const char* option) const; - int iValue() const; - double dValue() const; + int iValue() const; - const char* value() const; + inline const std::string_view operator()() { return _option; } + inline const std::string_view value() { return _value; } + inline const std::string_view key() { return _key; } // Iterator support: inline PinOption const* operator->() const { return this; } inline PinOption operator*() const { return *this; } PinOption& operator++(); - bool operator==(const PinOption& o) const { return _key == o._key && _keyend == o._keyend; } - bool operator!=(const PinOption& o) const { return _key != o._key || _keyend != o._keyend; } + bool operator==(const PinOption& o) const { return _key == o._key; } + bool operator!=(const PinOption& o) const { return _key != o._key; } }; // This parses the options passed to the Pin class. class PinOptionsParser { - const char* _buffer; - const char* _bufferEnd; + std::string_view _options; public: - PinOptionsParser(const char* buffer, const char* endBuffer); + PinOptionsParser(std::string_view options); - inline PinOption begin() const { return PinOption(_buffer, _bufferEnd); } - inline PinOption end() const { return PinOption(_bufferEnd, _bufferEnd); } + inline PinOption begin() const { return PinOption(_options); } + inline PinOption end() const { return PinOption(std::string_view()); } }; } diff --git a/FluidNC/src/Protocol.cpp b/FluidNC/src/Protocol.cpp index a3a101626..4eacf26cc 100644 --- a/FluidNC/src/Protocol.cpp +++ b/FluidNC/src/Protocol.cpp @@ -185,7 +185,8 @@ const uint32_t heapWarnThreshold = 15000; uint32_t heapLowWater = UINT_MAX; uint32_t heapLowWaterReported = UINT_MAX; int32_t heapLowWaterReportTime = 0; -void protocol_main_loop() { + +void protocol_main_loop() { start_polling(); // --------------------------------------------------------------------------------- diff --git a/FluidNC/src/string_util.h b/FluidNC/src/string_util.h index e363344c3..4f4db5013 100644 --- a/FluidNC/src/string_util.h +++ b/FluidNC/src/string_util.h @@ -1,5 +1,6 @@ #pragma once +#include #include namespace string_util { diff --git a/FluidNC/tests/PinOptionsParserTest.cpp b/FluidNC/tests/PinOptionsParserTest.cpp index 696c864b5..e2d6c07c1 100644 --- a/FluidNC/tests/PinOptionsParserTest.cpp +++ b/FluidNC/tests/PinOptionsParserTest.cpp @@ -9,8 +9,7 @@ static void test_for_loop(PinOptionsParser& parser); static void test_for_loop_only_first(PinOptionsParser& parser); TEST(PinOptionsParser, WithEmptyString) { - char nullDescr[1] = { '\0' }; - PinOptionsParser parser(nullDescr, nullDescr); + PinOptionsParser parser(""); { auto opt = parser.begin(); @@ -29,17 +28,13 @@ TEST(PinOptionsParser, WithEmptyString) { } TEST(PinOptionsParser, SingleArg) { - const char* input = "first"; - char tmp[20]; - int n = snprintf(tmp, 20, "%s", input); - - PinOptionsParser parser(tmp, tmp + n); + PinOptionsParser parser("first"); { auto opt = parser.begin(); auto endopt = parser.end(); ASSERT_NE(opt, endopt) << "Expected an argument"; - ASSERT_TRUE(opt->is("first")) << "Expected 'first'"; + ASSERT_TRUE(opt.is("first")) << "Expected 'first'"; ++opt; ASSERT_EQ(opt, endopt) << "Expected one argument"; } @@ -48,17 +43,13 @@ TEST(PinOptionsParser, SingleArg) { } TEST(PinOptionsParser, SingleArgWithWS) { - const char* input = " first"; - char tmp[20]; - int n = snprintf(tmp, 20, "%s", input); - - PinOptionsParser parser(tmp, tmp + n); + PinOptionsParser parser(" first"); { auto opt = parser.begin(); auto endopt = parser.end(); ASSERT_NE(opt, endopt) << "Expected an argument"; - ASSERT_TRUE(opt->is("first")) << "Expected 'first'"; + ASSERT_TRUE(opt.is("first")) << "Expected 'first'"; ++opt; ASSERT_EQ(opt, endopt) << "Expected one argument"; } @@ -67,17 +58,12 @@ TEST(PinOptionsParser, SingleArgWithWS) { } TEST(PinOptionsParser, SingleArgWithWS2) { - const char* input = " first "; - char tmp[20]; - int n = snprintf(tmp, 20, "%s", input); - - PinOptionsParser parser(tmp, tmp + n); - + PinOptionsParser parser(" first "); { auto opt = parser.begin(); auto endopt = parser.end(); ASSERT_NE(opt, endopt) << "Expected an argument"; - ASSERT_TRUE(opt->is("first")) << "Expected 'first'"; + ASSERT_TRUE(opt.is("first")) << "Expected 'first'"; ++opt; ASSERT_EQ(opt, endopt) << "Expected one argument"; @@ -87,21 +73,16 @@ TEST(PinOptionsParser, SingleArgWithWS2) { } TEST(PinOptionsParser, TwoArg1) { - const char* input = "first;second"; - char tmp[20]; - int n = snprintf(tmp, 20, "%s", input); - - PinOptionsParser parser(tmp, tmp + n); - + PinOptionsParser parser("first;second"); { auto opt = parser.begin(); auto endopt = parser.end(); ASSERT_NE(opt, endopt) << "Expected an argument"; - ASSERT_TRUE(opt->is("first")) << "Expected 'first'"; + ASSERT_TRUE(opt.is("first")) << "Expected 'first'"; ++opt; ASSERT_NE(opt, endopt) << "Expected second argument"; - ASSERT_TRUE(opt->is("second")) << "Expected 'second'"; + ASSERT_TRUE(opt.is("second")) << "Expected 'second'"; ++opt; ASSERT_EQ(opt, endopt) << "Expected one argument"; @@ -111,21 +92,16 @@ TEST(PinOptionsParser, TwoArg1) { } TEST(PinOptionsParser, TwoArg2) { - const char* input = "first:second"; - char tmp[20]; - int n = snprintf(tmp, 20, "%s", input); - - PinOptionsParser parser(tmp, tmp + n); - + PinOptionsParser parser("first:second"); { auto opt = parser.begin(); auto endopt = parser.end(); ASSERT_NE(opt, endopt) << "Expected an argument"; - ASSERT_TRUE(opt->is("first")) << "Expected 'first'"; + ASSERT_TRUE(opt.is("first")) << "Expected 'first'"; ++opt; ASSERT_NE(opt, endopt) << "Expected second argument"; - ASSERT_TRUE(opt->is("second")) << "Expected 'second'"; + ASSERT_TRUE(opt.is("second")) << "Expected 'second'"; ++opt; ASSERT_EQ(opt, endopt) << "Expected one argument"; @@ -135,27 +111,21 @@ TEST(PinOptionsParser, TwoArg2) { } TEST(PinOptionsParser, TwoArgWithValues) { - const char* input = "first=12;second=13"; - char tmp[20]; - int n = snprintf(tmp, 20, "%s", input); - - PinOptionsParser parser(tmp, tmp + n); + PinOptionsParser parser("first=12;second=13"); { auto opt = parser.begin(); auto endopt = parser.end(); ASSERT_NE(opt, endopt) << "Expected an argument"; - ASSERT_TRUE(opt->is("first")) << "Expected 'first'"; - ASSERT_EQ(strcmp("12", opt->value()), 0); - ASSERT_EQ(12, opt->iValue()); - ASSERT_EQ(12, opt->dValue()); + ASSERT_TRUE(opt.is("first")) << "Expected 'first'"; + ASSERT_TRUE(opt.value() == "12"); + ASSERT_EQ(12, opt.iValue()); ++opt; ASSERT_NE(opt, endopt) << "Expected second argument"; - ASSERT_TRUE(opt->is("second")) << "Expected 'second'"; - ASSERT_EQ(strcmp("13", opt->value()), 0); - ASSERT_EQ(13, opt->iValue()); - ASSERT_EQ(13, opt->dValue()); + ASSERT_TRUE(opt.is("second")) << "Expected 'second'"; + ASSERT_TRUE(opt.value() == "13"); + ASSERT_EQ(13, opt.iValue()); ++opt; ASSERT_EQ(opt, endopt) << "Expected one argument"; diff --git a/platformio.ini b/platformio.ini index 4d0073956..bba69fd51 100644 --- a/platformio.ini +++ b/platformio.ini @@ -155,7 +155,7 @@ build_flags = ${common_esp32.build_flags} ${common_wifi.build_flags} ${common_b platform = native test_framework = googletest test_build_src = true -build_src_filter = + +build_src_filter = + + build_flags = -std=c++17 -g [env:tests]