terminal: enforce F(...) as commands

- store name+callback pairs in list instead of a hash map
  remove vector reserve() overhead
- rework callbacks to expect flash strings
- comments...

Small difference is that now there's no way to know if command already
exists. Meaning, while we do override the existing callback, comman
names list will have more than one entry.

Flash strings are quite weird. In case we finally get all `const char*`s
in PROGMEM (which is the case for ESP32?), this could use a comment-proposed
method of having handlers per-module instead of handlers per-command.

code/espurna/terminal.cpp

@@ -186,21 +186,21 @@ static unsigned char _serial_rx_pointer = 0;
 // -----------------------------------------------------------------------------
 
 void _terminalHelpCommand(const terminal::CommandContext& ctx) {
+    auto names = _terminal.names();
 
-    // Get sorted list of commands
-    auto commands = _terminal.commandNames();
-    std::sort(commands.begin(), commands.end(), [](const String& rhs, const String& lhs) -> bool {
-        return lhs.compareTo(rhs) > 0;
+    // XXX: Core's ..._P funcs only allow 2nd pointer to be in PROGMEM,
+    //      explicitly load the 1st one
+    std::sort(names.begin(), names.end(), [](const __FlashStringHelper* lhs, const __FlashStringHelper* rhs) {
+        const String lhs_as_string(lhs);
+        return strncasecmp_P(lhs_as_string.c_str(), reinterpret_cast<const char*>(rhs), lhs_as_string.length()) < 0;
     });
 
-    // Output the list asap
     ctx.output.print(F("Available commands:\n"));
-    for (auto& command : commands) {
-        ctx.output.printf("> %s\n", command.c_str());
+    for (auto* name : names) {
+        ctx.output.printf("> %s\n", reinterpret_cast<const char*>(name));
     }
 
     terminalOK(ctx.output);
-
 }
 
 #if LWIP_VERSION_MAJOR != 1
@@ -635,7 +635,7 @@ void terminalInject(char ch) {
     _io.inject(ch);
 }
 
-void terminalRegisterCommand(const String& name, terminal::Terminal::CommandFunc func) {
+void terminalRegisterCommand(const __FlashStringHelper* name, terminal::Terminal::CommandFunc func) {
     terminal::Terminal::addCommand(name, func);
 };
 

code/espurna/terminal.h

@@ -29,7 +29,7 @@ void terminalError(Print&, const String& error);
 void terminalOK(const terminal::CommandContext&);
 void terminalError(const terminal::CommandContext&, const String&);
 
-void terminalRegisterCommand(const String& name, terminal::Terminal::CommandFunc func);
+void terminalRegisterCommand(const __FlashStringHelper* name, terminal::Terminal::CommandFunc func);
 
 size_t terminalCapacity();
 void terminalInject(void *data, size_t len);

code/espurna/terminal_commands.cpp

@@ -17,23 +17,41 @@ Heavily inspired by the Embedis design:
 
 namespace terminal {
 
-std::unordered_map<String, Terminal::CommandFunc,
-    parsing::LowercaseFnv1Hash<String>,
-    parsing::LowercaseEquals<String>> Terminal::commands;
-
-void Terminal::addCommand(const String& name, CommandFunc func) {
-    if (!func) return;
-    commands.emplace(std::make_pair(name, func));
+Terminal::Commands Terminal::_commands;
+
+// TODO: `name` is never checked for uniqueness, unlike the previous implementation with the `unordered_map`
+// (and note that the map used hash IDs instead of direct string comparison)
+//
+// One possible workaround is to delegate command matching to a callback function of a module:
+//
+// > addCommandMatcher([](const String& argv0) -> Callback {
+// >     if (argv0.equalsIgnoreCase(F("one")) {
+// >         return cmd_one;
+// >     } else if (argv0.equalsIgnoreCase(F("two")) {
+// >         return cmd_two;
+// >     }
+// >     return nullptr;
+// > });
+// 
+// Or, using a PROGMEM static array of `{progmem_name, callback_ptr}` pairs.
+// There would be a lot of PROGMEM boilerplate, though, since PROGMEM strings cannot be
+// written inline with the array itself, and must be declared with a symbol name beforehand.
+// (unless, progmem_name is a fixed-size char[], but then it must have a special limit for command length)
+
+void Terminal::addCommand(const __FlashStringHelper* name, CommandFunc func) {
+    if (func) {
+        _commands.emplace_front(std::make_pair(name, func));
+    }
 }
 
-size_t Terminal::commandsSize() {
-    return commands.size();
+size_t Terminal::commands() {
+    return std::distance(_commands.begin(), _commands.end());
 }
 
-std::vector<String> Terminal::commandNames() {
-    std::vector<String> out;
-    out.reserve(commands.size());
-    for (auto& command : commands) {
+Terminal::Names Terminal::names() {
+    Terminal::Names out;
+    out.reserve(commands());
+    for (auto& command : _commands) {
         out.push_back(command.first);
     }
     return out;
@@ -42,37 +60,45 @@ std::vector<String> Terminal::commandNames() {
 Terminal::Result Terminal::processLine() {
 
     // Arduino stream API returns either `char` >= 0 or -1 on error
-    int c = -1;
-    while ((c = stream.read()) >= 0) {
-        if (buffer.size() >= (buffer_size - 1)) {
-            buffer.clear();
+    int c { -1 };
+    while ((c = _stream.read()) >= 0) {
+        if (_buffer.size() >= (_buffer_size - 1)) {
+            _buffer.clear();
             return Result::BufferOverflow;
         }
-        buffer.push_back(c);
+        _buffer.push_back(c);
         if (c == '\n') {
             // in case we see \r\n, offset minus one and overwrite \r
-            auto end = buffer.end() - 1;
+            auto end = _buffer.end() - 1;
             if (*(end - 1) == '\r') {
                 --end;
             }
             *end = '\0';
 
-            // parser should pick out at least one arg (command)
-            auto cmdline = parsing::parse_commandline(buffer.data());
-            buffer.clear();
-            if (cmdline.argc >= 1) {
-                auto command = commands.find(cmdline.argv[0]);
-                if (command == commands.end()) return Result::CommandNotFound;
-                (*command).second(CommandContext{std::move(cmdline.argv), cmdline.argc, stream});
+            // parser should pick out at least one arg aka command
+            auto cmdline = parsing::parse_commandline(_buffer.data());
+            _buffer.clear();
+            if ((cmdline.argc >= 1) && (cmdline.argv[0].length())) {
+                auto found = std::find_if(_commands.begin(), _commands.end(), [&cmdline](const Command& command) {
+                    // note that `String::equalsIgnoreCase(const __FlashStringHelper*)` does not exist, and will create a temporary `String`
+                    // both use read-1-byte-at-a-time for PROGMEM, however this variant saves around 200μs in time since there's no temporary object
+                    auto* lhs = cmdline.argv[0].c_str();
+                    auto* rhs = reinterpret_cast<const char*>(command.first);
+                    auto len = strlen_P(rhs);
+
+                    return (cmdline.argv[0].length() == len) && (0 == strncasecmp_P(lhs, rhs, len));
+                });
+                if (found == _commands.end()) return Result::CommandNotFound;
+                (*found).second(CommandContext{std::move(cmdline.argv), cmdline.argc, _stream});
                 return Result::Command;
             }
         }
     }
 
     // we need to notify about the fixable things
-    if (buffer.size() && (c < 0)) {
+    if (_buffer.size() && (c < 0)) {
         return Result::Pending;
-    } else if (!buffer.size() && (c < 0)) {
+    } else if (!_buffer.size() && (c < 0)) {
         return Result::NoInput;
     // ... and some unexpected conditions
     } else {
@@ -90,4 +116,4 @@ void Terminal::process(ProcessFunc func) {
     }    
 }
 
-} // ns terminal
+} // namespace terminal

code/espurna/terminal_commands.h

@@ -12,7 +12,7 @@ Copyright (C) 2020 by Maxim Prokhorov <prokhorov dot max at outlook dot com>
 
 #include "terminal_parsing.h"
 
-#include <unordered_map>
+#include <forward_list>
 #include <functional>
 #include <vector>
 
@@ -28,8 +28,8 @@ struct CommandContext {
     Print& output;
 };
 
-struct Terminal {
-
+class Terminal {
+public:
     enum class Result {
         Error,           // Genric error condition
         Command,         // We successfully parsed the line and executed the callback specified via addCommand
@@ -42,18 +42,22 @@ struct Terminal {
     using CommandFunc = void(*)(const CommandContext&);
     using ProcessFunc = bool(*)(Result);
 
+    using Names = std::vector<const __FlashStringHelper*>;
+    using Command = std::pair<const __FlashStringHelper*, CommandFunc>;
+    using Commands = std::forward_list<Command>;
+
     // stream      - see `stream` description below
     // buffer_size - set internal limit for the total command line length
     Terminal(Stream& stream, size_t buffer_size = 128) :
-        stream(stream),
-        buffer_size(buffer_size)
+        _stream(stream),
+        _buffer_size(buffer_size)
     {
-        buffer.reserve(buffer_size);
+        _buffer.reserve(buffer_size);
     }
 
-    static void addCommand(const String& name, CommandFunc func);
-    static size_t commandsSize();
-    static std::vector<String> commandNames();
+    static void addCommand(const __FlashStringHelper* name, CommandFunc func);
+    static size_t commands();
+    static Names names();
 
     // Try to process a single line (until either `\r\n` or just `\n`)
     Result processLine();
@@ -69,22 +73,18 @@ struct Terminal {
     static bool defaultProcessFunc(Result);
 
     // general input / output stream:
-    // - stream.read() should return user iput
+    // - stream.read() should return user input
     // - stream.write() can be called from the command callback
     // - stream.write() can be called by us to show error messages
-    Stream& stream;
+    Stream& _stream;
 
-    // buffer for the input stream, fixed in size
-    std::vector<char> buffer;
-    const size_t buffer_size;
-
-    // TODO: every command is shared, instance should probably also have an
-    //       option to add 'private' commands list?
-    // Note: we can save ~2.5KB by using std::vector<std::pair<String, CommandFunc>>
-    //       https://github.com/xoseperez/espurna/pull/2247#issuecomment-633689741
-    static std::unordered_map<String, CommandFunc,
-        parsing::LowercaseFnv1Hash<String>,
-        parsing::LowercaseEquals<String>> commands;
+    // input stream is buffered until it can be parsed
+    // in case parsing did not happen and we filled the buffer up to it's size,
+    // the error will be returned and the buffer parsing will start from the beginning
+    std::vector<char> _buffer;
+    const size_t _buffer_size;
 
+    static Commands _commands;
 };
-}
+
+} // namespace terminal

code/espurna/terminal_parsing.cpp

@@ -202,27 +202,29 @@ err:
 
 // Fowler–Noll–Vo hash function to hash command strings that treats input as lowercase
 // ref: https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function
-template<>
-size_t LowercaseFnv1Hash<String>::operator()(const String& str) const {
+//
+// This is here in case `std::unordered_map` becomes viable
+// TODO: afaik, map implementation should handle collisions (however rare they are in our case)
+// if not, we can always roll static commands allocation and just match strings with strcmp_P
+
+uint32_t lowercase_fnv1_hash(const char* ptr) {
     constexpr uint32_t fnv_prime = 16777619u;
     constexpr uint32_t fnv_basis = 2166136261u;
 
+    const auto length = strlen_P(ptr);
+
     uint32_t hash = fnv_basis;
-    for (size_t idx = 0; idx < str.length(); ++idx) {
-        // TODO: String::operator[] is slightly slower here
-        //       does not happen with the std::string
-        hash = hash ^ static_cast<uint32_t>(tolower(str.c_str()[idx]));
+    for (size_t idx = 0; idx < length; ++idx) {
+        hash = hash ^ static_cast<uint32_t>(tolower(pgm_read_byte(&ptr[idx])));
         hash = hash * fnv_prime;
     }
 
     return hash;
 }
 
-template<>
-bool LowercaseEquals<String>::operator()(const String& lhs, const String& rhs) const {
-    return lhs.equalsIgnoreCase(rhs);
+uint32_t lowercase_fnv1_hash(const __FlashStringHelper* ptr) {
+    return lowercase_fnv1_hash(reinterpret_cast<const char*>(ptr));
 }
 
-
 } // namespace parsing
 } // namespace terminal

code/espurna/terminal_parsing.h

@@ -25,21 +25,5 @@ struct CommandLine {
 
 CommandLine parse_commandline(const char *line);
 
-// Fowler–Noll–Vo hash function to hash command strings that treats input as lowercase
-// ref: https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function
-//
-// TODO: afaik, unordered_map should handle collisions (however rare they are in our case)
-//       if not, we can always roll static commands allocation and just match strings
-//       with LowercaseEquals (which is not that much slower)
-template <typename T>
-struct LowercaseFnv1Hash {
-    size_t operator()(const T& str) const;
-};
-
-template <typename T>
-struct LowercaseEquals {
-    bool operator()(const T& lhs, const T& rhs) const;
-};
-
-}
-}
+} // namespace parsing
+} // namespace terminal