From 3145e8be241074db8c88c5544500226143f0927f Mon Sep 17 00:00:00 2001 From: Max Prokhorov Date: Fri, 3 Jul 2020 22:07:48 +0300 Subject: [PATCH] Rework settings (#2282) * wip based on early draft. todo benchmarking * fixup eraser, assume keys are unique * fix cursor copy, test removal at random * small benchmark via permutations. todo lambdas and novirtual * fix empty condition / reset * overwrite optimizations, fix move offsets overflows * ...erase using 0xff instead of 0 * test the theory with code, different length kv were bugged * try to check for out-of-bounds writes / reads * style * trying to fix mover again * clarify length, defend against reading len on edge * fix uncommited rewind change * prove space assumptions * more concise traces, fix move condition (agrh!!!) * slightly more internal knowledge (estimates API?) * make sure cursor is only valid within the range * ensure 0 does not blow things * go back up * cursor comments * comments * rewrite writes through cursor * in del too * estimate kv storage requirements, return available size * move raw erase / move into a method, allow ::set to avoid scanning storage twice * refactor naming, use in code * amend storage slicing test * fix crash handler offsets, cleanup configuration * start -> begin * eeprom readiness * dependencies * unused * SPI_FLASH constants for older Core * vtables -> templates * less include dependencies * gcov help, move estimate outside of the class * writer position can never match, use begin + offset * tweak save_crash to trigger only once in a serious crash * doh, header function should be inline * foreach api, tweak structs for public api * use test helper class * when not using foreach, move cursor reset closer to the loop using read_kv * coverage comments, fix typo in tests decltype * ensure set() does not break with offset * make codacy happy again --- code/espurna/board.cpp | 2 +- code/espurna/config/all.h | 1 + code/espurna/config/general.h | 11 - code/espurna/crash.cpp | 243 +++++++---- code/espurna/crash.h | 29 +- code/espurna/debug.cpp | 2 +- code/espurna/espurna.h | 2 +- code/espurna/gpio.cpp | 2 +- code/espurna/relay.cpp | 11 - code/espurna/settings.cpp | 184 +++------ code/espurna/settings.h | 42 +- code/espurna/settings_embedis.h | 674 +++++++++++++++++++++++++++++++ code/espurna/storage_eeprom.cpp | 18 +- code/espurna/storage_eeprom.h | 13 + code/espurna/system.cpp | 2 +- code/espurna/telnet.cpp | 3 +- code/espurna/terminal.cpp | 2 +- code/espurna/utils.cpp | 5 +- code/espurna/web.cpp | 5 +- code/platformio.ini | 1 - code/test/platformio.ini | 19 + code/test/unit/settings/main.cpp | 487 ++++++++++++++++++++++ 22 files changed, 1484 insertions(+), 274 deletions(-) create mode 100644 code/espurna/settings_embedis.h create mode 100644 code/test/unit/settings/main.cpp diff --git a/code/espurna/board.cpp b/code/espurna/board.cpp index a6927712..4cc25ddc 100644 --- a/code/espurna/board.cpp +++ b/code/espurna/board.cpp @@ -4,7 +4,7 @@ BOARD MODULE */ -#include "board.h" +#include "espurna.h" #include "relay.h" #include "sensor.h" diff --git a/code/espurna/config/all.h b/code/espurna/config/all.h index 1fd9678e..29a94439 100644 --- a/code/espurna/config/all.h +++ b/code/espurna/config/all.h @@ -24,6 +24,7 @@ #include #include #include +#include #ifdef USE_CUSTOM_H #include "custom.h" diff --git a/code/espurna/config/general.h b/code/espurna/config/general.h index 84623c8d..927d62f7 100644 --- a/code/espurna/config/general.h +++ b/code/espurna/config/general.h @@ -214,21 +214,10 @@ // EEPROM //------------------------------------------------------------------------------ -#define EEPROM_SIZE SPI_FLASH_SEC_SIZE // EEPROM size in bytes (1 sector = 4096 bytes) - //#define EEPROM_RORATE_SECTORS 2 // Number of sectors to use for EEPROM rotation // If not defined the firmware will use a number based // on the number of available sectors -#define EEPROM_RELAY_STATUS 0 // Address for the relay status (1 byte) -#define EEPROM_ENERGY_COUNT 1 // Address for the energy counter (4 bytes) -#define EEPROM_CUSTOM_RESET 5 // Address for the reset reason (1 byte) -#define EEPROM_CRASH_COUNTER 6 // Address for the crash counter (1 byte) -#define EEPROM_MESSAGE_ID 7 // Address for the MQTT message id (4 bytes) -#define EEPROM_ROTATE_DATA 11 // Reserved for the EEPROM_ROTATE library (3 bytes) -#define EEPROM_DATA_END 14 // End of custom EEPROM data block - - #ifndef SAVE_CRASH_ENABLED #define SAVE_CRASH_ENABLED 1 // Save stack trace to EEPROM by default // Depends on DEBUG_SUPPORT == 1 diff --git a/code/espurna/crash.cpp b/code/espurna/crash.cpp index e5701692..aa9e5707 100644 --- a/code/espurna/crash.cpp +++ b/code/espurna/crash.cpp @@ -1,6 +1,15 @@ +/* + +Part of the DEBUG MODULE + +Copyright (C) 2016-2019 by Xose Pérez +Copyright (C) 2019-2020 by Maxim Prokhorov + +*/ + // ----------------------------------------------------------------------------- // Save crash info -// Taken from krzychb EspSaveCrash +// Original code by @krzychb // https://github.com/krzychb/EspSaveCrash // ----------------------------------------------------------------------------- @@ -14,9 +23,13 @@ #include "system.h" #include "storage_eeprom.h" -uint16_t _save_crash_stack_trace_max = SAVE_CRASH_STACK_TRACE_MAX; bool _save_crash_enabled = true; +size_t crashReservedSize() { + if (!_save_crash_enabled) return 0; + return CrashReservedSize; +} + /** * Save crash information in EEPROM * This function is called automatically if ESP8266 suffers an exception @@ -25,49 +38,60 @@ bool _save_crash_enabled = true; */ extern "C" void custom_crash_callback(struct rst_info * rst_info, uint32_t stack_start, uint32_t stack_end ) { - // Do not record crash data when resetting the board + // Small safeguard to protect from calling crash handler very early on boot. + if (!eepromReady()) { + return; + } + + // If we crash more than once in a row, don't store (similar) crash log every time + if (systemStabilityCounter() > 1) { + return; + } + + // Do not record crash data when doing a normal reboot or when crash trace was disabled if (checkNeedsReset()) { return; } - // Check if runtime setting disabled this callback if (!_save_crash_enabled) { return; } - // write crash time to EEPROM, which we will later use as a marker that there was a crash + // We will use this later as a marker that there was a crash uint32_t crash_time = millis(); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_CRASH_TIME, crash_time); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_CRASH_TIME, crash_time); - // rst_info::reason and ::exccause are uint32_t, but are holding small values - EEPROMr.write(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_RESTART_REASON, rst_info->reason); - EEPROMr.write(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EXCEPTION_CAUSE, rst_info->exccause); + // XXX rst_info::reason and ::exccause are uint32_t, but are holding small values + // make sure we are using ::write() instead of ::put(), former tries to deduce the required size based on variable type + EEPROMr.write(EepromCrashBegin + SAVE_CRASH_RESTART_REASON, + static_cast(rst_info->reason)); + EEPROMr.write(EepromCrashBegin + SAVE_CRASH_EXCEPTION_CAUSE, + static_cast(rst_info->exccause)); // write epc1, epc2, epc3, excvaddr and depc to EEPROM as uint32_t - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EPC1, rst_info->epc1); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EPC2, rst_info->epc2); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EPC3, rst_info->epc3); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EXCVADDR, rst_info->excvaddr); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_DEPC, rst_info->depc); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_EPC1, rst_info->epc1); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_EPC2, rst_info->epc2); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_EPC3, rst_info->epc3); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_EXCVADDR, rst_info->excvaddr); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_DEPC, rst_info->depc); // EEPROM size is limited, write as little as possible. - // we sometimes want to avoid big stack traces, e.g. if stack_end == 0x3fffffb0, we are in SYS context. + // we definitely want to avoid big stack traces, e.g. like when stack_end == 0x3fffffb0 and we are in SYS context. // but still should get enough relevant info and it is possible to set needed size at build/runtime - const uint16_t stack_size = constrain((stack_end - stack_start), 0, _save_crash_stack_trace_max); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_STACK_START, stack_start); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_STACK_END, stack_end); - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_STACK_SIZE, stack_size); - - // starting EEPROM address of Embedis data plus reserve - const uint16_t settings_start = ( - ((SPI_FLASH_SEC_SIZE - settingsSize() + 31) & -32) - 0x20); - - // write stack trace to EEPROM and avoid overwriting settings - int16_t eeprom_addr = SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_STACK_TRACE; - for (uint32_t* addr = (uint32_t*)stack_start; addr < (uint32_t*)(stack_start + stack_size); addr++) { - if (eeprom_addr >= settings_start) break; + const uint16_t stack_size = constrain((stack_end - stack_start), 0, CrashReservedSize); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_STACK_START, stack_start); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_STACK_END, stack_end); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_STACK_SIZE, stack_size); + + // write stack trace to EEPROM and avoid overwriting settings and reserved data + // [EEPROM RESERVED SPACE] >>> ... CRASH DATA ... >>> [SETTINGS] + int eeprom_addr = EepromCrashBegin + SAVE_CRASH_STACK_TRACE; + + auto *addr = reinterpret_cast(stack_start); + while (EepromCrashEnd > eeprom_addr) { EEPROMr.put(eeprom_addr, *addr); eeprom_addr += sizeof(uint32_t); + ++addr; } EEPROMr.commit(); @@ -75,92 +99,149 @@ extern "C" void custom_crash_callback(struct rst_info * rst_info, uint32_t stack } /** - * Clears crash info + * Clears crash info CRASH_TIME value, later checked in crashDump() */ void crashClear() { uint32_t crash_time = 0xFFFFFFFF; - EEPROMr.put(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_CRASH_TIME, crash_time); + EEPROMr.put(EepromCrashBegin + SAVE_CRASH_CRASH_TIME, crash_time); EEPROMr.commit(); } +namespace { + /** * Print out crash information that has been previusly saved in EEPROM + * We can optionally check for the recorded crash time before proceeding. */ -void crashDump() { +void _crashDump(Print& print, bool check) { + + char buffer[256] = {0}; uint32_t crash_time; - EEPROMr.get(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_CRASH_TIME, crash_time); - if ((crash_time == 0) || (crash_time == 0xFFFFFFFF)) { - DEBUG_MSG_P(PSTR("[DEBUG] No crash info\n")); + EEPROMr.get(EepromCrashBegin + SAVE_CRASH_CRASH_TIME, crash_time); + + bool crash_time_erased = ((crash_time == 0) || (crash_time == 0xFFFFFFFF)); + if (check && crash_time_erased) { return; } - DEBUG_MSG_P(PSTR("[DEBUG] Latest crash was at %lu ms after boot\n"), crash_time); - DEBUG_MSG_P(PSTR("[DEBUG] Reason of restart: %u\n"), EEPROMr.read(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_RESTART_REASON)); - DEBUG_MSG_P(PSTR("[DEBUG] Exception cause: %u\n"), EEPROMr.read(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EXCEPTION_CAUSE)); + uint8_t reason = EEPROMr.read(EepromCrashBegin + SAVE_CRASH_RESTART_REASON); + if (!crash_time_erased) { + snprintf_P(buffer, sizeof(buffer), PSTR("\nLatest crash was at %lu ms after boot\n"), crash_time); + print.print(buffer); + } - uint32_t epc1, epc2, epc3, excvaddr, depc; - EEPROMr.get(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EPC1, epc1); - EEPROMr.get(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EPC2, epc2); - EEPROMr.get(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EPC3, epc3); - EEPROMr.get(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_EXCVADDR, excvaddr); - EEPROMr.get(SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_DEPC, depc); + snprintf_P(buffer, sizeof(buffer), PSTR("Reason of restart: %u\n"), reason); + print.print(buffer); - DEBUG_MSG_P(PSTR("[DEBUG] epc1=0x%08x epc2=0x%08x epc3=0x%08x\n"), epc1, epc2, epc3); - DEBUG_MSG_P(PSTR("[DEBUG] excvaddr=0x%08x depc=0x%08x\n"), excvaddr, depc); + if (reason == REASON_EXCEPTION_RST) { + snprintf_P(buffer, sizeof(buffer), PSTR("\nException (%u):\n"), + EEPROMr.read(EepromCrashBegin + SAVE_CRASH_EXCEPTION_CAUSE)); + print.print(buffer); + + uint32_t epc1, epc2, epc3, excvaddr, depc; + EEPROMr.get(EepromCrashBegin + SAVE_CRASH_EPC1, epc1); + EEPROMr.get(EepromCrashBegin + SAVE_CRASH_EPC2, epc2); + EEPROMr.get(EepromCrashBegin + SAVE_CRASH_EPC3, epc3); + EEPROMr.get(EepromCrashBegin + SAVE_CRASH_EXCVADDR, excvaddr); + EEPROMr.get(EepromCrashBegin + SAVE_CRASH_DEPC, depc); + + snprintf_P(buffer, sizeof(buffer), PSTR("epc1=0x%08x epc2=0x%08x epc3=0x%08x excvaddr=0x%08x depc=0x%08x\n"), + epc1, epc2, epc3, excvaddr, depc); + print.print(buffer); + } + + // We need something like + // + //>>>stack>>> + // + //ctx: sys + //sp: 3fffecf0 end: 3fffffb0 offset: 0000 + //...addresses... + //...addresses... + //...addresses... + //...addresses... + //<<>>stack>>>\n\nctx: todo\nsp: %08x end: %08x offset: 0000\n"), + stack_start, stack_end + ); + print.print(buffer); - DEBUG_MSG_P(PSTR("[DEBUG] >>>stack>>>\n[DEBUG] ")); + constexpr auto step = sizeof(uint32_t); + int eeprom_addr = EepromCrashBegin + SAVE_CRASH_STACK_TRACE; uint16_t offset = 0; - do { - DEBUG_MSG_P(PSTR("%08x: "), stack_start + offset); - for (byte b = 0; b < 4; b++) { - EEPROMr.get(current_address, stack_trace); - DEBUG_MSG_P(PSTR("%08x "), stack_trace); - current_address += 4; - } - DEBUG_MSG_P(PSTR("\n[DEBUG] ")); - } while ((offset < stack_size) && (offset += 0x10)); - DEBUG_MSG_P(PSTR("<< +Copyright (C) 2019-2020 by Maxim Prokhorov + +*/ + // ----------------------------------------------------------------------------- // Save crash info -// Taken from krzychb EspSaveCrash +// Original code by @krzychb // https://github.com/krzychb/EspSaveCrash // ----------------------------------------------------------------------------- @@ -11,8 +20,6 @@ #include #include -#define SAVE_CRASH_EEPROM_OFFSET 0x0100 // initial address for crash data - /** * Structure of the single crash data set * @@ -41,12 +48,18 @@ #define SAVE_CRASH_STACK_START 0x1A // 4 bytes #define SAVE_CRASH_STACK_END 0x1E // 4 bytes #define SAVE_CRASH_STACK_SIZE 0x22 // 2 bytes -#define SAVE_CRASH_STACK_TRACE 0x24 // variable +#define SAVE_CRASH_STACK_TRACE 0x24 // variable, 4 bytes per value + +constexpr int EepromCrashBegin = EepromReservedSize; +constexpr int EepromCrashEnd = 256; -constexpr size_t crashUsedSpace() { - return (SAVE_CRASH_EEPROM_OFFSET + SAVE_CRASH_STACK_SIZE + 2); -} +constexpr size_t CrashReservedSize = EepromCrashEnd - EepromCrashBegin; +constexpr size_t CrashTraceReservedSize = CrashReservedSize - SAVE_CRASH_STACK_TRACE; +size_t crashReservedSize(); + +void crashForceDump(Print&); +void crashDump(Print&); void crashClear(); -void crashDump(); + void crashSetup(); diff --git a/code/espurna/debug.cpp b/code/espurna/debug.cpp index 31298a33..3818cf09 100644 --- a/code/espurna/debug.cpp +++ b/code/espurna/debug.cpp @@ -6,7 +6,7 @@ Copyright (C) 2016-2019 by Xose Pérez */ -#include "debug.h" +#include "espurna.h" #if DEBUG_SUPPORT diff --git a/code/espurna/espurna.h b/code/espurna/espurna.h index 5461cf71..2d8e141d 100644 --- a/code/espurna/espurna.h +++ b/code/espurna/espurna.h @@ -28,8 +28,8 @@ along with this program. If not, see . #include "board.h" #include "debug.h" #include "gpio.h" -#include "settings.h" #include "storage_eeprom.h" +#include "settings.h" #include "system.h" #include "terminal.h" #include "utils.h" diff --git a/code/espurna/gpio.cpp b/code/espurna/gpio.cpp index dc0a4d6f..14ba9f12 100644 --- a/code/espurna/gpio.cpp +++ b/code/espurna/gpio.cpp @@ -6,7 +6,7 @@ Copyright (C) 2017-2019 by Xose Pérez */ -#include "gpio.h" +#include "espurna.h" #include diff --git a/code/espurna/relay.cpp b/code/espurna/relay.cpp index f8f36a3c..4fec0b32 100644 --- a/code/espurna/relay.cpp +++ b/code/espurna/relay.cpp @@ -747,17 +747,6 @@ PayloadStatus relayParsePayload(const char * payload) { // BACKWARDS COMPATIBILITY void _relayBackwards() { - #if defined(EEPROM_RELAY_STATUS) - { - uint8_t mask = EEPROMr.read(EEPROM_RELAY_STATUS); - if (mask != 0xff) { - _relayMaskSettings(static_cast(mask)); - EEPROMr.write(EEPROM_RELAY_STATUS, 0xff); - eepromCommit(); - } - } - #endif - for (unsigned char id = 0; id < _relays.size(); ++id) { const settings_key_t key {"mqttGroupInv", id}; if (!hasSetting(key)) continue; diff --git a/code/espurna/settings.cpp b/code/espurna/settings.cpp index 70069090..db0b8790 100644 --- a/code/espurna/settings.cpp +++ b/code/espurna/settings.cpp @@ -6,30 +6,41 @@ Copyright (C) 2016-2019 by Xose Pérez */ -#include "settings.h" +#include "espurna.h" +#include "crash.h" #include "terminal.h" +#include "storage_eeprom.h" +#include #include #include #include -#include "storage_eeprom.h" - BrokerBind(ConfigBroker); -// ----------------------------------------------------------------------------- -// (HACK) Embedis storage format, reverse engineered // ----------------------------------------------------------------------------- -unsigned long settingsSize() { - unsigned pos = SPI_FLASH_SEC_SIZE - 1; - while (size_t len = EEPROMr.read(pos)) { - if (0xFF == len) break; - pos = pos - len - 2; - } - return SPI_FLASH_SEC_SIZE - pos + EEPROM_DATA_END; +namespace settings { + +// Depending on features enabled, we may end up with different left boundary +// Settings are written right-to-left, so we only have issues when there are a lot of key-values +// XXX: slightly hacky, because we EEPROMr.length() is 0 before we enter setup() code +kvs_type kv_store( + EepromStorage{}, +#if DEBUG_SUPPORT + EepromReservedSize + CrashReservedSize, +#else + EepromReservedSize, +#endif + EepromSize +); + +} // namespace settings + +size_t settingsSize() { + return settings::kv_store.size() - settings::kv_store.available(); } // -------------------------------------------------------------------------- @@ -119,44 +130,6 @@ unsigned char convert(const String& value) { // ----------------------------------------------------------------------------- -size_t settingsKeyCount() { - unsigned count = 0; - unsigned pos = SPI_FLASH_SEC_SIZE - 1; - while (size_t len = EEPROMr.read(pos)) { - if (0xFF == len) break; - pos = pos - len - 2; - len = EEPROMr.read(pos); - pos = pos - len - 2; - count ++; - } - return count; -} - -String settingsKeyName(unsigned int index) { - - String s; - - unsigned count = 0; - unsigned pos = SPI_FLASH_SEC_SIZE - 1; - while (size_t len = EEPROMr.read(pos)) { - if (0xFF == len) break; - pos = pos - len - 2; - if (count == index) { - s.reserve(len); - for (unsigned char i = 0 ; i < len; i++) { - s += (char) EEPROMr.read(pos + i + 1); - } - break; - } - count++; - len = EEPROMr.read(pos); - pos = pos - len - 2; - } - - return s; - -} - /* struct SettingsKeys { @@ -216,33 +189,12 @@ struct SettingsKeys { }; */ +// Note: we prefer things sorted via this function, not kv_store.keys() directly std::vector settingsKeys() { - - // Get sorted list of keys - std::vector keys; - - //unsigned int size = settingsKeyCount(); - auto size = settingsKeyCount(); - for (unsigned int i=0; i 0) { - keys.insert(keys.begin() + j, key); - inserted = true; - break; - } - - } - - // If we could not insert it, just push it at the end - if (!inserted) keys.push_back(key); - - } + auto keys = settings::kv_store.keys(); + std::sort(keys.begin(), keys.end(), [](const String& rhs, const String& lhs) -> bool { + return lhs.compareTo(rhs) > 0; + }); return keys; } @@ -305,24 +257,13 @@ void moveSettings(const String& from, const String& to) { } } -#if 0 -template Rfunc = settings::internal::convert> -R getSetting(const settings_key_t& key, R defaultValue) { - String value; - if (!Embedis::get(key.toString(), value)) { - return defaultValue; - } - return Rfunc(value); -} -#endif - template<> String getSetting(const settings_key_t& key, String defaultValue) { - String value; - if (!Embedis::get(key.toString(), value)) { - value = defaultValue; + auto result = settings::kv_store.get(key.toString()); + if (!result) { + return defaultValue; } - return value; + return result.value; } template @@ -367,16 +308,15 @@ String getSetting(const settings_key_t& key, const __FlashStringHelper* defaultV template<> bool setSetting(const settings_key_t& key, const String& value) { - return Embedis::set(key.toString(), value); + return settings::kv_store.set(key.toString(), value); } bool delSetting(const settings_key_t& key) { - return Embedis::del(key.toString()); + return settings::kv_store.del(key.toString()); } bool hasSetting(const settings_key_t& key) { - String value; - return Embedis::get(key.toString(), value); + return settings::kv_store.has(key.toString()); } void saveSettings() { @@ -386,9 +326,8 @@ void saveSettings() { } void resetSettings() { - for (unsigned int i = 0; i < EEPROM_SIZE; i++) { - EEPROMr.write(i, 0xFF); - } + auto* ptr = EEPROMr.getDataPtr(); + std::fill(ptr + EepromReservedSize, ptr + EepromSize, 0xFF); EEPROMr.commit(); } @@ -396,31 +335,21 @@ void resetSettings() { // API // ----------------------------------------------------------------------------- -size_t settingsMaxSize() { - size_t size = EEPROM_SIZE; - if (size > SPI_FLASH_SEC_SIZE) size = SPI_FLASH_SEC_SIZE; - size = (size + 3) & (~3); - return size; -} - bool settingsRestoreJson(JsonObject& data) { - // Check this is an ESPurna configuration file (must have "app":"ESPURNA") + // Note: we try to match what /config generates, expect {"app":"ESPURNA",...} const char* app = data["app"]; if (!app || strcmp(app, APP_NAME) != 0) { DEBUG_MSG_P(PSTR("[SETTING] Wrong or missing 'app' key\n")); return false; } - // Clear settings - bool is_backup = data["backup"]; - if (is_backup) { - for (unsigned int i = EEPROM_DATA_END; i < SPI_FLASH_SEC_SIZE; i++) { - EEPROMr.write(i, 0xFF); - } + // .../config will add this key, but it is optional + if (data["backup"].as()) { + resetSettings(); } - // Dump settings to memory buffer + // These three are just metadata, no need to actually store them for (auto element : data) { if (strcmp(element.key, "app") == 0) continue; if (strcmp(element.key, "version") == 0) continue; @@ -428,7 +357,6 @@ bool settingsRestoreJson(JsonObject& data) { setSetting(element.key, element.value.as()); } - // Persist to EEPROM saveSettings(); DEBUG_MSG_P(PSTR("[SETTINGS] Settings restored successfully\n")); @@ -483,17 +411,6 @@ void settingsProcessConfig(const settings_cfg_list_t& config, settings_filter_t void settingsSetup() { - Embedis::dictionary( F("EEPROM"), - SPI_FLASH_SEC_SIZE, - [](size_t pos) -> char { return EEPROMr.read(pos); }, - [](size_t pos, char value) { EEPROMr.write(pos, value); }, - #if SETTINGS_AUTOSAVE - []() { eepromCommit(); } - #else - []() {} - #endif - ); - terminalRegisterCommand(F("CONFIG"), [](const terminal::CommandContext& ctx) { // TODO: enough of a buffer? DynamicJsonBuffer jsonBuffer(1024); @@ -504,20 +421,17 @@ void settingsSetup() { }); terminalRegisterCommand(F("KEYS"), [](const terminal::CommandContext& ctx) { - // Get sorted list of keys auto keys = settingsKeys(); - // Write key-values ctx.output.println(F("Current settings:")); for (unsigned int i=0; i %s => \"%s\"\n", (keys[i]).c_str(), value.c_str()); } - unsigned long freeEEPROM [[gnu::unused]] = SPI_FLASH_SEC_SIZE - settingsSize(); + auto available [[gnu::unused]] = settings::kv_store.available(); ctx.output.printf("Number of keys: %u\n", keys.size()); - ctx.output.printf("Current EEPROM sector: %u\n", EEPROMr.current()); - ctx.output.printf("Free EEPROM: %lu bytes (%lu%%)\n", freeEEPROM, 100 * freeEEPROM / SPI_FLASH_SEC_SIZE); + ctx.output.printf("Available: %u bytes (%u%%)\n", available, (100 * available) / settings::kv_store.size()); terminalOK(ctx); }); @@ -530,7 +444,7 @@ void settingsSetup() { int result = 0; for (auto it = (ctx.argv.begin() + 1); it != ctx.argv.end(); ++it) { - result += Embedis::del(*it); + result += settings::kv_store.del(*it); } if (result) { @@ -546,7 +460,7 @@ void settingsSetup() { return; } - if (Embedis::set(ctx.argv[1], ctx.argv[2])) { + if (settings::kv_store.set(ctx.argv[1], ctx.argv[2])) { terminalOK(ctx); return; } @@ -562,8 +476,8 @@ void settingsSetup() { for (auto it = (ctx.argv.begin() + 1); it != ctx.argv.end(); ++it) { const String& key = *it; - String value; - if (!Embedis::get(key, value)) { + auto result = settings::kv_store.get(key); + if (!result) { const auto maybeDefault = settingsQueryDefaults(key); if (maybeDefault.length()) { ctx.output.printf("> %s => %s (default)\n", key.c_str(), maybeDefault.c_str()); @@ -573,7 +487,7 @@ void settingsSetup() { continue; } - ctx.output.printf("> %s => \"%s\"\n", key.c_str(), value.c_str()); + ctx.output.printf("> %s => \"%s\"\n", key.c_str(), result.value.c_str()); } terminalOK(ctx); diff --git a/code/espurna/settings.h b/code/espurna/settings.h index 7209da6c..60adff0f 100644 --- a/code/espurna/settings.h +++ b/code/espurna/settings.h @@ -15,14 +15,43 @@ Copyright (C) 2016-2019 by Xose Pérez #include #include -#include #include "broker.h" +#include "storage_eeprom.h" +#include "settings_embedis.h" BrokerDeclare(ConfigBroker, void(const String& key, const String& value)); // -------------------------------------------------------------------------- +namespace settings { + +struct EepromStorage { + + uint8_t read(size_t pos) { + return EEPROMr.read(pos); + } + + void write(size_t pos, uint8_t value) { + EEPROMr.write(pos, value); + } + + void commit() { +#if SETTINGS_AUTOSAVE + eepromCommit(); +#endif + } + +}; + +using kvs_type = embedis::KeyValueStore; + +extern kvs_type kv_store; + +} // namespace settings + +// -------------------------------------------------------------------------- + class settings_key_t { public: @@ -154,11 +183,11 @@ R getSetting(const settings_key_t& key, R defaultValue) __attribute__((noinline) template Rfunc = settings::internal::convert> R getSetting(const settings_key_t& key, R defaultValue) { - String value; - if (!Embedis::get(key.toString(), value)) { + auto result = settings::kv_store.get(key.toString()); + if (!result) { return defaultValue; } - return Rfunc(value); + return Rfunc(result.value); } template<> @@ -170,7 +199,7 @@ String getSetting(const settings_key_t& key, const __FlashStringHelper* defaultV template bool setSetting(const settings_key_t& key, const T& value) { - return Embedis::set(key.toString(), String(value)); + return settings::kv_store.set(key.toString(), String(value)); } template<> @@ -187,12 +216,11 @@ bool settingsRestoreJson(char* json_string, size_t json_buffer_size = 1024); bool settingsRestoreJson(JsonObject& data); size_t settingsKeyCount(); -String settingsKeyName(unsigned int index); std::vector settingsKeys(); void settingsProcessConfig(const settings_cfg_list_t& config, settings_filter_t filter = nullptr); -unsigned long settingsSize(); +size_t settingsSize(); void migrate(); void settingsSetup(); diff --git a/code/espurna/settings_embedis.h b/code/espurna/settings_embedis.h new file mode 100644 index 00000000..fe047d91 --- /dev/null +++ b/code/espurna/settings_embedis.h @@ -0,0 +1,674 @@ +/* + +Part of the SETTINGS MODULE + +Copyright (C) 2020 by Maxim Prokhorov + +Reimplementation of the Embedis storage format: +- https://github.com/thingSoC/embedis + +*/ + +#pragma once + +#include + +#include +#include +#include + +#include "libs/TypeChecks.h" + +namespace settings { +namespace embedis { + +// 'optional' type for byte range +struct ValueResult { + operator bool() { + return result; + } + bool result { false }; + String value; +}; + +// We can't save empty keys but can save empty values as 0x00 0x00 0x00 0x00 +// total sum is: +// - 2 bytes gap at the end (will be re-used by the next value length byte) +// - 4 bytes to store length of 2 values (stored as big-endian) +// - N bytes of values themselves +inline size_t estimate(const String& key, const String& value) { + if (!key.length()) { + return 0; + } + + const auto key_len = key.length(); + const auto value_len = value.length(); + + return (4 + key_len + ((value_len > 0) ? value_len : 2)); +} + +// Note: KeyValueStore is templated to avoid having to provide RawStorageBase via virtual inheritance. + +template +class KeyValueStore { + + // ----------------------------------------------------------------------------------- + // Notice: we can only use sfinae checks with the current compiler version + + // TODO: provide actual benchmark comparison with 'lambda'-list-as-vtable (old Embedis style) + // and vtable approach (write(), read() and commit() as pure virtual) + // TODO: consider overrides for bulk operations like move (see ::del method) + + template + using storage_can_write_t = decltype(std::declval().write( + std::declval(), std::declval())); + template + using storage_can_write = is_detected; + + template + using storage_can_read_t = decltype(std::declval().read(std::declval())); + template + using storage_can_read = is_detected; + + template + using storage_can_commit_t = decltype(std::declval().commit()); + template + using storage_can_commit = is_detected; + + static_assert( + (storage_can_commit{} && + storage_can_read{} && + storage_can_write{}), + "Storage class must implement read(index), write(index, byte) and commit()" + ); + + // ----------------------------------------------------------------------------------- + + protected: + + // Tracking state of the parser inside of _raw_read() + enum class State { + Begin, + End, + LenByte1, + LenByte2, + EmptyValue, + Value + }; + + // Pointer to the region of data that we are using + // + // XXX: It does not matter right now, but we **will** overflow position when using sizes >= (2^16) - 1 + // Note: Implementation is also in the header b/c c++ won't allow us + // to have a plain member (not a ptr or ref) of unknown size. + // Note: There was a considiration to implement this as 'stashing iterator' to be compatible with stl algorithms. + // In such implementation, we would store intermediate index and allow the user to receive a `value_proxy`, + // temporary returned by `value_proxy& operator*()' that is bound to Cursor instance. + // This **will** cause problems with 'reverse_iterator' or anything like it, as it expects reference to + // outlive the iterator object (specifically, result of `return *--tmp`, where `tmp` is created inside of a function block) + struct Cursor { + + Cursor(RawStorageBase& storage, uint16_t position_, uint16_t begin_, uint16_t end_) : + position(position_), + begin(begin_), + end(end_), + _storage(storage) + {} + + Cursor(RawStorageBase& storage, uint16_t begin_, uint16_t end_) : + Cursor(storage, 0, begin_, end_) + {} + + explicit Cursor(RawStorageBase& storage) : + Cursor(storage, 0, 0, 0) + {} + + static Cursor merge(RawStorageBase& storage, const Cursor& key, const Cursor& value) { + return Cursor(storage, key.begin, value.end); + } + + static Cursor fromEnd(RawStorageBase& storage, uint16_t begin, uint16_t end) { + return Cursor(storage, end - begin, begin, end); + } + + Cursor() = delete; + + void reset(uint16_t begin_, uint16_t end_) { + position = 0; + begin = begin_; + end = end_; + } + + uint8_t read() { + return _storage.read(begin + position); + } + + void write(uint8_t value) { + _storage.write(begin + position, value); + } + + void resetBeginning() { + position = 0; + } + + void resetEnd() { + position = end - begin; + } + + size_t size() { + return (end - begin); + } + + bool inRange(uint16_t position_) { + return (position_ < (end - begin)); + } + + operator bool() { + return inRange(position); + } + + uint8_t operator[](size_t position_) const { + return _storage.read(begin + position_); + } + + bool operator ==(const Cursor& other) { + return (begin == other.begin) && (end == other.end); + } + + bool operator !=(const Cursor& other) { + return !(*this == other); + } + + Cursor& operator++() { + ++position; + return *this; + } + + Cursor operator++(int) { + Cursor other(*this); + ++*this; + return other; + } + + Cursor& operator--() { + --position; + return *this; + } + + Cursor operator--(int) { + Cursor other(*this); + --*this; + return other; + } + + uint16_t position; + uint16_t begin; + uint16_t end; + + private: + + RawStorageBase& _storage; + + }; + + public: + + // Store value location in a more reasonable forward-iterator-style manner + // Allows us to skip string creation when just searching for specific values + // XXX: be cautious that cursor positions **will** break when underlying storage changes + struct ReadResult { + + friend class KeyValueStore; + + ReadResult(const Cursor& cursor_) : + length(0), + cursor(cursor_), + result(false) + {} + + ReadResult(RawStorageBase& storage) : + length(0), + cursor(storage), + result(false) + {} + + operator bool() { + return result; + } + + String read() { + String out; + out.reserve(length); + if (!length) { + return out; + } + + decltype(length) index = 0; + cursor.resetBeginning(); + while (index < length) { + out += static_cast(cursor.read()); + ++cursor; + ++index; + } + + return out; + } + + uint16_t length; + + private: + + Cursor cursor; + bool result; + + }; + + // Internal storage consists of sequences of + struct KeyValueResult { + operator bool() { + return (key) && (value) && (key.length > 0); + } + + bool operator !() { + return !(static_cast(*this)); + } + + template + KeyValueResult(T&& key_, T&& value_) : + key(std::forward(key_)), + value(std::forward(value_)) + {} + + KeyValueResult(RawStorageBase& storage) : + key(storage), + value(storage) + {} + + ReadResult key; + ReadResult value; + }; + + // one and only possible constructor, simply move the class object into the + // member variable to avoid forcing the user of the API to keep 2 objects alive. + KeyValueStore(RawStorageBase&& storage, uint16_t begin, uint16_t end) : + _storage(std::move(storage)), + _cursor(_storage, begin, end), + _state(State::Begin) + {} + + // Try to find the matching key. Datastructure that we use does not specify + // any value 'type' inside of it. We expect 'key' to be the first non-empty string, + // 'value' can be empty. + ValueResult get(const String& key) { + return _get(key, true); + } + + bool has(const String& key) { + return static_cast(_get(key, false)); + } + + // We going be using this pattern all the time here, because we need 2 consecutive **valid** ranges + // TODO: expose _read_kv() and _cursor_reset_end() so we can have 'break' here? + // perhaps as a wrapper object, allow something like next() and seekBegin() + template + void foreach(CallbackType callback) { + _cursor_reset_end(); + do { + auto kv = _read_kv(); + if (!kv) { + break; + } + callback(std::move(kv)); + } while (_state != State::End); + } + + // read every key into a vector + std::vector keys() { + std::vector out; + out.reserve(count()); + + foreach([&](KeyValueResult&& kv) { + out.push_back(kv.key.read()); + }); + + return out; + } + + // set or update key with value contents. ensure 'key' isn't empty, 'value' can be empty + bool set(const String& key, const String& value) { + + // ref. 'estimate()' implementation in regards to the storage calculation + auto need = estimate(key, value); + if (!need) { + return false; + } + + auto key_len = key.length(); + auto value_len = value.length(); + + Cursor to_erase(_storage); + bool need_erase = false; + + auto start_pos = _cursor_reset_end(); + + do { + auto kv = _read_kv(); + if (!kv) { + break; + } + + start_pos = kv.value.cursor.begin; + + // in the very special case we can match the existing key + if ((kv.key.length == key_len) && (kv.key.read() == key)) { + if (kv.value.length == value.length()) { + if (kv.value.read() == value) { + return true; + } + start_pos = kv.key.cursor.end; + break; + } + // but we may need to write over it, when contents are different + to_erase.reset(kv.value.cursor.begin, kv.key.cursor.end); + need_erase = true; + } + + } while (_state != State::End); + + if (need_erase) { + _raw_erase(start_pos, to_erase); + start_pos += to_erase.size(); + } + + // we should only insert when possition is still within possible size + if (start_pos && (start_pos >= need)) { + auto writer = Cursor::fromEnd(_storage, start_pos - need, start_pos); + + // put the length of the value as 2 bytes and then write the data + (--writer).write(key_len & 0xff); + (--writer).write((key_len >> 8) & 0xff); + while (key_len--) { + (--writer).write(key[key_len]); + } + + (--writer).write(value_len & 0xff); + (--writer).write((value_len >> 8) & 0xff); + + if (value_len) { + while (value_len--) { + (--writer).write(value[value_len]); + } + } else { + (--writer).write(0); + (--writer).write(0); + } + + // we also need to pad the space *after* the value + // but, only when we still have some space left + if ((start_pos - need) >= 2) { + _cursor_set_position(writer.begin - _cursor.begin); + auto next_kv = _read_kv(); + if (!next_kv) { + auto padding = Cursor::fromEnd(_storage, writer.begin - 2, writer.begin); + (--padding).write(0); + (--padding).write(0); + } + } + + _storage.commit(); + + return true; + } + + return false; + } + + // remove key from the storage. will check that 'key' argument isn't empty + bool del(const String& key) { + size_t key_len = key.length(); + if (!key_len) { + return false; + } + + // Removes key from the storage by overwriting the key with left-most data + size_t start_pos = _cursor_reset_end() - 1; + auto to_erase = Cursor::fromEnd(_storage, _cursor.begin, _cursor.end); + + // we should only compare strings of equal length. + // when matching, record { value ... key } range + 4 bytes for length + // continue searching for the leftmost boundary + foreach([&](KeyValueResult&& kv) { + start_pos = kv.value.cursor.begin; + if (!to_erase && (kv.key.length == key_len) && (kv.key.read() == key)) { + to_erase.reset(kv.value.cursor.begin, kv.key.cursor.end); + } + }); + + if (!to_erase) { + return false; + } + + _raw_erase(start_pos, to_erase); + + return true; + } + + // Simply count key-value pairs that we could parse + size_t count() { + size_t result = 0; + foreach([&result](KeyValueResult&&) { + ++result; + }); + + return result; + } + + // Do exactly the same thing as 'keys' does, but return the amount + // of bytes to the left of the last kv + size_t available() { + size_t result = _cursor.size(); + foreach([&result](KeyValueResult&& kv) { + result -= kv.key.cursor.size(); + result -= kv.value.cursor.size(); + }); + + return result; + } + + // How much bytes can be used is directly read from the cursor, based on begin and end values + size_t size() { + return _cursor.size(); + } + + protected: + + // Try to find the matching key. Datastructure that we use does not specify + // any value 'type' inside of it. We expect 'key' to be the first non-empty string, + // 'value' can be empty. + // To implement has(), allow to skip reading the value + ValueResult _get(const String& key, bool read_value) { + ValueResult out; + auto len = key.length(); + + _cursor_reset_end(); + + do { + auto kv = _read_kv(); + if (!kv) { + break; + } + + // no point in comparing keys when length does not match + // (and we also don't want to allocate the string) + if (kv.key.length != len) { + continue; + } + + auto key_result = kv.key.read(); + if (key_result == key) { + if (read_value) { + out.value = kv.value.read(); + } + out.result = true; + break; + } + } while (_state != State::End); + + return out; + } + + // Place cursor at the `end` and resets the parser to expect length byte + uint16_t _cursor_reset_end() { + _cursor.resetEnd(); + _state = State::Begin; + return _cursor.end; + } + + uint16_t _cursor_set_position(uint16_t position) { + _state = State::Begin; + _cursor.position = position; + return _cursor.position; + } + + // implementation quirk is that `Cursor::operator=` won't work because of the `RawStorageBase&` member + // right now, just construct in place and assume that compiler will inline things + // on one hand, we can implement it. but, we can't specifically + KeyValueResult _read_kv() { + auto key = _raw_read(); + if (!key || !key.length) { + return {_storage}; + } + + auto value = _raw_read(); + + return {key, value}; + }; + + void _raw_erase(size_t start_pos, Cursor& to_erase) { + // we either end up to the left or to the right of the boundary + if (start_pos < to_erase.begin) { + auto from = Cursor::fromEnd(_storage, start_pos, to_erase.begin); + auto to = Cursor::fromEnd(_storage, start_pos + to_erase.size(), to_erase.end); + + while (--from && --to) { + to.write(from.read()); + from.write(0xff); + }; + } else { + // just null the length bytes, since we at the last key + to_erase.resetEnd(); + (--to_erase).write(0); + (--to_erase).write(0); + } + + _storage.commit(); + } + + // Returns Cursor to the region that holds the data + // Result object does not hold any data, we need to explicitly request read() + // + // Cursor object is always expected to point to something, e.g. minimum: + // 0x01 0x00 0x01 + // data len2 len1 + // Position will be 0, end will be 4. Total length is 3, data length is 1 + // + // Note the distinction between real length and data length. For example, + // special-case for empty 'value' (as 'key' can never be empty and will be rejected): + // 0x00 0x00 0x00 0x00 + // data data len2 len1 + // Position will be 0, end will be 5. Total length is 4, data length is 0 + ReadResult _raw_read() { + uint16_t len = 0; + ReadResult out(_storage); + + do { + // storage is written right-to-left, cursor is always decreasing + switch (_state) { + case State::Begin: + if (_cursor.position > 2) { + --_cursor; + _state = State::LenByte1; + } else { + _state = State::End; + } + continue; + // len is 16 bit uint (bigendian) + // special case is 0, which is valid and should be returned when encountered + // another special case is 0xffff, meaning we just hit an empty space + case State::LenByte1: + len = _cursor.read(); + --_cursor; + _state = State::LenByte2; + break; + case State::LenByte2: + { + uint8_t len2 = _cursor.read(); + --_cursor; + if ((0 == len) && (0 == len2)) { + len = 2; + _state = State::EmptyValue; + } else if ((0xff == len) && (0xff == len2)) { + _state = State::End; + } else { + len |= len2 << 8; + _state = State::Value; + } + break; + } + case State::EmptyValue: + case State::Value: { + uint16_t left = len; + + // ensure we don't go out-of-bounds + switch (_state) { + case State::Value: + while (_cursor && --left) { + --_cursor; + } + break; + // ...and only read 0's + case State::EmptyValue: + while (_cursor && (_cursor.read() == 0) && --left) { + --_cursor; + } + break; + default: + break; + } + + if (left) { + _state = State::End; + break; + } + + // set the resulting cursor as [pos:len+2) + out.result = true; + out.length = (_state == State::EmptyValue) ? 0 : len; + out.cursor.reset( + _cursor.begin + _cursor.position, + _cursor.begin + _cursor.position + len + 2 + ); + + _state = State::Begin; + + goto return_result; + } + case State::End: + default: + break; + } + + } while (_state != State::End); + + return_result: + + return out; + } + + RawStorageBase _storage; + Cursor _cursor; + State _state { State::Begin }; +}; + +} // namespace embedis +} // namespace settings diff --git a/code/espurna/storage_eeprom.cpp b/code/espurna/storage_eeprom.cpp index 00d7b0e5..003b5eb7 100644 --- a/code/espurna/storage_eeprom.cpp +++ b/code/espurna/storage_eeprom.cpp @@ -4,14 +4,19 @@ EEPROM MODULE */ -#include "storage_eeprom.h" -#include "settings.h" +// XXX: including storage_eeprom.h here directly causes dependency issue with settings +#include "espurna.h" EEPROM_Rotate EEPROMr; bool _eeprom_commit = false; uint32_t _eeprom_commit_count = 0; bool _eeprom_last_commit_result = false; +bool _eeprom_ready = false; + +bool eepromReady() { + return _eeprom_ready; +} void eepromRotate(bool value) { // Enable/disable EEPROM rotation only if we are using more sectors than the @@ -66,11 +71,10 @@ void eepromBackup(uint32_t index){ void _eepromInitCommands() { terminalRegisterCommand(F("EEPROM"), [](const terminal::CommandContext&) { - infoMemory("EEPROM", SPI_FLASH_SEC_SIZE, SPI_FLASH_SEC_SIZE - settingsSize()); eepromSectorsDebug(); if (_eeprom_commit_count > 0) { DEBUG_MSG_P(PSTR("[MAIN] Commits done: %lu\n"), _eeprom_commit_count); - DEBUG_MSG_P(PSTR("[MAIN] Last result: %s\n"), _eeprom_last_commit_result ? "OK" : "ERROR"); + DEBUG_MSG_P(PSTR("[MAIN] Last result: %s\n"), _eeprom_last_commit_result ? "OK" : "ERROR"); } terminalOK(); }); @@ -135,8 +139,8 @@ void eepromSetup() { } #endif - EEPROMr.offset(EEPROM_ROTATE_DATA); - EEPROMr.begin(EEPROM_SIZE); + EEPROMr.offset(EepromRotateOffset); + EEPROMr.begin(EepromSize); #if TERMINAL_SUPPORT _eepromInitCommands(); @@ -144,4 +148,6 @@ void eepromSetup() { espurnaRegisterLoop(eepromLoop); + _eeprom_ready = true; + } diff --git a/code/espurna/storage_eeprom.h b/code/espurna/storage_eeprom.h index 75cc0f3c..106a7d59 100644 --- a/code/espurna/storage_eeprom.h +++ b/code/espurna/storage_eeprom.h @@ -13,8 +13,21 @@ EEPROM MODULE #include "espurna.h" +// Note: backwards compatibility requires us to reserve these much bytes at the start. +// main reason right now is EEPROM_Rotate crc bytes +constexpr size_t EepromReservedSize = 14; + +// "The library uses 3 bytes to track last valid sector, so there must be at least 3" +// Reserve addresses 11, 12 and 13 for EEPROM_Rotate +constexpr int EepromRotateOffset = 11; +constexpr size_t EepromRotateReservedSize = 3; + +constexpr size_t EepromSize = SPI_FLASH_SEC_SIZE; + extern EEPROM_Rotate EEPROMr; +bool eepromReady(); + void eepromSectorsDebug(); void eepromRotate(bool value); uint32_t eepromCurrent(); diff --git a/code/espurna/system.cpp b/code/espurna/system.cpp index b5a5a9d1..1ae72700 100644 --- a/code/espurna/system.cpp +++ b/code/espurna/system.cpp @@ -6,7 +6,7 @@ Copyright (C) 2019 by Xose Pérez */ -#include "system.h" +#include "espurna.h" #include #include diff --git a/code/espurna/telnet.cpp b/code/espurna/telnet.cpp index f4e5a44f..bc3bbe13 100644 --- a/code/espurna/telnet.cpp +++ b/code/espurna/telnet.cpp @@ -306,9 +306,8 @@ void _telnetNotifyConnected(unsigned char i) { // If there is no terminal support automatically dump info and crash data #if DEBUG_SUPPORT #if not TERMINAL_SUPPORT - info(); wifiDebug(); - crashDump(); + crashDump(terminalDefaultStream()); crashClear(); #endif #endif diff --git a/code/espurna/terminal.cpp b/code/espurna/terminal.cpp index bfdecfb4..6ee7a5d3 100644 --- a/code/espurna/terminal.cpp +++ b/code/espurna/terminal.cpp @@ -7,7 +7,7 @@ Copyright (C) 2020 by Maxim Prokhorov */ -#include "terminal.h" +#include "espurna.h" #if TERMINAL_SUPPORT diff --git a/code/espurna/utils.cpp b/code/espurna/utils.cpp index d594cf76..3c9abc49 100644 --- a/code/espurna/utils.cpp +++ b/code/espurna/utils.cpp @@ -6,9 +6,7 @@ Copyright (C) 2017-2019 by Xose Pérez */ -#include - -#include "utils.h" +#include "espurna.h" #include "board.h" #include "influxdb.h" @@ -20,6 +18,7 @@ Copyright (C) 2017-2019 by Xose Pérez #include "libs/TypeChecks.h" +#include //-------------------------------------------------------------------------------- // Reset reasons diff --git a/code/espurna/web.cpp b/code/espurna/web.cpp index 22411bfc..0b0ecfde 100644 --- a/code/espurna/web.cpp +++ b/code/espurna/web.cpp @@ -266,9 +266,8 @@ void _onGetConfig(AsyncWebServerRequest *request) { #endif // Write the keys line by line (not sorted) - unsigned long count = settingsKeyCount(); - for (unsigned int i=0; iprintf(",\n\"%s\": \"%s\"", key.c_str(), value.c_str()); } diff --git a/code/platformio.ini b/code/platformio.ini index ae0adf7e..3ed747d1 100644 --- a/code/platformio.ini +++ b/code/platformio.ini @@ -141,7 +141,6 @@ lib_deps = https://github.com/marvinroger/async-mqtt-client#v0.8.1 Brzo I2C https://github.com/xoseperez/eeprom_rotate#0.9.2 - Embedis https://github.com/plerup/espsoftwareserial#3.4.1 https://github.com/me-no-dev/ESPAsyncTCP#7e9ed22 https://github.com/me-no-dev/ESPAsyncWebServer#b0c6144 diff --git a/code/test/platformio.ini b/code/test/platformio.ini index 98ff688a..baba4e2c 100644 --- a/code/test/platformio.ini +++ b/code/test/platformio.ini @@ -1,7 +1,25 @@ +# This file is used compile and run tests located in the `unit` directory. +# For more info, see: +# https://docs.platformio.org/en/latest/plus/unit-testing.html +# https://github.com/ThrowTheSwitch/Unity +# https://github.com/ThrowTheSwitch/Unity/blob/master/docs/UnityAssertionsReference.md + [platformio] test_dir = unit src_dir = ../espurna +# TODO: add `-t coverage` via python scripting? +# TODO: do we need `-O0`? + +# To prepare coverage data for lcov, add ${coverage.build_flags} to env:test build flags +# To actually generate coverage report: +# $ `pio test` / run the test `program` manually +# $ lcov --include (readlink -f ../espurna)'/*' --capture --directory .pio/build/test/ --output-file test.info +# $ genhtml --ignore-errors source test.info --output-directory out + +[coverage] +build_flags = -lgcov -fprofile-arcs -ftest-coverage + [env:test] platform = native lib_compat_mode = off @@ -16,5 +34,6 @@ build_flags = -DMANUFACTURER="PLATFORMIO" -DDEVICE="TEST" -std=gnu++11 + -g -Os -I../espurna/ diff --git a/code/test/unit/settings/main.cpp b/code/test/unit/settings/main.cpp new file mode 100644 index 00000000..fa66e2f1 --- /dev/null +++ b/code/test/unit/settings/main.cpp @@ -0,0 +1,487 @@ +#include +#include + +#pragma GCC diagnostic warning "-Wall" +#pragma GCC diagnostic warning "-Wextra" +#pragma GCC diagnostic warning "-Wstrict-aliasing" +#pragma GCC diagnostic warning "-Wpointer-arith" +#pragma GCC diagnostic warning "-Wstrict-overflow=5" + +#include + +#include +#include +#include + +namespace settings { +namespace embedis { + +template +struct StaticArrayStorage { + + explicit StaticArrayStorage(T& blob) : + _blob(blob), + _size(blob.size()) + {} + + uint8_t read(size_t index) { + TEST_ASSERT_LESS_THAN(_size, index); + return _blob[index]; + } + + void write(size_t index, uint8_t value) { + TEST_ASSERT_LESS_THAN(_size, index); + _blob[index] = value; + } + + void commit() { + } + + T& _blob; + const size_t _size; + +}; + +} // namespace embedis +} // namespace settings + +template +struct StorageHandler { + using array_type = std::array; + using storage_type = settings::embedis::StaticArrayStorage; + using kvs_type = settings::embedis::KeyValueStore; + + StorageHandler() : + kvs(std::move(storage_type{blob}), 0, Size) + { + blob.fill(0xff); + } + + array_type blob; + kvs_type kvs; +}; + +// generate stuff depending on the mode +// - Indexed: key1:val1, key2:val2, ... +// - IncreasingLength: k:v, kk:vv, ... +struct TestSequentialKvGenerator { + + using kv = std::pair; + enum class Mode { + Indexed, + IncreasingLength + }; + + TestSequentialKvGenerator() = default; + explicit TestSequentialKvGenerator(Mode mode) : + _mode(mode) + {} + + const kv& next() { + auto index = _index++; + + _current.first = ""; + _current.second = ""; + + switch (_mode) { + case Mode::Indexed: + _current.first = String("key") + String(index); + _current.second = String("val") + String(index); + break; + case Mode::IncreasingLength: { + size_t sizes = _index; + _current.first.reserve(sizes); + _current.second.reserve(sizes); + + do { + _current.first += "k"; + _current.second += "v"; + } while (--sizes); + break; + } + } + TEST_ASSERT(_last.first != _current.first); + TEST_ASSERT(_last.second != _current.second); + + return (_last = _current); + } + + std::vector make(size_t size) {; + std::vector res; + for (size_t index = 0; index < size; ++index) { + res.push_back(next()); + } + return res; + } + + kv _current; + kv _last; + + Mode _mode { Mode::Indexed }; + size_t _index { 0 }; + +}; + +// ---------------------------------------------------------------------------- + +using TestStorageHandler = StorageHandler<1024>; + +template +void check_kv(T& instance, const String& key, const String& value) { + auto result = instance.kvs.get(key); + TEST_ASSERT_MESSAGE(static_cast(result), key.c_str()); + TEST_ASSERT(result.value.length()); + TEST_ASSERT_EQUAL_STRING(value.c_str(), result.value.c_str()); +}; + +void test_sizes() { + + // empty storage is still manageble, it just does not work :) + { + StorageHandler<0> empty; + TEST_ASSERT_EQUAL(0, empty.kvs.count()); + TEST_ASSERT_FALSE(empty.kvs.set("cannot", "happen")); + TEST_ASSERT_FALSE(static_cast(empty.kvs.get("cannot"))); + } + + // some hard-coded estimates to notify us about internal changes + { + StorageHandler<16> instance; + TEST_ASSERT_EQUAL(0, instance.kvs.count()); + TEST_ASSERT_EQUAL(16, instance.kvs.available()); + TEST_ASSERT_EQUAL(0, settings::embedis::estimate("", "123456")); + TEST_ASSERT_EQUAL(16, settings::embedis::estimate("123456", "123456")); + TEST_ASSERT_EQUAL(10, settings::embedis::estimate("123", "123")); + TEST_ASSERT_EQUAL(9, settings::embedis::estimate("345", "")); + } + +} + +void test_longkey() { + + TestStorageHandler instance; + const auto estimate = instance.kvs.size() - 6; + + String key; + key.reserve(estimate); + for (size_t n = 0; n < estimate; ++n) { + key += 'a'; + } + + TEST_ASSERT(instance.kvs.set(key, "")); + auto result = instance.kvs.get(key); + TEST_ASSERT(static_cast(result)); + +} + +void test_perseverance() { + + // ensure we can handle setting the same key + using storage_type = StorageHandler<128>; + using blob_type = decltype(std::declval().blob); + + // xxx: implementation detail? + // can we avoid blob modification when value is the same as the existing one + { + storage_type instance; + blob_type original(instance.blob); + + TEST_ASSERT(instance.kvs.set("key", "value")); + TEST_ASSERT(instance.kvs.set("another", "keyvalue")); + TEST_ASSERT(original != instance.blob); + blob_type snapshot(instance.blob); + + TEST_ASSERT(instance.kvs.set("key", "value")); + TEST_ASSERT(snapshot == instance.blob); + } + + // xxx: pointless implementation detail? + // can we re-use existing 'value' storage and avoid data-shift + { + storage_type instance; + blob_type original(instance.blob); + + // insert in a specific order, change middle + TEST_ASSERT(instance.kvs.set("aaa", "bbb")); + TEST_ASSERT(instance.kvs.set("cccc", "dd")); + TEST_ASSERT(instance.kvs.set("ee", "fffff")); + TEST_ASSERT(instance.kvs.set("cccc", "ff")); + TEST_ASSERT(original != instance.blob); + blob_type before(instance.blob); + + // purge, insert again with updated values + TEST_ASSERT(instance.kvs.del("aaa")); + TEST_ASSERT(instance.kvs.del("cccc")); + TEST_ASSERT(instance.kvs.del("ee")); + + TEST_ASSERT(instance.kvs.set("aaa", "bbb")); + TEST_ASSERT(instance.kvs.set("cccc", "ff")); + TEST_ASSERT(instance.kvs.set("ee", "fffff")); + blob_type after(instance.blob); + + TEST_ASSERT(original != before); + TEST_ASSERT(original != after); + TEST_ASSERT(before == after); + } +} + +template +struct test_overflow_runner { + void operator ()() const { + StorageHandler instance; + + TEST_ASSERT(instance.kvs.set("a", "b")); + TEST_ASSERT(instance.kvs.set("c", "d")); + + TEST_ASSERT_EQUAL(2, instance.kvs.count()); + TEST_ASSERT_FALSE(instance.kvs.set("e", "f")); + + TEST_ASSERT(instance.kvs.del("a")); + + TEST_ASSERT_EQUAL(1, instance.kvs.count()); + TEST_ASSERT(instance.kvs.set("e", "f")); + + TEST_ASSERT_EQUAL(2, instance.kvs.count()); + + check_kv(instance, "e", "f"); + check_kv(instance, "c", "d"); + } +}; + +void test_overflow() { + // slightly more that available, but we cannot fit the key + test_overflow_runner<16>(); + + // no more space + test_overflow_runner<12>(); +} + +void test_small_gaps() { + + // ensure we can intemix empty and non-empty values + TestStorageHandler instance; + + TEST_ASSERT(instance.kvs.set("key", "value")); + TEST_ASSERT(instance.kvs.set("empty", "")); + TEST_ASSERT(instance.kvs.set("empty_again", "")); + TEST_ASSERT(instance.kvs.set("finally", "avalue")); + + auto check_empty = [&instance](const String& key) { + auto result = instance.kvs.get(key); + TEST_ASSERT(static_cast(result)); + TEST_ASSERT_FALSE(result.value.length()); + }; + + check_empty("empty_again"); + check_empty("empty"); + check_empty("empty_again"); + check_empty("empty"); + + auto check_value = [&instance](const String& key, const String& value) { + auto result = instance.kvs.get(key); + TEST_ASSERT(static_cast(result)); + TEST_ASSERT(result.value.length()); + TEST_ASSERT_EQUAL_STRING(value.c_str(), result.value.c_str()); + }; + + check_value("finally", "avalue"); + check_value("key", "value"); + +} + +void test_remove_randomized() { + + // ensure we can remove keys in any order + // 8 seems like a good number to stop on, 9 will spend ~10seconds + // TODO: seems like a good start benchmarking read / write performance? + constexpr size_t KeysNumber = 8; + + TestSequentialKvGenerator generator(TestSequentialKvGenerator::Mode::IncreasingLength); + auto kvs = generator.make(KeysNumber); + + // generate indexes array to allow us to reference keys at random + TestStorageHandler instance; + std::array indexes; + std::iota(indexes.begin(), indexes.end(), 0); + + // - insert keys sequentially + // - remove keys based on the order provided by next_permutation() + size_t index = 0; + do { + TEST_ASSERT(0 == instance.kvs.count()); + for (auto& kv : kvs) { + TEST_ASSERT(instance.kvs.set(kv.first, kv.second)); + } + + for (auto index : indexes) { + auto key = kvs[index].first; + TEST_ASSERT(static_cast(instance.kvs.get(key))); + TEST_ASSERT(instance.kvs.del(key)); + TEST_ASSERT_FALSE(static_cast(instance.kvs.get(key))); + } + + index++; + } while (std::next_permutation(indexes.begin(), indexes.end())); + + String message("- keys: "); + message += KeysNumber; + message += ", permutations: "; + message += index; + TEST_MESSAGE(message.c_str()); + +} + +void test_basic() { + TestStorageHandler instance; + + constexpr size_t KeysNumber = 5; + + // ensure insert works + TestSequentialKvGenerator generator; + auto kvs = generator.make(KeysNumber); + + for (auto& kv : kvs) { + instance.kvs.set(kv.first, kv.second); + } + + // and we can retrieve keys back + for (auto& kv : kvs) { + auto result = instance.kvs.get(kv.first); + TEST_ASSERT(static_cast(result)); + TEST_ASSERT_EQUAL_STRING(kv.second.c_str(), result.value.c_str()); + } + +} + +void test_storage() { + + constexpr size_t Size = 32; + StorageHandler instance; + + // empty keys are invalid + TEST_ASSERT_FALSE(instance.kvs.set("", "value1")); + TEST_ASSERT_FALSE(instance.kvs.del("")); + + // ...and both keys are not yet set + TEST_ASSERT_FALSE(instance.kvs.del("key1")); + TEST_ASSERT_FALSE(instance.kvs.del("key2")); + + // some different ways to set keys + TEST_ASSERT(instance.kvs.set("key1", "value0")); + TEST_ASSERT_EQUAL(1, instance.kvs.count()); + TEST_ASSERT(instance.kvs.set("key1", "value1")); + TEST_ASSERT_EQUAL(1, instance.kvs.count()); + + TEST_ASSERT(instance.kvs.set("key2", "value_old")); + TEST_ASSERT_EQUAL(2, instance.kvs.count()); + TEST_ASSERT(instance.kvs.set("key2", "value2")); + TEST_ASSERT_EQUAL(2, instance.kvs.count()); + + auto kvsize = settings::embedis::estimate("key1", "value1"); + TEST_ASSERT_EQUAL((Size - (2 * kvsize)), instance.kvs.available()); + + // checking keys one by one by using a separate kvs object, + // working on the same underlying data-store + using storage_type = decltype(instance)::storage_type; + using kvs_type = decltype(instance)::kvs_type; + + // - ensure we can operate with storage offsets + // - test for internal length optimization that will overwrite the key in-place + // - make sure we did not break the storage above + // storage_type accepts reference to the blob, so we can seamlessly use the same + // underlying data storage and share it between kvs instances + { + kvs_type slice(storage_type(instance.blob), (Size - kvsize), Size); + TEST_ASSERT_EQUAL(1, slice.count()); + TEST_ASSERT_EQUAL(kvsize, slice.size()); + TEST_ASSERT_EQUAL(0, slice.available()); + auto result = slice.get("key1"); + TEST_ASSERT(static_cast(result)); + TEST_ASSERT_EQUAL_STRING("value1", result.value.c_str()); + } + + // ensure that right offset also works + { + kvs_type slice(storage_type(instance.blob), 0, (Size - kvsize)); + TEST_ASSERT_EQUAL(1, slice.count()); + TEST_ASSERT_EQUAL((Size - kvsize), slice.size()); + TEST_ASSERT_EQUAL((Size - kvsize - kvsize), slice.available()); + auto result = slice.get("key2"); + TEST_ASSERT(static_cast(result)); + TEST_ASSERT_EQUAL_STRING("value2", result.value.c_str()); + } + + // ensure offset does not introduce offset bugs + // for instance, test in-place key overwrite by moving left boundary 2 bytes to the right + { + const auto available = instance.kvs.available(); + const auto offset = 2; + + TEST_ASSERT_GREATER_OR_EQUAL(offset, available); + + kvs_type slice(storage_type(instance.blob), offset, Size); + TEST_ASSERT_EQUAL(2, slice.count()); + + auto key1 = slice.get("key1"); + TEST_ASSERT(static_cast(key1)); + + String updated(key1.value); + for (size_t index = 0; index < key1.value.length(); ++index) { + updated[index] = 'A'; + } + + TEST_ASSERT(slice.set("key1", updated)); + TEST_ASSERT(slice.set("key2", updated)); + + TEST_ASSERT_EQUAL(2, slice.count()); + + auto check_key1 = slice.get("key1"); + TEST_ASSERT(static_cast(check_key1)); + TEST_ASSERT_EQUAL_STRING(updated.c_str(), check_key1.value.c_str()); + + auto check_key2 = slice.get("key2"); + TEST_ASSERT(static_cast(check_key2)); + TEST_ASSERT_EQUAL_STRING(updated.c_str(), check_key2.value.c_str()); + + TEST_ASSERT_EQUAL(available - offset, slice.available()); + } + +} + +void test_keys_iterator() { + + constexpr size_t Size = 32; + StorageHandler instance; + + TEST_ASSERT_EQUAL(Size, instance.kvs.available()); + TEST_ASSERT_EQUAL(Size, instance.kvs.size()); + TEST_ASSERT(instance.kvs.set("key", "value")); + TEST_ASSERT(instance.kvs.set("another", "thing")); + + // ensure we get the same order of keys when iterating via foreach + std::vector keys; + instance.kvs.foreach([&keys](decltype(instance)::kvs_type::KeyValueResult&& kv) { + keys.push_back(kv.key.read()); + }); + + TEST_ASSERT(instance.kvs.keys() == keys); + TEST_ASSERT_EQUAL(2, keys.size()); + TEST_ASSERT_EQUAL(2, instance.kvs.count()); + TEST_ASSERT_EQUAL_STRING("key", keys[0].c_str()); + TEST_ASSERT_EQUAL_STRING("another", keys[1].c_str()); + +} + +int main(int argc, char** argv) { + UNITY_BEGIN(); + RUN_TEST(test_storage); + RUN_TEST(test_keys_iterator); + RUN_TEST(test_basic); + RUN_TEST(test_remove_randomized); + RUN_TEST(test_small_gaps); + RUN_TEST(test_overflow); + RUN_TEST(test_perseverance); + RUN_TEST(test_longkey); + RUN_TEST(test_sizes); + return UNITY_END(); +}