From c58ff23e903fd5e042ae2d2148befdb4713cd44e Mon Sep 17 00:00:00 2001 From: BlackMark Date: Fri, 23 Oct 2026 21:50:01 +0200 Subject: [PATCH] console: each command carries its summary, and dispatch names it, -166 B The help text was thirteen help_row(N) calls and the dispatch thirteen case N labels, both counting positions in a list defined elsewhere. A command is now one row of name and summary; the summaries become a second NUL-separated flash blob printed in a loop, and every case label is command_index("name"), which does not compile for a name the list lacks. The help column derives from the longest name, the dotted padding is one helper for labels and help rows alike, and the ADC scale, the monitor interval and the uptime units are named. Output is unchanged; the image goes from 8004 B to 7838 B, both modes byte-identical. The 32-bit narrowing in dev/tasks.md is re-measured against the new image. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- dev/tasks.md | 4 +- src/controller.hpp | 6 +- src/terminal.hpp | 270 +++++++++++++++++++++----------------------- test/CMakeLists.txt | 2 +- 5 files changed, 137 insertions(+), 147 deletions(-) diff --git a/README.md b/README.md index d143819..bfca542 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ cmake --preset atmega328p-generated cmake --build --preset atmega328p-generated ``` -The firmware is **8004 B** of flash, byte-identical between the generated and +The firmware is **7838 B** of flash, byte-identical between the generated and reflect modes, and `ctest` holds it to that number. ## Atmel Studio diff --git a/dev/tasks.md b/dev/tasks.md index 8142c16..71c1e20 100644 --- a/dev/tasks.md +++ b/dev/tasks.md @@ -16,8 +16,8 @@ and re-propose work the owner has already refused. ### The millisecond clock stays 64 bits Narrowing `uptime::millis()` and the three timestamps that hold its value from -`uint64_t` to `uint32_t` is **608 B of flash, 7.6 % of the image** (8004 B down -to 7396), plus 16 B of RAM. +`uint64_t` to `uint32_t` is **602 B of flash, 7.7 % of the image** (7838 B down +to 7236), plus 16 B of RAM. **Refused, owner-stated: this board runs continuously and an uptime that restarts every 49.7 days is not acceptable.** That is what a 32-bit diff --git a/src/controller.hpp b/src/controller.hpp index 62547f3..83e4dff 100644 --- a/src/controller.hpp +++ b/src/controller.hpp @@ -38,11 +38,11 @@ class controller { fan::set_duty(avr::percent_t::of(m_percent)); } - static void set_manual(std::uint8_t p) + static void set_manual(std::uint8_t percent) { m_auto_mode = false; - m_percent = p; - fan::set_duty(avr::percent_t::of(p)); + m_percent = percent; + fan::set_duty(avr::percent_t::of(percent)); } static void set_automatic() diff --git a/src/terminal.hpp b/src/terminal.hpp index b0d80ad..deebb1c 100644 --- a/src/terminal.hpp +++ b/src/terminal.hpp @@ -3,6 +3,7 @@ #include #include #include +#include #include @@ -18,6 +19,11 @@ // until Ctrl+C, and Ctrl+C abandons a half-typed line anywhere else. namespace app { +struct command { + std::string_view name; + std::string_view summary; +}; + // 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, @@ -25,57 +31,71 @@ namespace app { // 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() +// blobs below are being built and never as storage of its own. +consteval auto commands() { - return std::to_array({ - "help", - "show", - "curve", - "monitor", - "bootloader", - "uptime", - "statistics", - "histogram", - "reset", - "set", - "auto", - "version", - "save", + return std::to_array({ + {"help", "prints this help message"}, + {"show", "shows current temperature and fan speed"}, + {"curve", "shows mapping from temperature to fan speed"}, + {"monitor", "loops the show command until Ctrl+C is pressed"}, + {"bootloader", "enters the bootloader"}, + {"uptime", "shows system uptime"}, + {"statistics", "prints overall statistics like min and max temp"}, + {"histogram", "prints a histogram of the temperature"}, + {"reset", "resets statistics to 0 in EEPROM and RAM (no abbreviation)"}, + {"set", "sets the fan speed to the provided value, 0-100"}, + {"auto", "turns on automatic fan control"}, + {"version", "displays firmware version"}, + {"save", "writes the statistics to EEPROM now"}, }); } +// Found by name rather than written as an index, so reordering the list cannot +// point a command at another one's code. A name not in the list does not +// compile. +consteval std::uint8_t command_index(std::string_view name) +{ + const auto list = commands(); + for (std::uint8_t i = 0; i < list.size(); ++i) { + if (list[i].name == name) { + return i; + } + } + std::unreachable(); +} + class terminal { static constexpr char ctrl_c = 0x03; static constexpr char backspace = 0x08; static constexpr char del = 0x7f; + static constexpr std::uint16_t monitor_interval_ms = 1'000; static inline std::array m_line{}; static inline std::uint8_t m_at = 0; static inline bool m_overflowed = false; static inline bool m_monitoring = false; static inline std::uint64_t m_last_monitor = 0; - static constexpr std::uint8_t command_count = command_names().size(); + static constexpr std::uint8_t command_count = commands().size(); - 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{}; + // One column of the command table as a NUL-separated blob in flash. Matched + // and printed 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. Separators rather than an offset table, + // because an offset table is the RAM this exists to give back. + template + static constexpr auto joined = [] { + constexpr auto bytes = [] { + std::size_t total = 0; + for (const auto &one : commands()) { + total += (one.*Column).size() + 1; + } + return total; + }(); + std::array chars{}; std::size_t at = 0; - for (auto one : command_names()) { - for (char c : one) { + for (const auto &one : commands()) { + for (char c : one.*Column) { chars[at++] = c; } chars[at++] = '\0'; @@ -83,103 +103,86 @@ class terminal { return chars; }(); - static constexpr avr::flash_table names{}; + static constexpr avr::flash_table> names{}; + static constexpr avr::flash_table> summaries{}; // `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()); - }(); + // data. + static constexpr std::uint8_t exact_command = command_index("reset"); - // 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 - // one dot - the original's layout exactly. - static constexpr std::uint8_t help_column = 12; + // One space and at least one dot after the longest name, the original's + // layout exactly. + static constexpr std::uint8_t help_column = [] { + std::size_t longest = 0; + for (const auto &one : commands()) { + longest = one.name.size() > longest ? one.name.size() : longest; + } + return static_cast(longest + 2); + }(); static void prompt() { serial << "> "_P; } - // `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. Its width comes from the type, so the padding needs no - // hand-counted constant. + // The tail of `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. + static void pad(std::uint8_t width, std::uint8_t column) + { + serial << ' '; + for (std::uint8_t i = width + 1; i < column; ++i) { + serial << '.'; + } + serial << ": "_P; + } + template static void label(Flash text, std::uint8_t column) { - serial << text << ' '; - for (auto i = Flash::size + 1; i < column; ++i) { - serial << '.'; - } - serial << ": "_P; + serial << text; + pad(Flash::size, column); } - // 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) + // Prints one entry of a blob, from `at` to its separator, and answers its + // width - counted as it is printed rather than measured first, which would + // read the blob twice. + template + static std::uint8_t print_entry(Blob blob, std::uint16_t at) { - auto at = name_start(which); std::uint8_t width = 0; - for (char c; (c = names.at(at + width)) != '\0'; ++width) { + for (char c; (c = blob.at(at + width)) != '\0'; ++width) { serial << c; } - serial << ' '; - for (std::uint8_t i = width + 1; i < help_column; ++i) { - serial << '.'; - } - serial << ": "_P; + return width; } - // Quarter- C as a signed decimal with a two-digit fraction. The sign is taken - // off first: C++ gives a negative remainder for a negative dividend, so - // `(q % 4) * 25` on -40.25 C yields -25 and prints "-40.-25". + // Quarter-degrees as a signed decimal with a two-digit fraction. The sign is + // taken off first: C++ gives a negative remainder for a negative dividend, so + // the fraction of -40.25 C would print as "-40.-25". static void temperature(std::int16_t quarters) { + constexpr auto per_degree = thermistor::quarters_per_degree; if (quarters < 0) { serial << '-'; } auto magnitude = static_cast(quarters < 0 ? -quarters : quarters); - serial << magnitude / 4 << '.' << avr::dec<{.width = 2, .fill = '0'}>((magnitude % 4) * 25); + serial << magnitude / per_degree << '.' + << avr::dec<{.width = 2, .fill = '0'}>((magnitude % per_degree) * (100 / per_degree)); } static void help() { serial << "\r\nFanTemp "_P << version << " command overview\r\n"_P; - help_row(0); - serial << "prints this help message\r\n"_P; - help_row(1); - serial << "shows current temperature and fan speed\r\n"_P; - help_row(2); - serial << "shows mapping from temperature to fan speed\r\n"_P; - help_row(3); - serial << "loops the show command until Ctrl+C is pressed\r\n"_P; - help_row(4); - serial << "enters the bootloader\r\n"_P; - help_row(5); - serial << "shows system uptime\r\n"_P; - help_row(6); - serial << "prints overall statistics like min and max temp\r\n"_P; - help_row(7); - serial << "prints a histogram of the temperature\r\n"_P; - help_row(8); - serial << "resets statistics to 0 in EEPROM and RAM (no abbreviation)\r\n"_P; - help_row(9); - serial << "sets the fan speed to the provided value, 0-100\r\n"_P; - help_row(10); - serial << "turns on automatic fan control\r\n"_P; - help_row(11); - serial << "displays firmware version\r\n"_P; - help_row(12); - serial << "writes the statistics to EEPROM now\r\n"_P; + std::uint16_t name = 0; + std::uint16_t summary = 0; + for (std::uint8_t i = 0; i < command_count; ++i) { + const auto width = print_entry(names, name); + name += width + 1; + pad(width, help_column); + summary += print_entry(summaries, summary) + 1; + serial << "\r\n"_P; + } serial << "commands may be abbreviated: 'up' is uptime\r\n"_P; } @@ -206,7 +209,7 @@ class terminal { return; } label("ADC value"_P, reading_column); - serial << controller::last_adc() << " / 1023\r\n"_P; + serial << controller::last_adc() << " / "_P << thermistor::adc_full_scale << "\r\n"_P; label("Resistance"_P, reading_column); if (auto ohms = resistance(); ohms == open_circuit) { @@ -247,9 +250,12 @@ class terminal { static void print_uptime() { + constexpr std::uint32_t minute = 60; + constexpr std::uint32_t hour = 60 * minute; + constexpr std::uint32_t day = 24 * hour; auto seconds = static_cast(uptime::millis() / 1000); - serial << seconds / 86400 << "d "_P << (seconds / 3600) % 24 << "h "_P << (seconds / 60) % 60 << "m "_P - << seconds % 60 << "s\r\n"_P; + serial << seconds / day << "d "_P << seconds % day / hour << "h "_P << seconds % hour / minute << "m "_P + << seconds % minute << "s\r\n"_P; } static void print_statistics() @@ -303,24 +309,10 @@ class terminal { } } - // 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) - { - std::uint8_t at = 0; - for (std::uint8_t i = 0; i < which; ++i) { - while (names.at(at) != '\0') { - ++at; - } - ++at; - } - 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. + // Which command a word names, or `no_command` 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 @@ -365,8 +357,8 @@ class terminal { return; } - // Split on the first space with the (pointer, length) constructor rather - // than substr, which throws - see matches(). + // Split on the first space with the (pointer, length) constructor: + // substr checks its bounds by throwing, which this build cannot. const auto space = input.find(' '); const auto word = space == std::string_view::npos ? input : std::string_view{input.data(), space}; const auto rest = space == std::string_view::npos @@ -376,40 +368,38 @@ class terminal { return; } - const auto which = lookup(word); - - switch (which) { - case 0: + switch (lookup(word)) { + case command_index("help"): help(); return; - case 1: + case command_index("show"): show(); return; - case 2: + case command_index("curve"): print_curve(); return; - case 3: + case command_index("monitor"): m_monitoring = true; return; - case 4: + case command_index("bootloader"): serial << "entering bootloader\r\n"_P; statistics::save(); serial.drain(); bootloader::enter(); - case 5: + case command_index("uptime"): print_uptime(); return; - case 6: + case command_index("statistics"): print_statistics(); return; - case 7: + case command_index("histogram"): print_histogram(); return; - case 8: + case command_index("reset"): statistics::reset(); serial << "statistics cleared in EEPROM and RAM\r\n"_P; return; - case 9: { + case command_index("set"): { std::uint16_t percent = 0; bool valid = !rest.empty(); for (char c : rest) { @@ -430,14 +420,14 @@ class terminal { } return; } - case 10: + case command_index("auto"): controller::set_automatic(); serial << "automatic fan control\r\n"_P; return; - case 11: + case command_index("version"): serial << "FanTemp "_P << version << " on libavr\r\n"_P; return; - case 12: + case command_index("save"): statistics::save(); serial << "statistics written to EEPROM\r\n"_P; return; @@ -473,7 +463,7 @@ class terminal { static void poll() { if (m_monitoring) { - if (uptime::millis() >= m_last_monitor + 1000) { + if (uptime::millis() >= m_last_monitor + monitor_interval_ms) { show(); m_last_monitor = uptime::millis(); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 5f38153..916c81b 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 8004) +libavr_size_claim_test(fantemp 7838) # The README says the two modes emit the same image, and only a tree with # both built can say whether they do.