From 8e3e23c148cbd347216206dddccef3efbe87c4fe Mon Sep 17 00:00:00 2001 From: Mitch Bradley Date: Thu, 10 Dec 2020 04:34:44 -1000 Subject: [PATCH] Fixed strange WCO values on first load (#702) When loading Grbl_Esp32 into a fresh ESP32, the WCOs would often have strange, very large, values. The problem was the code that tries to propagate data from the old "Eeprom" storage format into the new NVS scheme. The old format had a broken checksum computation that made the checksum so weak that if succeeds about half the time on random data. The solution is to get rid of all that old code. The downside is that migration from a build that uses the old format will lose the WCO values. The user will have to reestablish them. Subsequent updates between different versions that both use the new NVS format will propagate WCO values correctly. --- Grbl_Esp32/src/Eeprom.cpp | 81 -------------------------- Grbl_Esp32/src/Eeprom.h | 61 ------------------- Grbl_Esp32/src/GCode.cpp | 6 ++ Grbl_Esp32/src/GCode.h | 26 +++++++++ Grbl_Esp32/src/Grbl.h | 1 - Grbl_Esp32/src/ProcessSettings.cpp | 1 - Grbl_Esp32/src/SettingsDefinitions.cpp | 18 ++---- 7 files changed, 36 insertions(+), 158 deletions(-) delete mode 100644 Grbl_Esp32/src/Eeprom.cpp delete mode 100644 Grbl_Esp32/src/Eeprom.h diff --git a/Grbl_Esp32/src/Eeprom.cpp b/Grbl_Esp32/src/Eeprom.cpp deleted file mode 100644 index 083570c1..00000000 --- a/Grbl_Esp32/src/Eeprom.cpp +++ /dev/null @@ -1,81 +0,0 @@ -/* - Eeprom.cpp - Coordinate data stored in EEPROM - Part of Grbl - Copyright (c) 2014-2016 Sungeun K. Jeon for Gnea Research LLC - - 2018 - Bart Dring This file was modifed for use on the ESP32 - CPU. Do not use this with Grbl for atMega328P - - Grbl is free software: you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation, either version 3 of the License, or - (at your option) any later version. - Grbl is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - You should have received a copy of the GNU General Public License - along with Grbl. If not, see . -*/ - -#include "Grbl.h" - -void memcpy_to_eeprom_with_checksum(unsigned int destination, const char* source, unsigned int size) { - unsigned char checksum = 0; - for (; size > 0; size--) { - unsigned char data = static_cast(*source++); - // Note: This checksum calculation is broken as described below. - checksum = (checksum << 1) || (checksum >> 7); - checksum += data; - EEPROM.write(destination++, *(source++)); - } - EEPROM.write(destination, checksum); - EEPROM.commit(); -} - -int memcpy_from_eeprom_with_old_checksum(char* destination, unsigned int source, unsigned int size) { - unsigned char data, checksum = 0; - for (; size > 0; size--) { - data = EEPROM.read(source++); - // Note: This checksum calculation is broken - the || should be just | - - // thus making the checksum very weak. - // We leave it as-is so we can read old data after a firmware upgrade. - // The new storage format uses the tagged NVS mechanism, avoiding this bug. - checksum = (checksum << 1) || (checksum >> 7); - checksum += data; - *(destination++) = data; - } - return (checksum == EEPROM.read(source)); -} -int memcpy_from_eeprom_with_checksum(char* destination, unsigned int source, unsigned int size) { - unsigned char data, checksum = 0; - for (; size > 0; size--) { - data = EEPROM.read(source++); - checksum = (checksum << 1) | (checksum >> 7); - checksum += data; - *(destination++) = data; - } - return (checksum == EEPROM.read(source)); -} - -// Read selected coordinate data from EEPROM. Updates pointed coord_data value. -// This is now a compatibility routine that is used to propagate coordinate data -// in the old EEPROM format to the new tagged NVS format. -bool old_settings_read_coord_data(uint8_t coord_select, float* coord_data) { - uint32_t addr = coord_select * (sizeof(float) * N_AXIS + 1) + EEPROM_ADDR_PARAMETERS; - if (!(memcpy_from_eeprom_with_old_checksum((char*)coord_data, addr, sizeof(float) * N_AXIS)) && - !(memcpy_from_eeprom_with_checksum((char*)coord_data, addr, sizeof(float) * MAX_N_AXIS))) { - // Reset with default zero vector - clear_vector_float(coord_data); - // The old code used to rewrite the zeroed data but now that is done - // elsewhere, in a different format. - return false; - } - return true; -} - -// Allow iteration over CoordIndex values -CoordIndex& operator ++ (CoordIndex& i) { - i = static_cast(static_cast(i) + 1); - return i; -} diff --git a/Grbl_Esp32/src/Eeprom.h b/Grbl_Esp32/src/Eeprom.h deleted file mode 100644 index 32d66c65..00000000 --- a/Grbl_Esp32/src/Eeprom.h +++ /dev/null @@ -1,61 +0,0 @@ -#pragma once - -/* - Eeprom.h - Header for system level commands and real-time processes - Part of Grbl - Copyright (c) 2014-2016 Sungeun K. Jeon for Gnea Research LLC - - 2018 - Bart Dring This file was modifed for use on the ESP32 - CPU. Do not use this with Grbl for atMega328P - - Grbl is free software: you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation, either version 3 of the License, or - (at your option) any later version. - Grbl is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - You should have received a copy of the GNU General Public License - along with Grbl. If not, see . -*/ - -#include "Grbl.h" - -// Define EEPROM memory address location values for saved coordinate data. -const int EEPROM_SIZE = 1024U; -const int EEPROM_ADDR_PARAMETERS = 512U; - -//unsigned char eeprom_get_char(unsigned int addr); -//void eeprom_put_char(unsigned int addr, unsigned char new_value); -void memcpy_to_eeprom_with_checksum(unsigned int destination, const char* source, unsigned int size); -int memcpy_from_eeprom_with_checksum(char* destination, unsigned int source, unsigned int size); -int memcpy_from_eeprom_with_old_checksum(char* destination, unsigned int source, unsigned int size); - -// Reads selected coordinate data from EEPROM -bool old_settings_read_coord_data(uint8_t coord_select, float* coord_data); - -// Various places in the code access saved coordinate system data -// by a small integer index according to the values below. -enum CoordIndex : uint8_t{ - Begin = 0, - G54 = Begin, - G55, - G56, - G57, - G58, - G59, - // To support 9 work coordinate systems it would be necessary to define - // the following 3 and modify GCode.cpp to support G59.1, G59.2, G59.3 - // G59_1, - // G59_2, - // G59_3, - NWCSystems, - G28 = NWCSystems, - G30, - // G92_2, - // G92_3, - End, -}; -// Allow iteration over CoordIndex values -CoordIndex& operator ++ (CoordIndex& i); diff --git a/Grbl_Esp32/src/GCode.cpp b/Grbl_Esp32/src/GCode.cpp index 70cce7d3..b1a7a079 100644 --- a/Grbl_Esp32/src/GCode.cpp +++ b/Grbl_Esp32/src/GCode.cpp @@ -24,6 +24,12 @@ #include "Grbl.h" +// Allow iteration over CoordIndex values +CoordIndex& operator++(CoordIndex& i) { + i = static_cast(static_cast(i) + 1); + return i; +} + // NOTE: Max line number is defined by the g-code standard to be 99999. It seems to be an // arbitrary value, and some GUIs may require more. So we increased it based on a max safe // value when converting a float (7.2 digit precision)s to an integer. diff --git a/Grbl_Esp32/src/GCode.h b/Grbl_Esp32/src/GCode.h index 4b070459..1ef3abf6 100644 --- a/Grbl_Esp32/src/GCode.h +++ b/Grbl_Esp32/src/GCode.h @@ -232,6 +232,32 @@ enum GCParserFlags { GCParserLaserIsMotion = bit(7), }; +// Various places in the code access saved coordinate system data +// by a small integer index according to the values below. +enum CoordIndex : uint8_t{ + Begin = 0, + G54 = Begin, + G55, + G56, + G57, + G58, + G59, + // To support 9 work coordinate systems it would be necessary to define + // the following 3 and modify GCode.cpp to support G59.1, G59.2, G59.3 + // G59_1, + // G59_2, + // G59_3, + NWCSystems, + G28 = NWCSystems, + G30, + // G92_2, + // G92_3, + End, +}; + +// Allow iteration over CoordIndex values +CoordIndex& operator ++ (CoordIndex& i); + // NOTE: When this struct is zeroed, the 0 values in the above types set the system defaults. typedef struct { Motion motion; // {G0,G1,G2,G3,G38.2,G80} diff --git a/Grbl_Esp32/src/Grbl.h b/Grbl_Esp32/src/Grbl.h index 5b1bf3db..31413e8a 100644 --- a/Grbl_Esp32/src/Grbl.h +++ b/Grbl_Esp32/src/Grbl.h @@ -41,7 +41,6 @@ const char* const GRBL_VERSION_BUILD = "20201207"; #include "Defaults.h" #include "Error.h" -#include "Eeprom.h" #include "WebUI/Authentication.h" #include "WebUI/Commands.h" #include "Probe.h" diff --git a/Grbl_Esp32/src/ProcessSettings.cpp b/Grbl_Esp32/src/ProcessSettings.cpp index dc19fd65..a6831713 100644 --- a/Grbl_Esp32/src/ProcessSettings.cpp +++ b/Grbl_Esp32/src/ProcessSettings.cpp @@ -79,7 +79,6 @@ namespace WebUI { } void settings_init() { - EEPROM.begin(EEPROM_SIZE); make_settings(); WebUI::make_web_settings(); make_grbl_commands(); diff --git a/Grbl_Esp32/src/SettingsDefinitions.cpp b/Grbl_Esp32/src/SettingsDefinitions.cpp index c25f35a5..2c0ac6c4 100644 --- a/Grbl_Esp32/src/SettingsDefinitions.cpp +++ b/Grbl_Esp32/src/SettingsDefinitions.cpp @@ -196,9 +196,9 @@ static bool checkSpindleChange(char* val) { } grbl_msg_sendf(CLIENT_ALL, MsgLevel::Info, "Spindle turned off with setting change"); } - gc_state.spindle_speed = 0; // Set S value to 0 - spindle->deinit(); // old spindle - Spindles::Spindle::select(); // get new spindle + gc_state.spindle_speed = 0; // Set S value to 0 + spindle->deinit(); // old spindle + Spindles::Spindle::select(); // get new spindle return true; } return true; @@ -219,17 +219,7 @@ void make_coordinate(CoordIndex index, const char* name) { auto coord = new Coordinates(name); coords[index] = coord; if (!coord->load()) { - grbl_msg_sendf(CLIENT_SERIAL, MsgLevel::Info, "Propagating %s data to NVS format", coord->getName()); - // If coord->load() returns false it means that no entry - // was present in non-volatile storage. In that case we - // first look for an old-format entry in the EEPROM section. - // If an entry is present some number of float values at - // the beginning of coord_data will be overwritten with - // the EEPROM data, and the rest will remain at 0.0. - // If no old-format entry is present, all will remain 0.0 - // Regardless, we create a new entry with that data. - (void)old_settings_read_coord_data(index, coord_data); - coords[index]->set(coord_data); + coords[index]->setDefault(); } } void make_settings() {