Kaleidoscope
Kaleidoscope copied to clipboard
keyboardio
Focus parsing changes
Larger than a feature request, this type of issue is for describing a proposed change to how Kaleidoscope works, in advance of a draft PR.
What do you want to change?
Move all parsing of Focus input into FocusSerial itself. This is an incremental improvement to Focus. Further design improvements might be desired in the future.
There are probably no changes required to the protocol syntax for now. Improvements to the protocol syntax can be handled as a separate issue.
Details
Data types
Focus will define an enumerated list of data types that plugins may parse from commands. There will be a null type to indicate that no further items are expected.
Command table
Each Focus plugin will define a command table. Each entry will contain (at minimum) a string to invoke the command. Upon receiving the start of a command over the serial input, Focus will scan the command table of each plugin for a matching command. Upon detecting a match, Focus will call a plugin method (name TBD, possibly onFocusBeginCommand) with the index of the matching command table entry. The method will return the data type of the next expected item.
One advantage of using command tables is that plugins are no longer responsible for matching input strings to determine if they have received a valid command. This eliminates a lot of repetitive code in plugins.
Another advantage of using command tables is that Focus can automatically generate the help output without further input from the plugin.
Per-item callbacks
Each time Focus parses another item from the serial input, it will call the appropriate callback for the data type requested by the plugin. Each plugin will return the data type of the next expected item.
End of command
Upon reading EOL from the serial input, Focus will call a plugin method (name TDB, possibly onFocusEndCommand) to indicate that the command is complete. Focus will write \r\n.\r\n to serial output, as currently, to indicate end the of command output.
Parse errors
Focus will call a plugin method for a parse error encountered while reading an item (name TBD, possibly onFocusParseError).
Command output
Any command output can be written using the same techniques as currently.
Backward compatibility
Focus could continue to call onFocusEvent for backward compatibility with older plugins.
How will it make Kaleidoscope better?
-
Mitigate lockups due to malformed input (e.g., from probing by third-party software).
-
Mitigate settings corruption caused by timing quirks from software (chunked writes, etc.).
-
Lockups due to unread output from Focus are a separate issue.
What trouble might users have in adapting to the new functionality?
None, unless there are bugs.
What trouble might developers have in adapting to the new functionality?
This requires adapting plugins to a callback-based flow for reading commands. Therefore, each Focus plugin needs to keep additional state while a command is being parsed. This additional state might include the index of the command being executed, as well as loop counters for commands that accumulate array-like input.
Alternatives considered
Having the command table declare the format of the entire command is a possibility, allowing Focus to accumulate an entire buffer of parsed input to pass to the plugin. Unfortunately, this might occupy too much RAM for AVR.
In general, this sounds pretty good to me. I'd love to explore what the ram overhead would look like for having the command table declare the format of the entire command. My first gut reaction is that if we can make it go, it'll result in cleaner, simpler code.
Would the command table declaring the entire format require us to allocate memory for the parsed input of all commands, together? Because that basically means we need to have enough RAM free to hold a copy of eeprom - or at least, as much RAM as the number of bytes we use from EEPROM.
On the Model01, that's ~980 bytes. We currently have 1216 free, so we'd have ~236 left for local vars. A bit tight, but... possibly ok. Assuming that we don't end up using more memory otherwise.
On GD32, we already have a copy of EEPROM in RAM, now we'd need it twice - no big deal, we still have plenty, and if need be, we could optimize FlashStorage to not need a full copy.
Yeah, I was thinking that eeprom.contents as a write command would probably be the most extreme case (and one that's probably rarely used).
I'm not entirely convinced that eeprom.contents as a write command is actually generally useful.
On Tue, Oct 18, 2022 at 11:57 AM Taylor Yu @.***> wrote:
Yeah, I was thinking that eeprom.contents as a write command would probably be the most extreme case (and one that's probably rarely used).
— Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/1285#issuecomment-1282868678, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2HR5DA6ZNXB3BKCGSTWD3XJ3ANCNFSM6AAAAAARILNO7A . You are receiving this because you commented.Message ID: @.***>
I'm not entirely convinced that eeprom.contents as a write command is actually generally useful.
It... had its uses. I used to use it a lot when testing firmware changes: I backed up the eeprom of my own sketch, cleared eeprom, flashed the test firmware. Once done with testing, flash my own firmware back, restore eeprom as-is. It was simple. But the same thing can be accomplished in other ways, like by doing a structured backup. I now just use kaleidoscope-focus.rs's focus backup & focus restore tools.
In other words, if eeprom.contents is gone as a write command, I wouldn't shed a tear. It's still useful as a read command, for backup and diagnostics purposes, but not for writes. If we desperately need to write to random areas of the eeprom, we could have a targeted eeprom.fiddle <index> <byte> command. It wouldn't be the fastest thing to tweak larger slices of it, but it'd get the job done, and it should be rare enough that speed wouldn't matter much.
Would the command table declaring the entire format require us to allocate memory for the parsed input of all commands, together? Because that basically means we need to have enough RAM free to hold a copy of eeprom - or at least, as much RAM as the number of bytes we use from EEPROM.
It would mean that we would need to at least dynamically allocate enough RAM to hold the parsed arguments of the command with the longest arguments.
It would mean that we would need to at least dynamically allocate enough RAM to hold the parsed arguments of the command with the longest arguments.
Can't we statically allocate enough for the longest? Dynamic allocation on AVR is expensive.
Also, requiring the processing of extremely long lines tickles all kinds of bugs at all layers of the stack, so if we can migrate away from command protocols that use extremely long lines, that would be great.
Like maybe the keymap commands should only operate on one layer at a time. Or maybe raw EEPROM commands can take an offset and/or length.
I don't know if there were specific constraints that led to us having the extremely long command strings that we do today.
Can't we statically allocate enough for the longest? Dynamic allocation on AVR is expensive.
Probably. We'd need to have a way to make sure it's big enough.
I believe the current design was "a first pass" more than anything else.
I'd be thrilled for us to migrate to commands that operate on a single keymap at a time.
On Tue, Oct 18, 2022 at 12:11 PM Taylor Yu @.***> wrote:
Can't we statically allocate enough for the longest? Dynamic allocation on AVR is expensive.
Probably. We'd need to have a way to make sure it's big enough.
— Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/1285#issuecomment-1282883286, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2CZQMESOV4XGFXHCNTWD3Y6LANCNFSM6AAAAAARILNO7A . You are receiving this because you commented.Message ID: @.***>
Also, requiring the processing of extremely long lines tickles all kinds of bugs at all layers of the stack, so if we can migrate away from command protocols that use extremely long lines, that would be great.
Totally agreed. But if we're migrating away from extremely long lines, that means we'll have to change Chrysalis too to support both. If we go down that route, wouldn't it make more sense to try and come up with a protocol, maybe from scratch, that doesn't have the shortcomings of the current one?
I don't know if there were specific constraints that led to us having the extremely long command strings that we do today.
We didn't put much thought into it, really. Jesse wanted something more or less human readable, something that felt easy to parse.
I wanted to avoid having to know the length of stuff up front, at least for the keyboard->host direction, because to know the length, we often would have needed to iterate over the data twice, once to figure out the rendered length, and once more to actually print them. That felt complicated and slow. We could have avoided this in a number of cases, like if we always padded keycodes to 5 bytes or something, but I guess I just didn't think of that back then.
Like maybe the
keymapcommands should only operate on one layer at a time.
I think we may have had something like that at some point. I seem to remember having discussed the idea when Focus was being developed... not sure if we have the repo and/or issue available still where that happened. I might need to consult my IRC logs, because I think we talked there, and just pasted the discussion onto GitHub at the time.
But if we're migrating away from extremely long lines, that means we'll have to change Chrysalis too to support both. If we go down that route, wouldn't it make more sense to try and come up with a protocol, maybe from scratch, that doesn't have the shortcomings of the current one?
I think eventually, yes. At the moment, I want to focus on making what the existing protocol more robust, because designing and implementing a new protocol from scratch is a much longer-term effort, especially with the number of plugins we already have.
I think eventually, yes. At the moment, I want to focus on making what the existing protocol more robust, because designing and implementing a new protocol from scratch is a much longer-term effort, especially with the number of plugins we already have.
True, but if we're migrating away from extremely long lines, and breaking keymap.custom into layers, then we're essentially migrating to a new protocol anyway, because consumers (eg, Chrysalis) would need to adapt too. It would be slightly easier to do so, since it would be familiar, and we could migrate step by step. But I'm not convinced that trying to salvage Focus is worth the effort, and if spending that effort on coming up with something better wouldn't be a better use of resources.
I'm not entirely convinced that eeprom.contents as a write command is actually generally useful.
Just realized that we had one strong use-case for it as a write command: to clear EEPROM before we had eeprom.erase. That's no longer a use case however, since we do have eeprom.erase now.
I think we can leave some trickier commands like eeprom.contents using the legacy API, at least to start with.