From dcba477058479660952d36e2b25bf8b7215667d2 Mon Sep 17 00:00:00 2001 From: Rob Giseburt Date: Mon, 6 Feb 2017 15:20:20 -0600 Subject: [PATCH] Improve handling of too-long lines, and up minimum buffers to 512bytes. Fixes #206 --- g2core/config.cpp | 3 +- g2core/config.h | 2 +- g2core/controller.h | 5 +- g2core/error.h | 3 - g2core/gcode_parser.cpp | 2 +- g2core/main.cpp | 1 - g2core/planner.cpp | 2 +- g2core/report.cpp | 5 +- g2core/xio.cpp | 122 ++++++++++++++++++++-------------------- g2core/xio.h | 4 +- 10 files changed, 71 insertions(+), 78 deletions(-) diff --git a/g2core/config.cpp b/g2core/config.cpp index 5e3574be1..b78b369de 100644 --- a/g2core/config.cpp +++ b/g2core/config.cpp @@ -178,8 +178,7 @@ stat_t config_test_assertions() (BAD_MAGIC(nvl.magic_start)) || (BAD_MAGIC(nvl.magic_end)) || (BAD_MAGIC(nvStr.magic_start)) || - (BAD_MAGIC(nvStr.magic_end)) || - (global_string_buf[GLOBAL_STRING_LEN-1] != NUL)) { + (BAD_MAGIC(nvStr.magic_end))) { return(cm_panic(STAT_CONFIG_ASSERTION_FAILURE, "config_test_assertions()")); } return (STAT_OK); diff --git a/g2core/config.h b/g2core/config.h index 106dc81b6..b60678e14 100644 --- a/g2core/config.h +++ b/g2core/config.h @@ -177,7 +177,7 @@ typedef uint16_t index_t; // use this if there are > 255 indexed o #define NV_MESSAGE_LEN 128 // sufficient space to contain end-user messages // pre-allocated defines (take RAM permanently) -#define NV_SHARED_STRING_LEN 512 // shared string for string values +#define NV_SHARED_STRING_LEN 1024 // shared string for string values #define NV_BODY_LEN 40 // body elements - allow for 1 parent + N children #define NV_EXEC_LEN 10 // elements reserved for exec, which won't directly respond // (each body element takes about 30 bytes of RAM) diff --git a/g2core/controller.h b/g2core/controller.h index f1630f8a7..d040590c2 100755 --- a/g2core/controller.h +++ b/g2core/controller.h @@ -28,9 +28,10 @@ #ifndef CONTROLLER_H_ONCE #define CONTROLLER_H_ONCE +#include "xio.h" + // see also: g2core.h MESSAGE_LEN and config.h NV_ lengths -#define SAVED_BUFFER_LEN 80 // saved buffer size (for reporting only) -#define MAXED_BUFFER_LEN 255 // same as streaming RX buffer size as a worst case +#define SAVED_BUFFER_LEN RX_BUFFER_SIZE // saved buffer size (for reporting only) #define OUTPUT_BUFFER_LEN 512 // text buffer size #define LED_NORMAL_BLINK_RATE 3000 // blink rate for normal operation (in ms) diff --git a/g2core/error.h b/g2core/error.h index 0376ad5a6..b7b00c593 100644 --- a/g2core/error.h +++ b/g2core/error.h @@ -53,9 +53,6 @@ typedef uint8_t stat_t; extern stat_t status_code; -#define GLOBAL_STRING_LEN 256 // allow sufficient space for JSON responses and message strings -extern char global_string_buf[]; - char *get_status_message(stat_t status); // ritorno is a handy way to provide exception returns diff --git a/g2core/gcode_parser.cpp b/g2core/gcode_parser.cpp index cfeac90ab..2eaead6f4 100644 --- a/g2core/gcode_parser.cpp +++ b/g2core/gcode_parser.cpp @@ -249,7 +249,7 @@ stat_t _verify_checksum(char *str) * - block_delete_flag is set true if block delete encountered, false otherwise */ -char _normalize_scratch[RX_BUFFER_MIN_SIZE]; +char _normalize_scratch[RX_BUFFER_SIZE]; void _normalize_gcode_block(char *str, char **active_comment, uint8_t *block_delete_flag) { diff --git a/g2core/main.cpp b/g2core/main.cpp index 625d38e5c..7667e6a4f 100644 --- a/g2core/main.cpp +++ b/g2core/main.cpp @@ -49,7 +49,6 @@ /******************** System Globals *************************/ stat_t status_code; // allocate a variable for the ritorno macro -char global_string_buf[GLOBAL_STRING_LEN]; // allocate a string for global message use /************* System Globals For Diagnostics ****************/ diff --git a/g2core/planner.cpp b/g2core/planner.cpp index efec41f46..83408aee3 100644 --- a/g2core/planner.cpp +++ b/g2core/planner.cpp @@ -72,7 +72,7 @@ mpMotionRuntimeSingleton_t mr; // context for block runtime #define JSON_COMMAND_BUFFER_SIZE 3 struct json_command_buffer_t { - char buf[RX_BUFFER_MIN_SIZE]; + char buf[RX_BUFFER_SIZE]; json_command_buffer_t *pv; json_command_buffer_t *nx; }; diff --git a/g2core/report.cpp b/g2core/report.cpp index d05bbda2d..5305368b8 100644 --- a/g2core/report.cpp +++ b/g2core/report.cpp @@ -59,9 +59,10 @@ stat_t rpt_exception(stat_t status, const char *msg) // you cannot send an exception report if the USB has not been set up. Causes a processor exception. if (cs.controller_state >= CONTROLLER_READY) { - sprintf(global_string_buf, "{\"er\":{\"fb\":%0.2f,\"st\":%d,\"msg\":\"%s - %s\"}}\n", + char buffer[128]; + sprintf(buffer, "{\"er\":{\"fb\":%0.2f,\"st\":%d,\"msg\":\"%s - %s\"}}\n", G2CORE_FIRMWARE_BUILD, status, get_status_message(status), msg); - xio_writeline(global_string_buf); + xio_writeline(buffer); } } return (status); // makes it possible to inline, e.g: return(rpt_exception(status)); diff --git a/g2core/xio.cpp b/g2core/xio.cpp index 374604cf6..5eb4dc3bf 100755 --- a/g2core/xio.cpp +++ b/g2core/xio.cpp @@ -49,6 +49,8 @@ using Motate::TXBuffer; #include "text_parser.h" #endif +// defines for assertions + /**** HIGH LEVEL EXPLANATION OF XIO **** * * The XIO subsystem serves three purposes: @@ -102,14 +104,6 @@ struct xioDeviceWrapperBase { // C++ base class for device primit devflags_t flags; // bitfield for device state flags (these are not) devflags_t next_flags; // bitfield for next-state transitions - // line reader functions -// uint16_t read_index; // index into line being read -// const uint16_t read_buf_size; // static variable set at init time -// char read_buf[USB_LINE_BUFFER_SIZE]; // buffer for reading lines - - // Internal use only: -// bool _ready_to_send; - // Checks against class flags variable: // bool canRead() { return caps & DEV_CAN_READ; } // bool canWrite() { return caps & DEV_CAN_WRITE; } @@ -422,7 +416,7 @@ extern xio_t xio; // LineRXBuffer takes the Motate RXBuffer (which handles "transfers", usually DMA), and adds G2 line-reading // semantics to it. -template +template struct LineRXBuffer : RXBuffer<_size, owner_type, char> { typedef RXBuffer<_size, owner_type, char> parent_type; @@ -440,7 +434,7 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { // START OF LineRXBuffer PROPER static_assert(((_header_count-1)&_header_count)==0, "_header_count must be 2^N"); - char _line_buffer[_line_buffer_size]; // hold exactly one line to return + char _line_buffer[_line_buffer_size+1]; // hold exactly one line to return uint32_t _line_end_guard = 0xBEEF; // General term usage: @@ -448,7 +442,9 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { // * "offset" means it's a character in the _data array uint16_t _scan_offset; // offset into data of the last character scanned - uint16_t _line_start_offset; // offset into first character of the line + uint16_t _line_start_offset; // offset into first character of the line, or the first char to ignore (too-long lines) + uint16_t _last_line_length; // used for ensuring lines aren't too long + bool _ignore_until_next_line; // if we get a too-long-line, we ignore the rest by setting this flag bool _at_start_of_line; // true if the last character scanned was the end of a line uint16_t _lines_found; // count of complete non-control lines that were found during scanning. @@ -728,13 +724,30 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { #endif // Look for line endings if (c == '\r' || c == '\n') { - if (!_at_start_of_line) { // We only mark ends_line for the first end-line char, and if + if (_ignore_until_next_line) { + // we finally ended the line we were ignoring + // add a skip section to jump over the overage + _skip_sections.addSkip(_line_start_offset, _scan_offset); + // move the start of the next skip section to after this skip + _line_start_offset = _scan_offset; + + // we DON'T want to end it normally (by counting a line) + _at_start_of_line = true; + _ignore_until_next_line = false; + + _last_line_length = 0; + } + else if (!_at_start_of_line) { // We only mark ends_line for the first end-line char, and if ends_line = true; // _at_start_of_line is already true, this is not the first. } } + // prevent going furnther if we are ignoring + else if (_ignore_until_next_line) + { + // don't do anything + } // Classify the line if it's a single character - else - if (_at_start_of_line && + else if (_at_start_of_line && ((c == '!') || (c == '~') || (c == ENQ) || // request ENQ/ack @@ -754,12 +767,14 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { if (_at_start_of_line) { // This is the first character at the beginning of the line. _line_start_offset = _scan_offset; + _last_line_length = 0; } _at_start_of_line = false; } // bump the _scan_offset _scan_offset = _getNextScanOffset(); + _last_line_length++; if (ends_line) { // _scan_offset is now one past the end of the line, @@ -767,7 +782,7 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { _at_start_of_line = true; // Here we classify the line. - // If we are is_control is already true, it's an already classified + // If is_control is already true, it's an already classified // single-character command. if (!is_control) { // TODO --- Call a function to do this @@ -793,7 +808,25 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { _lines_found++; } } // if ends_line + else if (_last_line_length == (_line_buffer_size - 1)) { + // force an end-of-line, splitting this line into two lines + _ignore_until_next_line = true; + _line_start_offset = _scan_offset; + _lines_found++; + } } //while (_isMoreToScan()) + + // special edge case: we ran out of items to scan (buffer full?), but we're ignoring because a line was too long + // example: we get a line that it multiple-times the length of the buffer + // so we'll dump skip sections to the readline will move the read pointer forward + if (_ignore_until_next_line && (_line_start_offset != _scan_offset)) { + // add a skip section to jump over the overage + _skip_sections.addSkip(_line_start_offset, _scan_offset); + // move the start of the next skip section to after this skip + _line_start_offset = _scan_offset; + } + + return false; // no control was found }; @@ -842,11 +875,8 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { } } - while ((_scan_offset != _line_start_offset) && - (line_size < (_line_buffer_size - 2))) { -// if (!_canBeRead(read_offset)) { // This test should NEVER fail. -// _debug_trap("readline hit unreadable and shouldn't have!"); -// } + // note that if it's marked as a control, it's guaranteed to fit in the line buffer + while (_scan_offset != _line_start_offset) { // copy the charater to _line_buffer char c = _data[_line_start_offset]; @@ -866,38 +896,6 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { if (ctrl_is_at_beginning_of_data) { _read_offset = _scan_offset; } -// else { -// // special case: if the return value is '%' -// // then we actually consider everything before it to be read -// -// if ('%' == _line_buffer[0]) { -// // Things that must be managed here: -// // * _read_offset -- we're skipping data -// // * _lines_found -- we shouldn't have any lines "left" -// // * _skip_sections -- there's nothing to skip, we just did -// -// // Things that won't be changed (further): -// // * _scan_offset -- we're not changing past where it's scanned -// // * _line_start_offset -- we've already adjusted it -// // * _at_start_of_line -- should always be true when we're here -// -// // move the read buffer up to where we're scanning -// _read_offset = _scan_offset; -// -// // record that we have 0 lines (of data) in the buffer -// _lines_found = 0; -// -// // and clear out any skip sections we have -// while (!_skip_sections.isEmpty()) { -// _skip_sections.popSkip(); -// } -// } -// } - -// if (ctrl_is_at_beginning_of_data) { -// // attempt to request more data -// _restartTransfer(); -// } return _line_buffer; } // end if (found_control) @@ -907,13 +905,16 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { return nullptr; } + // skip sections will always start at the beginning of a line + // handle this, even with no line, in case we're ignoring a huge too-long line + _skip_sections.skip(_read_offset); + if (_lines_found == 0) { // nothing to return line_size = 0; return nullptr; } - // By the time we get here, we know we have at least one line in _data. @@ -921,9 +922,6 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { _debug_trap("read ran into NULL"); } - // skip sections will always start at the beginning of a line - _skip_sections.skip(_read_offset); - // scan past any leftover CR or LF from the previous line char c = _data[_read_offset]; while ((c == '\n') || (c == '\r')) { @@ -936,11 +934,7 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { c = _data[_read_offset]; } - while (line_size < (_line_buffer_size - 2)) { -// if (!_canBeRead(read_offset)) { // This test should NEVER fail. -// _debug_trap("readline hit unreadable and shouldn't have!"); -// } - + while (line_size < (_line_buffer_size - 1)) { _read_offset = (_read_offset+1)&(_size-1); if ( c == '\r' || @@ -958,6 +952,10 @@ struct LineRXBuffer : RXBuffer<_size, owner_type, char> { c = _data[_read_offset]; } + if (line_size == (_line_buffer_size - 1)) { + // add a line-ending + *dst_ptr++ = '\n'; + } --_lines_found; @@ -1221,7 +1219,7 @@ struct xioDeviceWrapper : xioDeviceWrapperBase { // describes a device for re // Specialization for xio_flash_file -- we don't need most of the structure around a Device for xio_flash_file -template +template struct xioFlashFileDeviceWrapper : xioDeviceWrapperBase { // describes a device for reading and writing xio_flash_file *_current_file = nullptr; diff --git a/g2core/xio.h b/g2core/xio.h index 065fafe21..f2294fe97 100755 --- a/g2core/xio.h +++ b/g2core/xio.h @@ -61,8 +61,6 @@ #undef _FDEV_EOF #define _FDEV_EOF -2 -#define USB_LINE_BUFFER_SIZE 255 // text buffer size - //*** Device flags *** typedef uint16_t devflags_t; // might need to bump to 32 be 16 or 32 @@ -111,7 +109,7 @@ enum xioSPIMode { /**** readline stuff *****/ -#define RX_BUFFER_MIN_SIZE 256 // minimum requested buffer size (they are usually larger) +#define RX_BUFFER_SIZE 512 // maximum length of recieved lines from xio_readline /**** function prototypes ****/