From 0f5e40510c79d573f8ff5adde6c4ff13e631d02f Mon Sep 17 00:00:00 2001 From: BlackMark Date: Sun, 23 Aug 2026 05:29:01 +0200 Subject: [PATCH] perf: the console's constants move to flash, -378 B and -214 B of RAM 8382 -> 8004 B of flash, 725 -> 511 B of RAM on a part that has 2048. The command names were a std::to_array of string_view, and on a Harvard machine that is the worst of both: the characters land in .data and so does the table's own pointer-and-length pair for each of them, so the firmware carried 216 B of RAM for thirteen words that never change - and paid for them in flash too, since .data is copied out of an initialiser image at startup. They are one NUL-separated blob in flash now, walked with lpm. Separators rather than an offset table, because an offset table is the RAM this exists to give back; the names sit in it in match order, so the walk that finds a name is the same walk that compares it and measures it. Flash falls further than RAM does: the initialiser image and the two tables were 216 B of it, and the blob is 87. The header said the names "cannot" be in flash because they are matched at run time. Being matched at run time is not a reason to be in RAM on a machine with two address spaces - only being *written* is, and nothing writes these. Two smaller things came with it. `reset`'s exact-match rule was a bool on every entry to protect one; it is an index found by searching the list, so reordering the commands cannot move the protection onto a different one. And `version` was the last string_view left, holding its own characters and a pointer to them. The matching is now pinned rather than assumed: lookup() is constexpr and the battery asserts the load-bearing order the README documents - `s` is show and not statistics, `st` is statistics, no abbreviation of `reset` resolves, and `helpful` is not `help`. Red-checked by claiming `s` is statistics. Both modes byte-identical, ten tests green. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- src/terminal.hpp | 226 +++++++++++++++++++++++++++++--------------- test/CMakeLists.txt | 2 +- test/consteval.cpp | 36 +++++++ 4 files changed, 188 insertions(+), 78 deletions(-) diff --git a/README.md b/README.md index 25805ad..d143819 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ cmake --preset atmega328p-generated cmake --build --preset atmega328p-generated ``` -The firmware is **8382 B** of flash, byte-identical between the generated and +The firmware is **8004 B** of flash, byte-identical between the generated and reflect modes, and `ctest` holds it to that number. ## Atmel Studio diff --git a/src/terminal.hpp b/src/terminal.hpp index 7d17cb2..b0d80ad 100644 --- a/src/terminal.hpp +++ b/src/terminal.hpp @@ -18,6 +18,33 @@ // until Ctrl+C, and Ctrl+C abandons a half-typed line anywhere else. namespace app { +// Commands, in the order they are matched - which is the order the original +// firmware matched them in, and that order is load-bearing. An abbreviation +// resolves to the *first* entry it prefixes, so `s` is show (not statistics, +// not set) exactly as it always was, and anything appended to this list cannot +// steal an abbreviation that already meant something else. +// +// A function rather than a variable, so the list exists only while the flash +// blob below is being built and never as storage of its own. +consteval auto command_names() +{ + return std::to_array({ + "help", + "show", + "curve", + "monitor", + "bootloader", + "uptime", + "statistics", + "histogram", + "reset", + "set", + "auto", + "version", + "save", + }); +} + class terminal { static constexpr char ctrl_c = 0x03; static constexpr char backspace = 0x08; @@ -28,31 +55,48 @@ class terminal { static inline bool m_monitoring = false; static inline std::uint64_t m_last_monitor = 0; - // Commands, in the order they are matched - which is the order the original - // firmware matched them in, and that order is load-bearing. An abbreviation - // resolves to the *first* entry it prefixes, so `s` is show (not statistics, - // not set) exactly as it always was, and anything appended to this list - // cannot steal an abbreviation that already meant something else. - struct command { - std::string_view name; - bool exact; // reset only: an abbreviation must not be able to wipe data - }; + static constexpr std::uint8_t command_count = command_names().size(); - static constexpr auto commands = std::to_array({ - {"help", false}, - {"show", false}, - {"curve", false}, - {"monitor", false}, - {"bootloader", false}, - {"uptime", false}, - {"statistics", false}, - {"histogram", false}, - {"reset", true}, - {"set", false}, - {"auto", false}, - {"version", false}, - {"save", false}, - }); + static constexpr std::size_t name_bytes = [] { + std::size_t total = 0; + for (auto one : command_names()) { + total += one.size() + 1; + } + return total; + }(); + + // The names as one NUL-separated blob in flash. Matched at run time, which + // is not a reason to sit in RAM on a machine with two address spaces: a + // table of `string_view` puts the characters *and* its own pointers there, + // and on this part that was 216 B of 2 KB spent on data that never changes. + // Separators rather than an offset table, because an offset table is the + // RAM this exists to give back. + static constexpr auto names_data = [] { + std::array chars{}; + std::size_t at = 0; + for (auto one : command_names()) { + for (char c : one) { + chars[at++] = c; + } + chars[at++] = '\0'; + } + return chars; + }(); + + static constexpr avr::flash_table names{}; + + // `reset` must be typed in full - an abbreviation must not be able to wipe + // data. Found in the list rather than written as an index, so reordering the + // commands cannot move the protection onto a different one. + static constexpr std::uint8_t exact_command = [] { + const auto list = command_names(); + for (std::uint8_t i = 0; i < list.size(); ++i) { + if (list[i] == "reset") { + return i; + } + } + return static_cast(list.size()); + }(); // Column the descriptions' colons line up in, counted from the start of the // name. The longest name is `bootloader` at 10, so 12 leaves it a space and @@ -66,20 +110,8 @@ class terminal { // `name ....: ` - the dotted label the original used everywhere it printed a // list of things, which is what makes a column of values readable without - // counting spaces. One renderer for all three users; only the column differs. - static void label(std::string_view text, std::uint8_t column) - { - serial << text << ' '; - for (auto i = text.size() + 1; i < column; ++i) { - serial << '.'; - } - serial << ": "_P; - } - - // The same, for a label that is a literal rather than a command name: it - // stays in flash, and its width comes from the type, so the padding needs no - // hand-counted constant. The command names cannot use this - they are - // string_views because they are matched at run time. + // counting spaces. Its width comes from the type, so the padding needs no + // hand-counted constant. template static void label(Flash text, std::uint8_t column) { @@ -90,9 +122,21 @@ class terminal { serial << ": "_P; } - static void help_row(std::string_view name) + // The same shape for a command name, whose width is not a type but the walk + // to its separator - so it is counted as it is printed rather than measured + // first, which would read the blob twice. + static void help_row(std::uint8_t which) { - label(name, help_column); + auto at = name_start(which); + std::uint8_t width = 0; + for (char c; (c = names.at(at + width)) != '\0'; ++width) { + serial << c; + } + serial << ' '; + for (std::uint8_t i = width + 1; i < help_column; ++i) { + serial << '.'; + } + serial << ": "_P; } // Quarter- C as a signed decimal with a two-digit fraction. The sign is taken @@ -110,31 +154,31 @@ class terminal { static void help() { serial << "\r\nFanTemp "_P << version << " command overview\r\n"_P; - help_row(commands[0].name); + help_row(0); serial << "prints this help message\r\n"_P; - help_row(commands[1].name); + help_row(1); serial << "shows current temperature and fan speed\r\n"_P; - help_row(commands[2].name); + help_row(2); serial << "shows mapping from temperature to fan speed\r\n"_P; - help_row(commands[3].name); + help_row(3); serial << "loops the show command until Ctrl+C is pressed\r\n"_P; - help_row(commands[4].name); + help_row(4); serial << "enters the bootloader\r\n"_P; - help_row(commands[5].name); + help_row(5); serial << "shows system uptime\r\n"_P; - help_row(commands[6].name); + help_row(6); serial << "prints overall statistics like min and max temp\r\n"_P; - help_row(commands[7].name); + help_row(7); serial << "prints a histogram of the temperature\r\n"_P; - help_row(commands[8].name); + help_row(8); serial << "resets statistics to 0 in EEPROM and RAM (no abbreviation)\r\n"_P; - help_row(commands[9].name); + help_row(9); serial << "sets the fan speed to the provided value, 0-100\r\n"_P; - help_row(commands[10].name); + help_row(10); serial << "turns on automatic fan control\r\n"_P; - help_row(commands[11].name); + help_row(11); serial << "displays firmware version\r\n"_P; - help_row(commands[12].name); + help_row(12); serial << "writes the statistics to EEPROM now\r\n"_P; serial << "commands may be abbreviated: 'up' is uptime\r\n"_P; } @@ -259,25 +303,58 @@ class terminal { } } - // Abbreviations: the input matches a command when it is a non-empty prefix - // of it. `reset` is the exception and must be typed in full. - // - // starts_with, not substr: substr throws std::out_of_range, and one - // potentially-throwing call is enough to pull in std::terminate, which does - // not exist in a freestanding AVR build. The link fails rather than the - // firmware, so this is a build-time trap rather than a runtime one - but it - // is a trap, and the whole file avoids substr for that reason. - static bool matches(std::string_view input, const command &c) + // Where the given command's name begins in the blob. A walk over the + // separators, for the two callers that need to start at one name. + static constexpr std::uint8_t name_start(std::uint8_t which) { - if (input.empty()) { - return false; + std::uint8_t at = 0; + for (std::uint8_t i = 0; i < which; ++i) { + while (names.at(at) != '\0') { + ++at; + } + ++at; } - if (c.exact) { - return input == c.name; - } - return c.name.starts_with(input); + return at; } + public: + // Which command a word names, or `command_count` for none. The input + // matches when it is a non-empty prefix of a name; `reset` is the exception + // and must be typed in full. + // + // One pass over the blob answers both, because the names sit in it in match + // order: each name is compared as it is walked, and the walk to its + // separator is also what measures it. + // + // Public and `constexpr` so the battery can pin it: the match order is + // load-bearing, an abbreviation must never reach `reset`, and reading the + // names from flash must not have changed either. + static constexpr std::uint8_t lookup(std::string_view input) + { + if (input.empty()) { + return command_count; + } + std::uint8_t at = 0; + for (std::uint8_t i = 0; i < command_count; ++i) { + std::uint8_t length = 0; + bool prefix = true; + for (char c; (c = names.at(at + length)) != '\0'; ++length) { + if (length < input.size() && input[length] != c) { + prefix = false; + } + } + const bool whole = input.size() == length; + if (prefix && input.size() <= length && (i != exact_command || whole)) { + return i; + } + at += length + 1; + } + return command_count; + } + + static constexpr std::uint8_t no_command = command_count; + + private: static void dispatch(std::string_view input) { // A line that overflowed the buffer is not a command - it is the tail of @@ -299,13 +376,7 @@ class terminal { return; } - std::uint8_t which = commands.size(); - for (std::uint8_t i = 0; i < commands.size(); ++i) { - if (matches(word, commands[i])) { - which = i; - break; - } - } + const auto which = lookup(word); switch (which) { case 0: @@ -377,7 +448,10 @@ class terminal { } public: - static constexpr std::string_view version = "v2.2"; + // A flash string, like every other literal the console prints: as a + // `string_view` it was the last constant left in RAM, holding its own + // characters and a pointer to them. + static constexpr auto version = "v2.2"_P; static constexpr std::uint8_t bar_max = 100; // Columns the dotted labels' colons land in, and the curve's span. All four diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 6b5bff8..aff37b5 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -2,7 +2,7 @@ libavr_format_test() # The image must stay below the boot section at 0x7e00, and the README states # what it measures; a library advance that moves it says nothing on its own. -libavr_size_claim_test(fantemp 8382) +libavr_size_claim_test(fantemp 8004) # The README says the two modes emit the same image, and only a tree with # both built can say whether they do. diff --git a/test/consteval.cpp b/test/consteval.cpp index 176d87a..05a731d 100644 --- a/test/consteval.cpp +++ b/test/consteval.cpp @@ -6,6 +6,7 @@ #include #include "curve.hpp" +#include "terminal.hpp" #include "thermistor.hpp" namespace { @@ -81,4 +82,39 @@ consteval bool thermistor_falls() } static_assert(thermistor_falls()); +// The console's one parsing rule, read out of the flash blob the names live +// in. Every case here is a property of the *order* the names sit in, so this +// is what an edit to that list has to answer to. +using app::terminal; + +// Full names, and the ends of the list - a walk that miscounts a separator +// lands on a neighbour rather than failing, so both ends are named. +static_assert(terminal::lookup("help") == 0); +static_assert(terminal::lookup("save") == 12); +static_assert(terminal::lookup("bootloader") == 4); + +// Abbreviations resolve to the first entry they prefix. `s` prefixes show, +// statistics, set and save, and show is first - which is the original's +// behaviour and the reason the list is ordered rather than sorted. +static_assert(terminal::lookup("s") == 1); +static_assert(terminal::lookup("st") == 6); +static_assert(terminal::lookup("se") == 9); +static_assert(terminal::lookup("sa") == 12); +static_assert(terminal::lookup("up") == 5); +static_assert(terminal::lookup("b") == 4); + +// `reset` is the exception: it must be typed in full, so no abbreviation of it +// resolves - and none of its prefixes names anything else either. +static_assert(terminal::lookup("reset") == 8); +static_assert(terminal::lookup("rese") == terminal::no_command); +static_assert(terminal::lookup("res") == terminal::no_command); +static_assert(terminal::lookup("r") == terminal::no_command); + +// Nothing, and nothing that matches. +static_assert(terminal::lookup("") == terminal::no_command); +static_assert(terminal::lookup("xyzzy") == terminal::no_command); + +// Longer than the name it prefixes is not a match: `helpful` is not `help`. +static_assert(terminal::lookup("helpful") == terminal::no_command); + } // namespace