From 527e05d680cfa43e86c43419d301378da64eae08 Mon Sep 17 00:00:00 2001 From: Mitch Bradley Date: Thu, 31 Oct 2024 05:58:21 -1000 Subject: [PATCH] Revert "Allow rewinding of commands sent via UART buffer" (#1365) --- FluidNC/src/FixedCircularBuffer.h | 61 ------------------- FluidNC/src/Flowcontrol.cpp | 8 +-- FluidNC/src/Job.cpp | 32 +++------- FluidNC/src/Job.h | 1 - FluidNC/src/Protocol.h | 2 - FluidNC/src/UartChannel.cpp | 21 +------ FluidNC/src/UartChannel.h | 10 +-- FluidNC/tests/FixedCircularBufferTest.cpp | 42 ------------- fixture_tests/fixtures/flow_control_repeat.nc | 45 -------------- fixture_tests/run_fixture | 9 +-- fixture_tests/tool/controller.py | 26 ++------ fixture_tests/tool/op_entries.py | 36 +++-------- 12 files changed, 32 insertions(+), 261 deletions(-) delete mode 100644 FluidNC/src/FixedCircularBuffer.h delete mode 100644 FluidNC/tests/FixedCircularBufferTest.cpp delete mode 100644 fixture_tests/fixtures/flow_control_repeat.nc diff --git a/FluidNC/src/FixedCircularBuffer.h b/FluidNC/src/FixedCircularBuffer.h deleted file mode 100644 index f2936b15a..000000000 --- a/FluidNC/src/FixedCircularBuffer.h +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) 2024 - Dylan Knutson -// Use of this source code is governed by a GPLv3 license that can be found in the LICENSE file. - -#pragma once - -#include -#include - -/** - * A fixed-size circular buffer that stores elements of type T. - * Keeps track of how many elements have been pushed onto it, and allows - * for indexing as if it was an infinite sized array. If indexing into - * the buffer would result in an out-of-bounds access, returns std::nullopt. - * - * This is useful for implementing "scrollback" of a buffer of e.g. user - * provided commands, without using an unbounded amount of memory. - */ -template -class FixedCircularBuffer { -public: - std::vector storage; - std::size_t head_idx, tail_idx; - -public: - FixedCircularBuffer() : FixedCircularBuffer(0) {} - FixedCircularBuffer(size_t size) : storage(size), head_idx(0), tail_idx(0) {} - - /** - * Push an element onto the end of the buffer. - */ - void push(T&& elem) { - storage[tail_idx % storage.size()] = std::move(elem); - tail_idx += 1; - if (tail_idx - head_idx > storage.size()) { - head_idx += 1; - } - } - - /** - * Get the element at the given index, or std::nullopt if the index is out of bounds. - */ - std::optional at(std::size_t idx) const { - if (idx >= tail_idx) { - return std::nullopt; - } - if (idx < head_idx) { - return std::nullopt; - } - return storage[idx % storage.size()]; - } - - /** - * Is the buffer empty? - */ - bool is_empty() const { return head_idx == tail_idx; } - - /** - * Get the index of the last element pushed onto the buffer. - */ - std::size_t position() const { return tail_idx; } -}; diff --git a/FluidNC/src/Flowcontrol.cpp b/FluidNC/src/Flowcontrol.cpp index 0fe38e3e6..4be12aecd 100644 --- a/FluidNC/src/Flowcontrol.cpp +++ b/FluidNC/src/Flowcontrol.cpp @@ -233,10 +233,8 @@ Error flowcontrol(uint32_t o_label, char* line, size_t& pos, bool& skip) { case Op_Repeat: if (Job::active()) { if (!skipping && (status = expression(line, pos, value)) == Error::Ok) { - // TODO - return an error if value < 0 - // For now, just guard against negative values - stack_push(o_label, operation, !(value > 0.0)); - if (value > 0.0) { + stack_push(o_label, operation, !value); + if (value) { context.top().file = Job::source(); context.top().file_pos = context.top().file->position(); context.top().repeats = (uint32_t)value; @@ -251,7 +249,7 @@ Error flowcontrol(uint32_t o_label, char* line, size_t& pos, bool& skip) { if (Job::active()) { if (last_op == Op_Repeat) { if (o_label == context.top().o_label) { - if (context.top().repeats && --context.top().repeats > 0.0) { + if (context.top().repeats && --context.top().repeats) { context.top().file->set_position(context.top().file_pos); } else { stack_pull(); diff --git a/FluidNC/src/Job.cpp b/FluidNC/src/Job.cpp index a97801d08..5de670121 100644 --- a/FluidNC/src/Job.cpp +++ b/FluidNC/src/Job.cpp @@ -4,29 +4,17 @@ #include "Job.h" #include #include -#include "Protocol.h" std::stack job; Channel* Job::leader = nullptr; bool Job::active() { - return !job.empty() || activeChannel; + return !job.empty(); } -JobSource activeChannelJobSource(nullptr); - JobSource* Job::source() { - if (job.empty()) { - if (activeChannel) { - activeChannelJobSource.set_channel(activeChannel); - return &activeChannelJobSource; - } else { - return nullptr; - } - } else { - return job.top(); - } + return job.empty() ? nullptr : job.top(); } // save() and restore() are use to close/reopen an SD file atop the job stack @@ -50,11 +38,9 @@ void Job::nest(Channel* in_channel, Channel* out_channel) { job.push(source); } void Job::pop() { - if (!job.empty()) { - auto source = job.top(); - job.pop(); - delete source; - } + auto source = job.top(); + job.pop(); + delete source; if (!active()) { leader = nullptr; } @@ -74,14 +60,14 @@ void Job::abort() { } bool Job::get_param(const std::string& name, float& value) { - return source()->get_param(name, value); + return job.top()->get_param(name, value); } bool Job::set_param(const std::string& name, float value) { - return source()->set_param(name, value); + return job.top()->set_param(name, value); } bool Job::param_exists(const std::string& name) { - return source()->param_exists(name); + return job.top()->param_exists(name); } Channel* Job::channel() { - return source()->channel(); + return job.top()->channel(); } diff --git a/FluidNC/src/Job.h b/FluidNC/src/Job.h index 378b659b5..c44684b77 100644 --- a/FluidNC/src/Job.h +++ b/FluidNC/src/Job.h @@ -31,7 +31,6 @@ class JobSource { void set_position(size_t pos) { _channel->set_position(pos); } Channel* channel() { return _channel; } - void set_channel(Channel* channel) { _channel = channel; } ~JobSource() { delete _channel; } }; diff --git a/FluidNC/src/Protocol.h b/FluidNC/src/Protocol.h index 0ede1bc23..37d08c4d7 100644 --- a/FluidNC/src/Protocol.h +++ b/FluidNC/src/Protocol.h @@ -50,8 +50,6 @@ extern volatile bool rtCycleStop; extern volatile bool runLimitLoop; -extern Channel* activeChannel; - // Alarm codes. enum class ExecAlarm : uint8_t { None = 0, diff --git a/FluidNC/src/UartChannel.cpp b/FluidNC/src/UartChannel.cpp index 1c907d327..9859f78ae 100644 --- a/FluidNC/src/UartChannel.cpp +++ b/FluidNC/src/UartChannel.cpp @@ -6,10 +6,8 @@ #include "Serial.h" // allChannels UartChannel::UartChannel(int num, bool addCR) : Channel("uart_channel", num, addCR) { - _lineedit = new Lineedit(this, _line, Channel::maxLine - 1); - _active = false; - _history_buffer = FixedCircularBuffer(512); - _history_buffer_pos = 0; + _lineedit = new Lineedit(this, _line, Channel::maxLine - 1); + _active = false; } void UartChannel::init() { @@ -65,13 +63,10 @@ size_t UartChannel::write(const uint8_t* buffer, size_t length) { } int UartChannel::available() { - return (_history_buffer_pos < _history_buffer.position()) || _uart->available(); + return _uart->available(); } int UartChannel::peek() { - if (_history_buffer_pos < _history_buffer.position()) { - return _history_buffer.at(_history_buffer_pos).value(); - } return _uart->peek(); } @@ -95,22 +90,12 @@ bool UartChannel::lineComplete(char* line, char c) { } int UartChannel::read() { - if (_history_buffer_pos < _history_buffer.position()) { - int c = _history_buffer.at(_history_buffer_pos).value(); - _history_buffer_pos += 1; - return c; - } - int c = _uart->read(); if (c == 0x11) { // 0x11 is XON. If we receive that, it is a request to use software flow control _uart->setSwFlowControl(true, -1, -1); return -1; } - if (c != -1) { - _history_buffer.push((char)c); - _history_buffer_pos += 1; - } return c; } diff --git a/FluidNC/src/UartChannel.h b/FluidNC/src/UartChannel.h index 1c2ffbfff..eb8cbb955 100644 --- a/FluidNC/src/UartChannel.h +++ b/FluidNC/src/UartChannel.h @@ -6,14 +6,11 @@ #include "Uart.h" #include "Channel.h" #include "lineedit.h" -#include "FixedCircularBuffer.h" class UartChannel : public Channel, public Configuration::Configurable { private: - Lineedit* _lineedit; - Uart* _uart; - FixedCircularBuffer _history_buffer; - std::size_t _history_buffer_pos; + Lineedit* _lineedit; + Uart* _uart; int _uart_num = 0; int _report_interval_ms = 0; @@ -52,9 +49,6 @@ class UartChannel : public Channel, public Configuration::Configurable { handler.item("uart_num", _uart_num); handler.item("message_level", _message_level, messageLevels2); } - - size_t position() override { return _history_buffer_pos; } - void set_position(size_t pos) override { _history_buffer_pos = pos; } }; extern UartChannel Uart0; diff --git a/FluidNC/tests/FixedCircularBufferTest.cpp b/FluidNC/tests/FixedCircularBufferTest.cpp deleted file mode 100644 index 729f1ce13..000000000 --- a/FluidNC/tests/FixedCircularBufferTest.cpp +++ /dev/null @@ -1,42 +0,0 @@ -// Copyright (c) 2024 - Dylan Knutson -// Use of this source code is governed by a GPLv3 license that can be found in the LICENSE file. - -#include "gtest/gtest.h" -#include "src/FixedCircularBuffer.h" - -TEST(FixedCircularBuffer, Empty) { - FixedCircularBuffer buffer(0); - - ASSERT_TRUE(buffer.is_empty()); - ASSERT_EQ(buffer.position(), 0); - ASSERT_EQ(buffer.at(0), std::nullopt); - ASSERT_EQ(buffer.at(1), std::nullopt); - ASSERT_EQ(buffer.at(2), std::nullopt); -} - -TEST(FixedCircularBuffer, OneElement) { - FixedCircularBuffer buffer(1); - - buffer.push(42); - - ASSERT_FALSE(buffer.is_empty()); - ASSERT_EQ(buffer.position(), 1); - ASSERT_EQ(buffer.at(0), 42); - ASSERT_EQ(buffer.at(1), std::nullopt); - ASSERT_EQ(buffer.at(2), std::nullopt); -} - -TEST(FixedCircularBuffer, FrontElementsPopped) { - FixedCircularBuffer buffer(2); - - buffer.push(1); - buffer.push(2); - buffer.push(3); - - ASSERT_FALSE(buffer.is_empty()); - ASSERT_EQ(buffer.position(), 3); - ASSERT_EQ(buffer.at(0), std::nullopt); - ASSERT_EQ(buffer.at(1), 2); - ASSERT_EQ(buffer.at(2), 3); - ASSERT_EQ(buffer.at(3), std::nullopt); -} diff --git a/fixture_tests/fixtures/flow_control_repeat.nc b/fixture_tests/fixtures/flow_control_repeat.nc deleted file mode 100644 index efea4c1a0..000000000 --- a/fixture_tests/fixtures/flow_control_repeat.nc +++ /dev/null @@ -1,45 +0,0 @@ -ignore ok - -# test repeat zero times (should not print anything) --> o100 repeat [0] --> (print, fail lit 0) --> o100 endrepeat --> (print, pass lit 0) -<- [MSG:INFO: PRINT, pass lit 0] - -# test when using a variable --> # = 0 --> o100 repeat [#] --> (print, fail var 0) --> o100 endrepeat --> (print, pass var 0) -<- [MSG:INFO: PRINT, pass var 0] - -# test negative repeat (should not print anything) -# todo - negative repeat should probably set an error --> o100 repeat [-1] --> (print, fail lit -1) --> o100 endrepeat --> (print, pass lit -1) -<- [MSG:INFO: PRINT, pass lit -1] - -# test repeat a fixed number of times --> # = 0 --> o100 repeat [3] --> # = [# + 1] --> (print, count=%d#) -<- [MSG:INFO: PRINT, count=1] --> o100 endrepeat -<- [MSG:INFO: PRINT, count=2] -<- [MSG:INFO: PRINT, count=3] - -# test repeating a variable number of times --> # = 0 --> # = 3 --> o100 repeat [#] --> # = [# + 1] --> (print, count=%d#) -<- [MSG:INFO: PRINT, count=1] --> o100 endrepeat -<- [MSG:INFO: PRINT, count=2] -<- [MSG:INFO: PRINT, count=3] diff --git a/fixture_tests/run_fixture b/fixture_tests/run_fixture index a0f6c0e65..0398d748a 100755 --- a/fixture_tests/run_fixture +++ b/fixture_tests/run_fixture @@ -28,12 +28,6 @@ else: def run_fixture(fixture_path, controller): op_entries_parsed = op_entries.parse_file(fixture_path) - print( - colored(f"--- Run fixture ", "blue") - + colored(fixture_path, "blue", attrs=["bold"]) - + colored(" ---", "blue") - ) - try: for op_entry in op_entries_parsed: if not op_entry.execute(controller): @@ -45,8 +39,7 @@ def run_fixture(fixture_path, controller): exit(1) except KeyboardInterrupt: - print("Interrupted by user") - exit(1) + print("Interrupt") except TimeoutError as e: print("Timeout waiting for response, line: " + e.args[0]) diff --git a/fixture_tests/tool/controller.py b/fixture_tests/tool/controller.py index 10c8ca3a3..d443617a3 100644 --- a/fixture_tests/tool/controller.py +++ b/fixture_tests/tool/controller.py @@ -1,8 +1,5 @@ import serial from termcolor import colored -from tool.utils import color - -DEBUG_SERIAL = False class Controller: @@ -10,10 +7,8 @@ def __init__(self, device, baudrate, timeout): self._debug = False self._serial = serial.Serial(device, baudrate, timeout=timeout) self._current_line = None - self._ignored_lines = set() def send_soft_reset(self): - self._ignored_lines = set() self._serial.write(b"\x18") self._serial.flush() self.clear_line() @@ -22,21 +17,10 @@ def send_soft_reset(self): self.clear_line() self.clear_line() - def ignore_line(self, line): - self._ignored_lines.add(line) - def current_line(self): - while self._current_line is None: - line = self._serial.readline().decode("utf-8").strip() - if DEBUG_SERIAL: - print(colored("[c] <- " + line, "light_blue")) - - if line in self._ignored_lines: - print(color.dark_grey("<- " + line + " (ignored)", dark=True)) - continue - - self._current_line = line - break + if self._current_line is None: + self._current_line = self._serial.readline().decode("utf-8").strip() + # print(colored("[c] <- " + self._current_line, "light_blue")) return self._current_line def clear_line(self): @@ -47,9 +31,7 @@ def next_line(self): return self.current_line() def send_line(self, line): - if DEBUG_SERIAL: - print(colored("[c] -> " + line, "light_blue")) - + # print(colored("[c] -> " + line, "light_blue")) self._serial.write(line.encode("utf-8") + b"\n") def getc(self, size): diff --git a/fixture_tests/tool/op_entries.py b/fixture_tests/tool/op_entries.py index 057908e31..c453612dd 100644 --- a/fixture_tests/tool/op_entries.py +++ b/fixture_tests/tool/op_entries.py @@ -9,10 +9,6 @@ def parse_file(fixture_path): with open(fixture_path, "r") as f: op_entries = [] for lineno, line in enumerate(f.read().splitlines()): - if line == "": - # skip empty lines - continue - if line.startswith("#"): # skip comment lines continue @@ -63,13 +59,6 @@ def _op_str(self): return color.dark_grey(self.op) + " " -class IgnoreLineOpEntry(OpEntry): - def execute(self, controller): - print(self._op_str() + color.dark_grey(self.data, dark=True, bold=True)) - controller.ignore_line(self.data) - return True - - class SendLineOpEntry(OpEntry): def execute(self, controller): print(self._op_str() + color.sent_line(self.data)) @@ -120,29 +109,26 @@ def execute(self, controller): class UntilStringMatchOpEntry(OpEntry): def __init__(self, op, data, lineno, fixture_path): - self._glob_match = data.startswith("* ") + self.glob_match = data.startswith("* ") super().__init__(op, data.removeprefix("* "), lineno, fixture_path) def execute(self, controller): while True: - line = controller.current_line() - matches = self._line_matches(line) - - opstr = self._op_str() - if self._glob_match: - opstr += color.dark_grey("* ", bold=True) - - print(opstr + color.green(line, dark=True, bold=matches)) + matches = self._line_matches(controller) + print( + self._op_str() + + color.green(controller.current_line(), dark=True, bold=matches) + ) controller.clear_line() if matches: break return True - def _line_matches(self, line): - if self._glob_match: - return fnmatch.fnmatch(line, self.data) + def _line_matches(self, controller): + if self.glob_match: + return fnmatch.fnmatch(controller.current_line(), self.data) else: - return self.data == line + return self.data == controller.current_line() class SendFileOpEntry(OpEntry): @@ -231,8 +217,6 @@ def execute(self, controller): OPS_MAP = { - # ignores messages consisting of only the following line (e.g. 'ok') - "ignore": IgnoreLineOpEntry, # send command to controller "->": SendLineOpEntry, # send file to controller