From d7adb63b789ee9b54e0ef80633c0d50531c03fc0 Mon Sep 17 00:00:00 2001 From: Urs Schaufelberger Date: Wed, 9 Sep 2026 23:36:01 +0200 Subject: [PATCH 1/7] Add a diagnostic trace build for the FluidNC link FNC_RX_TRACE already existed but printed nothing on the M5Dial: dbg_print and dbg_write compile to no-ops unless DEBUG_TO_USB is also defined, and the m5dial env does not define it. The new m5dial_trace env sets both. Also stop the trace dropping lines. dbg_print discarded output whenever the USB TX buffer was full, which is exactly what happens during a burst -- so the log went quiet precisely where the interesting thing occurred and read as "the transfer stopped here" when it had not. Under FNC_RX_TRACE it now waits for room, bounded to 50 ms so a detached USB host cannot wedge the firmware. Co-Authored-By: Claude Opus 5 (cherry picked from commit 087dfa06716bf49f1acf3e975c356b3841818c68) --- platformio.ini | 11 +++++++++++ src/SystemArduino.cpp | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/platformio.ini b/platformio.ini index d355baf..77d748c 100644 --- a/platformio.ini +++ b/platformio.ini @@ -55,6 +55,17 @@ custom_filesystem_start=0x670000 extra_scripts = ./build_merged.py build_src_filter = ${common.build_src_filter} + + +# Diagnostic build: m5dial plus USB debug output and FluidNC wire tracing. +# NOTE: FNC_RX_TRACE alone prints nothing on m5dial -- dbg_print/dbg_write are +# compiled out unless DEBUG_TO_USB is also defined (see SystemArduino.cpp). +# pio run -e m5dial_trace -t upload && pio device monitor -e m5dial_trace +[env:m5dial_trace] +extends = env:m5dial +build_flags = + ${env:m5dial.build_flags} + -DDEBUG_TO_USB + -DFNC_RX_TRACE + [env:cyd_base] ; Pendant based on a 2432S028 "Cheap Yellow Display" and a hand wheel pulse encoder ; http://wiki.fluidnc.com/en/hardware/official/CYD_Dial_Pendant diff --git a/src/SystemArduino.cpp b/src/SystemArduino.cpp index 8b33775..50fb178 100644 --- a/src/SystemArduino.cpp +++ b/src/SystemArduino.cpp @@ -196,19 +196,51 @@ void delay_ms(uint32_t ms) { delay(ms); } +#ifdef DEBUG_TO_USB +# ifdef FNC_RX_TRACE +// Diagnostic builds must not lose trace lines. The non-blocking variants below +// drop output whenever the USB TX buffer is full, which is precisely what +// happens during a burst -- so the log goes quiet exactly where the interesting +// thing occurred and reads as "it stopped here" when it didn't. Wait for room +// instead, bounded so a detached USB host can never wedge the firmware. +static bool dbg_wait_writable(size_t needed) { + uint32_t deadline = millis() + 50; + while (debugPort.availableForWrite() <= (int)needed) { + if ((int32_t)(millis() - deadline) >= 0) { + return false; + } + delay(1); + } + return true; +} +# endif +#endif + void dbg_write(uint8_t c) { #ifdef DEBUG_TO_USB +# ifdef FNC_RX_TRACE + if (dbg_wait_writable(1)) { + debugPort.write(c); + } +# else if (debugPort.availableForWrite() > 1) { debugPort.write(c); } +# endif #endif } void dbg_print(const char* s) { #ifdef DEBUG_TO_USB +# ifdef FNC_RX_TRACE + if (dbg_wait_writable(strlen(s))) { + debugPort.print(s); + } +# else if (debugPort.availableForWrite() > strlen(s)) { debugPort.print(s); } +# endif #endif } From c1f2e1f35ddf49e43894779e965edfd83651c3f3 Mon Sep 17 00:00:00 2001 From: Urs Schaufelberger Date: Wed, 9 Sep 2026 23:36:01 +0200 Subject: [PATCH 2/7] Stop the WiFi stack starving the UART reader fnc_poll() reads a single byte and then calls poll_extra(), which drove wifi_poll() unconditionally whenever USE_WIFI was compiled in. At FNC_BAUD 1000000 a byte arrives every 10 us, so a UART-connected pendant entered the WiFi stack roughly 100k times a second. The reader could not keep up, the RX ring overflowed, and bytes vanished from the middle of a streamed document: runs of text disappeared and the tail of a status report ended up spliced into a JSON chunk, derailing the parser. Nothing in wifi_poll() is on the UART data path -- it services the OTA server and its AP-mode DNS -- so in UART mode it now runs on a 20 ms timer. Telnet and ESP-NOW keep the per-byte call, where the socket refill genuinely is on the data path. Two buffers on the same path were also undersized for 1 Mbaud: - the UART RX ring was 256 bytes, only twice the hardware FIFO and about 2.5 ms of headroom, less than a single frame render - the drain loop in loop() stopped after 64 bytes per tick, which could not keep pace with 100-byte file-transfer chunks, so the remainder aged in the ring until it overflowed Now 2048 and 1024. The drain loop already exits the moment RX is empty, so the larger bound costs nothing at idle. Co-Authored-By: Claude Opus 5 (cherry picked from commit 37488d7fdc8842417d23f6df94148c3bc53ecf27) --- src/SystemArduino.cpp | 23 +++++++++++++++++++++-- src/ardmain.cpp | 6 +++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/SystemArduino.cpp b/src/SystemArduino.cpp index 50fb178..b0853c4 100644 --- a/src/SystemArduino.cpp +++ b/src/SystemArduino.cpp @@ -100,8 +100,23 @@ extern "C" void poll_extra() { #ifdef USE_WIFI if (wifi_use_espnow_mode()) { espnow_poll(); - } else { + } else if (!wifi_use_uart_mode()) { wifi_poll(); + } else { + // UART transport. The network stack still needs servicing (the OTA + // server and its AP-mode DNS live in wifi_poll()), but fnc_poll() calls + // poll_extra() after EVERY received byte -- at 1 Mbaud that is ~100k + // calls per second, each one entering the WiFi stack. The reader can no + // longer keep up, the 256-byte UART RX ring overflows, and bytes + // disappear from the middle of a document: runs of text vanish and the + // tail of one message gets spliced into another. Poll it on a timer + // instead -- nothing here is on the UART data path. + static uint32_t last_wifi_poll_ms = 0; + uint32_t now = millis(); + if ((uint32_t)(now - last_wifi_poll_ms) >= 20) { + last_wifi_poll_ms = now; + wifi_poll(); + } } #endif #ifdef DEBUG_TO_USB @@ -161,7 +176,11 @@ void init_fnc_uart(int uart_num, int tx_pin, int rx_pin) { while (1) {} return; }; - uart_driver_install(fnc_uart_port, 256, 0, 0, NULL, ESP_INTR_FLAG_IRAM); + // 256 bytes is only twice the hardware FIFO and gives ~2.5 ms of headroom at + // 1 Mbaud -- less than a single frame render, so a burst overruns the ring + // and bytes vanish out of the middle of a streamed JSON document. The extra + // few KB of RAM is cheap next to losing the document. + uart_driver_install(fnc_uart_port, 2048, 0, 0, NULL, ESP_INTR_FLAG_IRAM); uart_set_sw_flow_ctrl(fnc_uart_port, true, 64, 120); uint32_t baud; uart_get_baudrate(fnc_uart_port, &baud); diff --git a/src/ardmain.cpp b/src/ardmain.cpp index 98ec55b..4b6f60d 100644 --- a/src/ardmain.cpp +++ b/src/ardmain.cpp @@ -164,7 +164,11 @@ void loop() { // // Drain all pending data, but stop when RX is empty to avoid // unnecessary Wi-Fi polling and reduce idle-loop jitter that can make small jog movements choppy. - for (int i = 0; i < 64; i++) { + // The cap only bounds a pathological burst -- the loop exits the moment RX + // is empty, so a larger bound costs nothing at idle. 64 bytes per tick could + // not keep up with a 100-byte-per-chunk file stream, leaving the remainder + // to age in the ring until it overflowed. + for (int i = 0; i < 1024; i++) { fnc_poll(); if (!fnc_rx_waiting()) { break; From 6c6c1602e3e759c463685180ff5027e4d1e36bc4 Mon Sep 17 00:00:00 2001 From: Urs Schaufelberger Date: Wed, 9 Sep 2026 23:36:21 +0200 Subject: [PATCH 3/7] Send FluidNC echo-off according to the active transport The Ctrl-L that turns FluidNC's command echo off was guarded by #ifndef USE_WIFI and commented "UART only". But USE_WIFI means a network transport is available in this build, not that one is in use, so a pendant built with it and wired to FluidNC's UART stopped sending echo-off entirely. With echo left on, FluidNC echoes every command back, and an echoed "$..." line arriving mid-document trips the json_reset_depth() in handle_other(), tearing down in-flight macro or file JSON. Gate it on wifi_use_uart_mode() instead. Co-Authored-By: Claude Opus 5 (cherry picked from commit 1ba93872660ba6255981bcdf5ae00ef4c3bbe1c2) --- src/FluidNCModel.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/FluidNCModel.cpp b/src/FluidNCModel.cpp index 07b27bb..c8202aa 100644 --- a/src/FluidNCModel.cpp +++ b/src/FluidNCModel.cpp @@ -353,8 +353,18 @@ bool awaiting_alarm = false; static void connect_init() { bootlog_printf("connected: state=%s", my_state_string); resetFlowControl(); // clear any stale XOFF on the link -#ifndef USE_WIFI - fnc_realtime((realtime_cmd_t)0x0c); // Ctrl-L - echo off (UART only) + // Ctrl-L - echo off. This must follow the *runtime* transport, not the + // build: USE_WIFI only means a WiFi/ESP-NOW transport is available, and a + // pendant built with it can still be wired to FluidNC's UART. Skipping the + // echo-off there leaves FluidNC echoing every command back, and an echoed + // "$..." line arriving mid-document trips the json_reset_depth() in + // handle_other(), tearing down in-flight macro/file JSON. +#ifdef USE_WIFI + if (wifi_use_uart_mode()) { + fnc_realtime((realtime_cmd_t)0x0c); + } +#else + fnc_realtime((realtime_cmd_t)0x0c); #endif send_line("$G"); // Refresh GCode modes send_line("$RI=200"); // Enable auto-reporting every 200 ms From 93017d628c1c81d6f2d73d6d19b9838dd8c1a94a Mon Sep 17 00:00:00 2001 From: Urs Schaufelberger Date: Wed, 9 Sep 2026 23:36:21 +0200 Subject: [PATCH 4/7] Fix commands mangled by re-entrant send_linef vsend_linef() formatted into a static buffer and passed it to send_line(). send_line() -> fnc_send_line() spins in a "while (_ackwait) { fnc_poll(); }" wait loop BEFORE it reads the string, and fnc_poll() dispatches received reports whose handlers call send_linef() again. The inner call overwrote the buffer while the outer one was still about to transmit from it, so the outer command went out mangled or missing its terminator. Observed on the wire as "$Files/ListGCode=/sd" arriving as "/sd$G" -- FluidNC answering {"files":[],"path":"/sd$G","error":"Bad path"} -- and as a steady stream of error:3 (Bad $ statement) from garbled commands. Give it automatic storage so a re-entrant call cannot touch the outer call's string. This bug is long-standing; it became reachable when the volume of handler-driven traffic increased. Co-Authored-By: Claude Opus 5 (cherry picked from commit 76262f239f48dfe825adb401c0b42aa8b42cf0a0) --- src/FluidNCModel.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/FluidNCModel.cpp b/src/FluidNCModel.cpp index c8202aa..9b01dc6 100644 --- a/src/FluidNCModel.cpp +++ b/src/FluidNCModel.cpp @@ -314,8 +314,16 @@ void send_jog_cancel() { } static void vsend_linef(const char* fmt, va_list va) { - static char buf[128]; - vsnprintf(buf, 128, fmt, va); + // This buffer MUST NOT be static. send_line() -> fnc_send_line() spins in a + // "while (_ackwait) { fnc_poll(); }" wait loop BEFORE it reads the string, + // and fnc_poll() dispatches received reports -- handlers of which call + // send_linef() again. A shared buffer is therefore overwritten by the inner + // call while the outer one is still about to transmit from it, so the outer + // command goes out mangled or missing its terminator. That is how a + // "$Files/ListGCode=/sd" ends up on the wire as "/sd$G" and why FluidNC + // answers a stream of error:3 (Bad $ statement). + char buf[128]; + vsnprintf(buf, sizeof(buf), fmt, va); send_line(buf); } void send_linef(const char* fmt, ...) { From 4e96f4bbe4a1f85e44872cd51d347fb67154661c Mon Sep 17 00:00:00 2001 From: Urs Schaufelberger Date: Wed, 9 Sep 2026 23:36:21 +0200 Subject: [PATCH 5/7] Bound config-item requests and stop stray errors killing transfers Two problems in error handling, both reachable from the same code path. Config requests retried forever. service_config_requests() re-sent configRequests.front() every 500 ms until FluidNC answered with a matching "$name=value". A setting the machine does not have -- an axis that is not configured, a key this firmware build lacks -- is answered with a bare "error:3", which never matches parse_dollar(), so the item never left the queue. detect_homing_info() queues twelve of these on connect, and since only front() is ever sent, one unanswerable item blocked every request behind it while flooding the link. Retries are now capped, and an error arriving while a query is outstanding drops that item immediately rather than burning the whole budget. show_error() also made its own guard dead. It reset the JSON depth and then called file_request_failed_advance(), whose first act is to check json_in_progress() -- which could no longer be true. That guard exists because FluidNC interleaves messages: an error from an already-failed request can land after the NEXT request's document has begun, and advancing there both tears down a healthy document and skips past it. Ask the question while the answer is still true. Co-Authored-By: Claude Opus 5 (cherry picked from commit 768e934b27c38e422857820fea49dc537d403fbb) --- src/ConfigItem.cpp | 59 +++++++++++++++++++++++++++++++++++++++++--- src/ConfigItem.h | 1 + src/FluidNCModel.cpp | 13 ++++++++-- 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/ConfigItem.cpp b/src/ConfigItem.cpp index 21c84cb..20cead9 100644 --- a/src/ConfigItem.cpp +++ b/src/ConfigItem.cpp @@ -1,23 +1,42 @@ #include "ConfigItem.h" #include "Scene.h" #include "System.h" +#include "FileParser.h" // json_in_progress() std::vector configRequests; static constexpr uint32_t CONFIG_REQUEST_RETRY_MS = 500; -static uint32_t configRequestSentMs = 0; + +// Bound the retries. FluidNC rejects a setting it doesn't have (an axis that +// isn't configured, a key this firmware build lacks) with a bare "error:3", +// which never matches parse_dollar(), so the item never leaves the queue. An +// unbounded retry loop then re-sends it every 500 ms forever. That floods the +// link, blocks every request queued behind it (we only ever send front()), and +// splices error responses into an in-flight $File/SendJSON document -- which is +// what empties the macro list. Give up on the item instead and move on. +static constexpr int CONFIG_REQUEST_MAX_TRIES = 4; + +static uint32_t configRequestSentMs = 0; +static int configRequestTries = 0; static bool can_send_config_request() { // ensure config requests are sent only when a job is not running - as it won't be processed otherwise return state == Idle || state == Alarm; } -static void send_next_config_request() { +static void send_config_request() { if (configRequests.empty() || !can_send_config_request()) { return; } configRequests.front()->send_request(); configRequestSentMs = millis(); + ++configRequestTries; +} + +// Move on to a different request. The try budget is per item, so reset it. +static void send_next_config_request() { + configRequestTries = 0; + send_config_request(); } void ConfigItem::init() { @@ -40,13 +59,45 @@ void ConfigItem::init() { void clear_config_requests() { configRequests.clear(); configRequestSentMs = 0; + configRequestTries = 0; } void service_config_requests() { - if (!configRequests.empty() && - (uint32_t)(millis() - configRequestSentMs) >= CONFIG_REQUEST_RETRY_MS) { + if (configRequests.empty()) { + return; + } + if ((uint32_t)(millis() - configRequestSentMs) < CONFIG_REQUEST_RETRY_MS) { + return; + } + // Never transmit while a JSON document is streaming in. FluidNC answers on + // the same link, so the reply (or its error) lands in the middle of the + // document and derails the parser mid-object. + if (json_in_progress()) { + return; + } + if (configRequestTries >= CONFIG_REQUEST_MAX_TRIES) { + // FluidNC is never going to answer this one. Drop it -- it stays + // !known(), which callers already handle -- so the rest of the queue + // can drain instead of being stuck behind it. + configRequests.erase(configRequests.begin()); send_next_config_request(); + return; + } + send_config_request(); +} + +// Called from show_error() when FluidNC rejects a command and no JSON document +// is in flight. If a config query is outstanding, that error is almost certainly +// its answer: the setting doesn't exist on this machine (an axis that isn't +// configured, a key this firmware lacks). Retrying cannot change that, so drop +// it now rather than burning the whole retry budget at 500 ms a go -- twelve +// homing items would otherwise spend ~24 s flooding the link with error:3. +void config_request_failed() { + if (configRequests.empty() || configRequestTries == 0) { + return; // nothing outstanding, so the error belongs to someone else } + configRequests.erase(configRequests.begin()); + send_next_config_request(); } void parse_dollar(const char* line) { diff --git a/src/ConfigItem.h b/src/ConfigItem.h index 99b69ae..67b62bd 100644 --- a/src/ConfigItem.h +++ b/src/ConfigItem.h @@ -6,6 +6,7 @@ class ConfigItem; extern std::vector configRequests; void service_config_requests(); +void config_request_failed(); void clear_config_requests(); class ConfigItem { diff --git a/src/FluidNCModel.cpp b/src/FluidNCModel.cpp index 9b01dc6..39a3c71 100644 --- a/src/FluidNCModel.cpp +++ b/src/FluidNCModel.cpp @@ -445,14 +445,23 @@ extern "C" void show_error(int error) { if (s_jog_window > 0) { --s_jog_window; // a rejected jog ends 1 outstanding line } + // Order matters. file_request_failed_advance() deliberately ignores an + // error:N that arrives while a JSON document is still streaming, because + // FluidNC interleaves messages on the wire: an error from an already-failed + // request can land after the NEXT request's document has begun. Resetting + // the depth before calling it made that guard dead code, so a stale error + // both tore down a healthy macro document and advanced the chain past it -- + // which is exactly the "No Macros" screen with the macros sitting in the + // buffer. Ask the question while the answer is still true. if (json_in_progress()) { - // "error:N" without a JSON wrapper ends an in-flight document. - json_reset_depth(); + request_redisplay(); + return; } // Telnet returns bare "error:N" with no JSON wrapper when $File/SendJSON // is rejected (file not present, etc). Without this hook the macro chain // sits on "Reading Macros" forever because endDocument never fires. file_request_failed_advance(); + config_request_failed(); request_redisplay(); } From cd603651d257799148dec072b2e7fc8a259b8c32 Mon Sep 17 00:00:00 2001 From: Urs Schaufelberger Date: Wed, 9 Sep 2026 23:36:44 +0200 Subject: [PATCH 6/7] Fix macro list loading and harden file transfers The Macros screen showed "No Macros" on a UART-connected pendant even with macros present. Several distinct causes, all on the preferences.json path. The macros key was never matched. PreferencesListener is installed part-way through the document, on the "result" key of the $File/SendJSON wrapper, so its _level is relative to wherever it took over: preferences.json puts "settings"/"macros" at _level 1 there, not 2. The check was pinned to _level == 2, and the _level < 2 bail above it swallowed the key before it was ever tested. Match it at whatever depth it appears, and record that depth so entries are the objects exactly one deeper -- nested structure inside an entry must not be mistaken for an entry. Per-entry state was never reset. Unlike the other two listeners, PreferencesListener::startObject() did not clear _name/_target/_filename, so an entry missing "action" inherited the previous entry's filename -- which had ALREADY had its "/localfs/" prefix inserted -- and was prefixed again, showing as "//localfs/". An entry missing "type" inherited the previous type and was added when it should have been skipped. Only one key spelling was accepted. WebUI versions disagree: older exports use filename/target (what MacroListListener expects), newer ones action/type. Accept either, treat ESP as a localfs target alongside FS, normalise a leading slash before prefixing so neither convention yields "/localfs//foo.g", and skip entries with an empty action rather than adding a row that does nothing. Auto-reporting corrupted transfers. $RI keeps emitting status lines while a file streams on the same channel, so any byte lost to a stall welds the two together and a chunk ends up carrying the tail of a status report. Suspend it for the duration of $File/SendJSON, $Files/ListGCode and $File/ShowSome. The resume is keyed on "no document in flight and the last request is old enough" rather than on the terminal branches, so it comes back down every route home, including for the list and preview transfers which have no chain state of their own. The chain could stall forever. With show_error() no longer advancing on an error that lands mid-document, a genuinely torn transfer left the menu on "Reading Macros" indefinitely. service_macro_chain(), polled from dispatch_events(), puts a deadline on an in-flight request. Trace output is now buffered and flushed only when nothing is streaming. Printing inline was self-defeating: dbg_print blocks waiting for USB buffer space, and 50 ms of blocking at 1 Mbaud is ~5 KB of arriving UART data, more than the RX ring holds -- so the act of tracing overflowed the ring and destroyed the transfer being traced. Co-Authored-By: Claude Opus 5 (cherry picked from commit d6b2bd9396188f7cbabf4eaeec4568ed97ac8ada) --- src/FileParser.cpp | 217 +++++++++++++++++++++++++++++++++++++++++---- src/FileParser.h | 8 ++ src/MacroMenu.cpp | 7 ++ src/Scene.cpp | 2 + 4 files changed, 217 insertions(+), 17 deletions(-) diff --git a/src/FileParser.cpp b/src/FileParser.cpp index e5ef97d..185389a 100644 --- a/src/FileParser.cpp +++ b/src/FileParser.cpp @@ -15,6 +15,46 @@ extern Menu macroMenu; +#ifdef FNC_RX_TRACE +# include +// Trace output is buffered in RAM and flushed only when the link is idle. +// Printing inline is self-defeating: dbg_print blocks waiting for USB buffer +// space, and 50 ms of blocking at 1 Mbaud is ~5 KB of arriving UART data -- +// more than the RX ring holds. The act of tracing was overflowing the ring and +// destroying the very transfer being traced. +static char s_trace_buf[4096]; +static size_t s_trace_len = 0; +static int s_trace_dropped = 0; + +static void trace_defer(const char* fmt, ...) { + char line[192]; + va_list ap; + va_start(ap, fmt); + vsnprintf(line, sizeof(line), fmt, ap); + va_end(ap); + size_t len = strlen(line); + if (s_trace_len + len + 1 >= sizeof(s_trace_buf)) { + ++s_trace_dropped; + return; + } + memcpy(s_trace_buf + s_trace_len, line, len); + s_trace_len += len; +} + +static void trace_flush() { + if (s_trace_len == 0 && s_trace_dropped == 0) { + return; + } + s_trace_buf[s_trace_len] = '\0'; + dbg_print(s_trace_buf); + if (s_trace_dropped) { + dbg_printf("[trace] %d line(s) dropped, buffer full\n", s_trace_dropped); + } + s_trace_len = 0; + s_trace_dropped = 0; +} +#endif + fileinfo fileInfo; std::vector fileVector; @@ -243,6 +283,10 @@ class PreferencesListener : public JsonListener { int _level = 0; bool _in_macros_section = false; + // _level at which the "macros" key appeared. Entries are the objects one + // deeper than this; anything deeper still is nested structure inside an + // entry and must not be mistaken for one. + int _macros_level = -1; public: void whitespace(char c) override {} @@ -256,33 +300,67 @@ class PreferencesListener : public JsonListener { void endArray() override { if (_in_macros_section) { _in_macros_section = false; + _macros_level = -1; +#ifdef FNC_RX_TRACE + trace_defer("[prefs] <<< macros section ends, %d item(s) added\n", macroMenu.num_items()); +#endif current_scene->onFilesList(); } } - void startObject() override { ++_level; } + void startObject() override { + ++_level; + // Reset per-entry state. Without this the fields carry over from the + // previous macro, so an entry missing "action" inherits the last one's + // filename -- which has ALREADY had its "/localfs/" prefix inserted -- + // and gets prefixed a second time, surfacing as "//localfs/". An entry + // missing "type" likewise inherits the previous type and gets added + // when it should have been skipped. The other two listeners already do + // this; this one was the odd one out. + if (_in_macros_section && _level == _macros_level + 1) { + _name.clear(); + _target.clear(); + _filename.clear(); + } + } void key(const char* key) override { _key = key; #ifdef FNC_RX_TRACE - // Surface every key the preferences listener actually sees, with its - // depth-from-listener-perspective, so we can verify the structure - // matches what _level == 2 expects. - dbg_printf("[prefs] L%d key=%s\n", _level, key); + // Only the top level and the macros section. Printing every key was + // self-defeating: dbg output blocks waiting for USB buffer space, and + // several hundred lines through the settings/keymap blobs stalled the + // UART reader long enough to drop the very bytes being traced. + if (_level <= 1 || _in_macros_section) { + trace_defer("[prefs] L%d key=%s\n", _level, key); + } +#endif + // Match the macros key at whatever depth it appears. This listener is + // installed part-way through the document (on the "result" key of the + // $File/SendJSON wrapper), so its _level is relative to wherever it + // took over -- preferences.json puts "settings"/"macros" at _level 1 + // here, not 2. Pinning the check to _level == 2 meant the earlier + // _level < 2 bail swallowed the macros key before it was ever tested. + if (strcmp(key, "macros") == 0) { + _in_macros_section = true; + _macros_level = _level; +#ifdef FNC_RX_TRACE + trace_defer("[prefs] >>> macros section begins at L%d\n", _level); #endif - if (_level < 2) { - // The only thing we care about is the macros section at level 2 return; } - if (_level == 2 && (strcmp(key, "macros") == 0)) { - _in_macros_section = true; + if (_level < 2) { return; } if (_in_macros_section) { - if (strcmp(key, "action") == 0) { + // WebUI versions disagree on the spelling: older exports use + // filename/target (what MacroListListener expects), newer ones use + // action/type. Accept either rather than silently producing an + // empty entry when the file uses the other one. + if (strcmp(key, "action") == 0 || strcmp(key, "filename") == 0) { _valuep = &_filename; return; } - if (strcmp(key, "type") == 0) { + if (strcmp(key, "type") == 0 || strcmp(key, "target") == 0) { _valuep = &_target; return; } @@ -304,14 +382,41 @@ class PreferencesListener : public JsonListener { void endObject() override { --_level; - if (_in_macros_section) { - if (_target == "FS") { + if (_in_macros_section && _level == _macros_level) { +#ifdef FNC_RX_TRACE + trace_defer("[prefs] macro name=\"%s\" type=\"%s\" action=\"%s\"\n", + _name.c_str(), _target.c_str(), _filename.c_str()); +#endif + // An entry with nothing to run is not a macro. Adding it yields a + // menu row that does nothing, or sends a bare command line. + if (_filename.empty()) { +#ifdef FNC_RX_TRACE + trace_defer("[prefs] ^^ DROPPED: empty action\n"); +#endif + return; + } + // Normalise before prefixing. One schema stores "foo.g", the + // other "/foo.g"; blindly inserting "/localfs/" turns the latter + // into "/localfs//foo.g". + if (_target == "FS" || _target == "ESP" || _target == "SD") { + if (!_filename.empty() && _filename[0] == '/') { + _filename.erase(0, 1); + } + } + if (_target == "FS" || _target == "ESP") { _filename.insert(0, "/localfs/"); } else if (_target == "SD") { _filename.insert(0, "/sd/"); } else if (_target == "CMD") { _filename.insert(0, "cmd:"); } else { +#ifdef FNC_RX_TRACE + // Every entry whose type isn't one of the three recognised + // spellings is silently discarded -- the array parses fine and + // the menu still ends up empty, which looks identical to a + // failed transfer. + trace_defer("[prefs] ^^ DROPPED: unrecognised type \"%s\"\n", _target.c_str()); +#endif return; } macroMenu.addItem(new MacroItem { _name.c_str(), _filename }); @@ -329,9 +434,40 @@ JsonStreamingParser* macro_parser; bool reading_macros = false; +// When the in-flight $File/SendJSON was issued, so a transfer that dies on the +// wire can't strand the macro chain. Zero means nothing is outstanding. +static uint32_t s_file_request_sent_ms = 0; +static constexpr uint32_t FILE_REQUEST_TIMEOUT_MS = 5000; +// How long after the last request, with no document in flight, before auto +// reporting is turned back on. +static constexpr uint32_t AUTO_REPORT_RESUME_MS = 1000; + +// FluidNC's auto-report ($RI) keeps emitting status lines while +// a file is streaming, on the same channel. Any byte lost to a stall then welds +// the two together -- a chunk ends up carrying the tail of a status report +// ("...|FS:0,0>") and the JSON parser derails. Nothing on the macro path needs +// live DRO, so silence auto-reporting for the duration of the transfer. +static bool s_auto_report_suspended = false; + +static void suspend_auto_report() { + if (!s_auto_report_suspended) { + s_auto_report_suspended = true; + send_line("$RI=0"); + } +} + +void resume_auto_report() { + if (s_auto_report_suspended) { + s_auto_report_suspended = false; + send_line("$RI=200"); + } +} + void request_json_file(const char* name) { + suspend_auto_report(); send_linef("$File/SendJSON=/%s", name); - parser_needs_reset = true; + parser_needs_reset = true; + s_file_request_sent_ms = milliseconds(); } // Track which file request is in flight so we can advance the macro @@ -408,7 +544,7 @@ void try_next_macro_file(JsonListener* listener) { extern "C" void file_request_failed_advance() { if (json_in_progress()) { #ifdef FNC_RX_TRACE - dbg_printf("[macro-chain] stale error suppressed (JSON in flight)\n"); + trace_defer("[macro-chain] stale error suppressed (JSON in flight)\n"); #endif return; } @@ -418,7 +554,7 @@ extern "C" void file_request_failed_advance() { if (s_chain_advance_at_ms != 0 && (milliseconds() - s_chain_advance_at_ms) < CHAIN_ADVANCE_COOLDOWN_MS) { #ifdef FNC_RX_TRACE - dbg_printf("[macro-chain] stale error suppressed (advance cooldown)\n"); + trace_defer("[macro-chain] stale error suppressed (advance cooldown)\n"); #endif return; } @@ -438,6 +574,42 @@ void request_macros() { try_next_macro_file(nullptr); } +// Called from dispatch_events(). show_error() deliberately ignores an error:N +// that lands while a document is streaming, which is right for a stale error +// but means a genuinely torn transfer never advances the chain -- the menu then +// sits on "Reading Macros" forever. Give the request a deadline instead. +void service_macro_chain() { + // Restore auto-reporting once nothing is streaming any more. Keying this on + // "no document in flight AND the last request is old enough" rather than on + // the terminal branches means the DRO comes back down every route home -- + // completion, error, give-up -- and for the file list and preview transfers + // too, which have no chain state of their own. + if (s_auto_report_suspended && !s_pending_file_listener && !json_in_progress() && + (uint32_t)(milliseconds() - s_file_request_sent_ms) >= AUTO_REPORT_RESUME_MS) { + resume_auto_report(); + } +#ifdef FNC_RX_TRACE + // Flush only when nothing is streaming, so the (blocking) USB writes can + // never stall the UART reader mid-document. + if (!json_in_progress()) { + trace_flush(); + } +#endif + if (!s_pending_file_listener) { + return; + } + if ((uint32_t)(milliseconds() - s_file_request_sent_ms) < FILE_REQUEST_TIMEOUT_MS) { + return; + } +#ifdef FNC_RX_TRACE + dbg_printf("[macro-chain] request timed out, advancing\n"); +#endif + JsonListener* l = s_pending_file_listener; + s_pending_file_listener = nullptr; + json_reset_depth(); + try_next_macro_file(l); +} + void init_macro_parser() { macro_parser = new JsonStreamingParser(); macro_parser->setListener(¯oLinesListener); @@ -639,6 +811,8 @@ void init_listener() { } void request_file_list(const char* dirname) { + suspend_auto_report(); + s_file_request_sent_ms = milliseconds(); send_linef("$Files/ListGCode=%s", dirname); // parser.reset(); parser_needs_reset = true; @@ -652,6 +826,8 @@ void init_file_list() { void request_file_preview(const char* name, int firstline, int nlines) { reading_macros = false; + suspend_auto_report(); + s_file_request_sent_ms = milliseconds(); send_linef("$File/ShowSome=%d:%d,%s", firstline, firstline + nlines, name); // parser.reset(); } @@ -720,7 +896,7 @@ extern "C" void handle_json(const char* line) { size_t pn = len < 60 ? len : 60; memcpy(peek, line, pn); peek[pn] = '\0'; - dbg_printf("[json] len=%u d=%d | %s%s\n", (unsigned)len, s_json_depth, + trace_defer("[json] len=%u d=%d | %s%s\n", (unsigned)len, s_json_depth, peek, len > 60 ? "..." : ""); #endif // Only reset the parser at a document boundary, never mid-stream — a reset @@ -732,6 +908,13 @@ extern "C" void handle_json(const char* line) { parser.reset(); } parser_feed_line(line); + + // No per-chunk ack is sent. Restoring the 0xB2 that aeddaa9 removed was + // tried here and made things worse: overlapping fragments appeared in the + // stream ("false" arriving as "falalse"), i.e. the sender re-emitting across + // a chunk boundary. The byte loss that motivated the experiment was really + // the WiFi stack starving the UART reader in poll_extra(), which is fixed + // separately. FluidNC's $File/SendJSON does not need the ack. } std::string wifi_mode; diff --git a/src/FileParser.h b/src/FileParser.h index 8de75f9..1c7aba0 100644 --- a/src/FileParser.h +++ b/src/FileParser.h @@ -50,3 +50,11 @@ void json_reset_depth(); // "Reading Macros" UI doesn't hang when a $File/SendJSON request was // rejected. No-op if no file request is currently in flight. extern "C" void file_request_failed_advance(); + +// Poll from dispatch_events(). Bounds how long the macro chain waits on a +// $File/SendJSON that never completes, so the menu can't stall on +// "Reading Macros" when a transfer is torn up on the wire. +void service_macro_chain(); + +// Re-enable FluidNC auto-reporting after a file transfer suspended it. +void resume_auto_report(); diff --git a/src/MacroMenu.cpp b/src/MacroMenu.cpp index cb0848c..ff51fe3 100644 --- a/src/MacroMenu.cpp +++ b/src/MacroMenu.cpp @@ -78,6 +78,10 @@ class MacroMenu : public Menu { void onRedButtonPress() { refreshMacros(); } void onFilesList() { +#ifdef FNC_RX_TRACE + dbg_printf("[macro-menu] onFilesList: %d item(s)%s\n", num_items(), + num_items() ? "" : " <-- screen will read \"No Macros\""); +#endif _error_string.clear(); _reading = false; if (num_items()) { @@ -88,6 +92,9 @@ class MacroMenu : public Menu { } void onError(const char* errstr) { +#ifdef FNC_RX_TRACE + dbg_printf("[macro-menu] onError: \"%s\"\n", errstr); +#endif _error_string = errstr; _reading = false; reDisplay(); diff --git a/src/Scene.cpp b/src/Scene.cpp index 5881bb9..c703af0 100644 --- a/src/Scene.cpp +++ b/src/Scene.cpp @@ -4,6 +4,7 @@ #include "Scene.h" #include "ConfigItem.h" #include "System.h" +#include "FileParser.h" // service_macro_chain() #ifdef USE_WIFI # include "WiFiConnection.h" #endif @@ -187,6 +188,7 @@ void service_redisplay() { void dispatch_events() { update_events(); service_config_requests(); + service_macro_chain(); static int16_t oldEncoder = 0; int16_t newEncoder = get_encoder(); From 34992e3cea365bc6efe94c002e0c67a257c7c53b Mon Sep 17 00:00:00 2001 From: Urs Schaufelberger Date: Wed, 9 Sep 2026 23:36:55 +0200 Subject: [PATCH 7/7] Run command macros from the green button A "cmd:" macro carries a command line rather than a path, so there is no file behind it. invoke() with no argument means "open the file preview", which left the green button and touch doing nothing at all for a command macro -- it was reachable only from the dial. onGreenButtonPress() also returned early unless state was Idle, and $Job/Resume and friends exist precisely for when the machine is NOT idle, which is the one state that guard refuses. Give MacroItem an is_command() accessor, run command macros directly from the green button and touch, and do not gate them on Idle. A command macro is a line of text on the channel; FluidNC rejects what it will not accept in the current state, so blocking it in the pendant adds nothing. File macros keep the existing Load -> Run flow, gated on Idle, unchanged. The legend follows: a command macro shows Run on both buttons and keeps them offered while the machine is busy. Also handle the cmd: prefix in invoke() itself -- splitting the body on newlines and ';' so a multi-statement macro is sent as separate lines, and skipping the preview scene, which has nothing to show for a command. Co-Authored-By: Claude Opus 5 (cherry picked from commit 6114f331e690650e4bfd7949ab94edf239cf8bf7) --- src/MacroItem.h | 3 +++ src/MacroMenu.cpp | 42 +++++++++++++++++++++++++++++++++++------- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/MacroItem.h b/src/MacroItem.h index d6b9439..fb15d1b 100644 --- a/src/MacroItem.h +++ b/src/MacroItem.h @@ -9,6 +9,9 @@ class MacroItem : public Item { public: MacroItem(const char* name, std::string filename) : Item(name), _filename(filename) {} + // A "cmd:" macro carries a command line rather than a path, so there is no + // file behind it to load or preview. + bool is_command() const { return _filename.rfind("cmd:", 0) == 0; } void invoke(void* arg) override; void show(const Point& where) override; }; diff --git a/src/MacroMenu.cpp b/src/MacroMenu.cpp index ff51fe3..962db3f 100644 --- a/src/MacroMenu.cpp +++ b/src/MacroMenu.cpp @@ -5,13 +5,18 @@ #include "MacroItem.h" #include "polar.h" #include "FileParser.h" +#include "FluidNCModel.h" // send_line() extern Scene statusScene; extern Scene filePreviewScene; void MacroItem::invoke(void* arg) { + // CMD macros carry a command line, not a path. Running one as a file + // would nest a job, and commands like $Job/Resume refuse to run with a + // job active - so send the text on this channel instead. + bool is_cmd = _filename.rfind("cmd:", 0) == 0; if (arg && strcmp((char*)arg, "Run") == 0) { - if (_filename.rfind("cmd:", 0) == 0) { + if (is_cmd) { // Split on \n, \r, and ';' — FluidNC parses ';' as a line-comment, // so multi-statement macros like "G0 Z45; G0 Y166" must be sent as // separate lines. Trim whitespace and skip empty segments. @@ -36,7 +41,8 @@ void MacroItem::invoke(void* arg) { } else { send_linef("$Localfs/Run=%s", _filename.c_str()); } - } else { + } else if (!is_cmd) { + // Nothing to preview for a command push_scene(&filePreviewScene, (void*)_filename.c_str()); // doFileScreen(_name); } @@ -112,13 +118,30 @@ class MacroMenu : public Menu { } } + // Only MacroItems are ever added to this menu. + bool selected_is_command() { + return num_items() && static_cast(_items[_selected])->is_command(); + } + void onGreenButtonPress() { - if (state != Idle) { + if (!num_items()) { return; } - if (num_items()) { - invoke(); + if (selected_is_command()) { + // Nothing to load: a command macro has no file to preview, so the + // green button would otherwise do nothing at all and the command + // would be reachable only from the dial. Run it instead. + // + // Deliberately not gated on Idle either -- $Job/Resume and friends + // exist precisely for when the machine is NOT idle, which is the + // one state the guard below would refuse. + invoke((void*)"Run"); + return; + } + if (state != Idle) { + return; } + invoke(); } void onTouchClick() { onGreenButtonPress(); } @@ -157,8 +180,13 @@ class MacroMenu : public Menu { const char* orangeLabel = ""; const char* grnLabel = ""; - if (state == Idle) { - if (num_items()) { + if (num_items()) { + if (selected_is_command()) { + // Both buttons do the same thing here, and both stay offered + // while the machine is busy -- see onGreenButtonPress(). + orangeLabel = "Run"; + grnLabel = "Run"; + } else if (state == Idle) { orangeLabel = "Run"; grnLabel = "Load"; }