Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote calls - a simple, resource friendly communication interface #801

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

noseglasses
Copy link
Collaborator

@noseglasses noseglasses commented Jan 16, 2020

Please read doc/remote_call.md added by this PR before reviewing.


I spend the last month on a rock climbing trip in Greece. Between climbs on beautiful red limestone cliffs above a nice little town close to the Aegen Sea I finally found time to think about an idea that I was carrying around with me for quite a while.

During the long winter evenings I managed to turn this idea into code. The outcome is a boilerplate-free, type safe and resource friendly serial communication/remote control interface.

Disclaimer: This is not meant to replace the Focus interface which has its own merits. The new infrastructure intents to replace some of the plugins' Focus event handlers with something that

  • is easier to read and easier to maintain due to a well defined API,
  • supports type safe communication,
  • allows for integrated documentation of the interface at zero cost and
  • will allow for a much greater amount of functionality to be exported for
    remote control at the same costs (see the Resource Consumption section in the doc file).

Focus is still used under the hood.

Here's a little usage example (GNU/Linux):

# Scan firmware build
> ./scan_packages \
   --binutils_dir=$HOME/Software_Local/arduino-1.8.10/hardware/tools/avr/bin/ \
   --binutils_prefix=avr- \
   --sketch_dir=/tmp/kaleidoscope-$USER/sketch/1575610-Model01-Firmware.ino \
   --yaml_output_file=bla.yml
...

# Communicated with the device
> DEVICE=/dev/ttyACM3 ./remote \
    --kaleidoscope_repo_dir=$PWD/../../ \
    --interface_description_yaml=$PWD/bla.yml \
    call plugin::LEDControl::setCrgbAt row=2 col=1 red=255 green=255 blue=0

Kaleidoscope remote call utility
   2019 by Florian Fleissner (shinynoseglasses@gmail.com)

Calling Kaleidoscope function plugin::LEDControl::setCrgbAt
arguments:
   row = 2 [address = 552, type = uint8_t, size = 1]
   col = 1 [address = 553, type = uint8_t, size = 1]
   red = 255 [address = 554, type = uint8_t, size = 1]
   green = 255 [address = 555, type = uint8_t, size = 1]
   blue = 0 [address = 556, type = uint8_t, size = 1]
results:
   [void]

I am aware that this PR is a complex thing. Because of that, after implementing and polishing it I wrote a long documentation doc/remote_call.md. Please read it before starting a review. I hope it will answer many questions. If you find it missing any relevant information, please let me know so I can update it. That's probably more efficient than providing lengthy explanations in this PRs discussion thread that later have to be added to the documentation anyway.

Prerequisites of this PR:

During review, please ignore commits (0c22758) and (a01b97c) as those are already covered by #799 and #800. They have only been added to this PR to simplify final rebasing.

Once this PR has been approved, I will rebase only the relevant commits on top of master.

Florian Fleissner added 2 commits January 16, 2020 12:03
By means of this macro, headers can check if they are compiled
in the sketch compilation unit or in any other compilation unit.

Signed-off-by: Florian Fleissner <florian.fleissner@inpartik.de>
Default plugins are those that are automatically added to the list
of plugins considered by KALEIDOSCOPE_INIT_PLUGINS(...).

They can either be defined to precede or trail the list defined by
KALEIDOSCOPE_INIT_PLUGINS(...).

This commit adds the necessary infrastructure but does not add any
default plugins by itself.

Signed-off-by: Florian Fleissner <florian.fleissner@inpartik.de>
@noseglasses
Copy link
Collaborator Author

It seems like travis still hasn't adopted the sketch-header/footer thing. Travis failing seems to be related to #774.

@noseglasses noseglasses force-pushed the pr_remote_call branch 2 times, most recently from 2dc742d to f785d8f Compare January 16, 2020 16:49
Florian Fleissner added 4 commits January 16, 2020 17:54
Please read doc/remote_call.md for a detailed description.

Signed-off-by: Florian Fleissner <florian.fleissner@inpartik.de>
Signed-off-by: Florian Fleissner <florian.fleissner@inpartik.de>
Signed-off-by: Florian Fleissner <florian.fleissner@inpartik.de>
Signed-off-by: Florian Fleissner <florian.fleissner@inpartik.de>
@algernon
Copy link
Contributor

Oh boy, do I have some fancy ideas. I have not read the entire documentation yet, but I'd like to share a few ideas anyway, before I forget.

The main issue I see is that until today, we mostly carried the .hex file around. However, that's no longer enough if we want to back up our compiled firmwares. We need the .elf, the .map, the .hex, and the .yaml. Some of those can be derived from the others (elf->hex; elf+map -> yaml). Can we generate .map from the .elf? I know not all of it can be (ie, removed sections can't), but the parts the tool needs - are those generatable from the elf file?

In any case, it would be very helpful if either Arduino itself, or at worst, kaleidoscope-builder would produce a single file with everything useful. Be that a tar, a zip, or an ELF with custom sections.

Since we create an .elf on all platforms, I suppose we could just push all the stuff we want as extra into custom ELF sections with a linker script. Why not a tar or zip? Because we already have the tooling for ELF installed, on all platforms.

With this packed way, people would only need to carry that one file around, and generate anything else from there. As a reasonable compromise, I'd put the hex, the map, and the yaml into ELF sections, so we wouldn't need tools to regenerate them, just a generic elf-extraction tool. There's an ELF parser for Node, so Chrysalis could easily support this elf package. This would make it easier for end-users to keep things in sync and backed up, since there's only one file they need to carry around. We could also teach Chrysalis to save ELF packs it flashes, and when connecting to a keyboard, look at the firmware checksum, and try find the ELF file in its cache, and use the yaml from there, if found - thus automating away the scary parts.

Mind you, for this to work, we'd need the scan_packages tool to work on all 3 major platforms, preferably without having to install extra dependencies. So I guess a compiled language like Go, Rust, or Haskell (all of which compile to a single static binary without any other dependencies) would go a long way. We could also use Java, since we have that due to the IDE, but... I'd rather move away from having to have the IDE around, rather than tieing ourselves closer to it.

Mind you, all of the above are just ideas, and not a request to update this PR with those in mind. All of the above can be built upon this PR, if we decide to go down that route.

(I'm liking what I'm seeing so far, but I'm nowhere near finished reading through yet.)

@algernon
Copy link
Contributor

I'm liking the idea, and this can be incredibly useful for a whole lot of things, at the cost of having to make the host libraries smarter, in some cases, a lot smarter. One particularly critical case is the keymap: right now, we transfer all layers back and forth with one command. This is simple on both sides, and is a reasonably bandwidth-efficient operation. Take the Model01 as an example: with 4 layers at 64 keys each, and 3-byte keycodes on average, we're talking about 1024 bytes for the result, and a single keymap.custom to retrieve the keymap. With this RPC, we're looking at remote.poke ADDR N and remote.call ADDR for all 64*4 keys for the call alone, and about 768 bytes for the results. That's almost an order of magnitude more traffic over 9600 baud. This will be noticably slower than the current method. Still within acceptable limits, I guess, but slower still. Updating the whole keymap would be similarly slow.

This can, of course, be mitigated to some extent in various ways:

  • For updates, we can send only the differences, since we can address things with finer granularity.
  • We can use the traditional Focus commands for the few outliers that benefit from bigger amounts of data being transferred (keymap & colormap at this time).

Additionally, with RPC, it becomes a little harder to do REPL-style poking at the device: we have to make host-side tools smarter first. The trivial thing that is focus-test is no longer enough. I wonder if we should abandon Focus when using RPC, and instead do it in a more... firmware-efficient way instead?

I have a couple of more... "feels", will post those later after digesting things a bit longer, likely later today.

@noseglasses
Copy link
Collaborator Author

@algernon, thanks for sharing your ideas and for bringing up RPC. It's good to have some proper terminology at hand. I was not aware of that, to be honest.

We need the .elf, the .map, the .hex, and the .yaml. Some of those can be derived from the others (elf->hex; elf+map -> yaml).

Currently KRC uses only the sketch object file and the map file to generate the yaml. It could possibly gain the same information from the elf file instead of the map. But parsing the latter is easier.

Can we generate .map from the .elf? I know not all of it can be (ie, removed sections can't), but the parts the tool needs - are those generatable from the elf file?

The map file is generated while linking the elf. Some of the information like discarded sections
are no more present in the elf file, AFAIK. That's why elf->map is probably not possible.

In any case, it would be very helpful if either Arduino itself, or at worst, kaleidoscope-builder would produce a single file with everything useful. Be that a tar, a zip, or an ELF with custom sections. Since we create an .elf on all platforms, I suppose we could just push all the stuff we want as extra into custom ELF sections with a linker script. Why not a tar or zip? Because we already have the tooling for ELF installed, on all platforms.

Why would we need anything else than the yaml (for RPC) and the hex (for re-flashing the firmware)?
A zip solution sounds reasonable. But with the Arduino build system this is almost impossible as it would need compression tools that are shipped with vanilla versions of all three major platforms. Unless we start shipping tool binaries with Kaleidoscope...

With this packed way, people would only need to carry that one file around, and generate anything else from there. As a reasonable compromise, I'd put the hex, the map, and the yaml into ELF sections, so we wouldn't need tools to regenerate them, just a generic elf-extraction tool. There's an ELF parser for Node, so Chrysalis could easily support this elf package. This would make it easier for end-users to keep things in sync and backed up, since there's only one file they need to carry around. We could also teach Chrysalis to save ELF packs it flashes, and when connecting to a keyboard, look at the firmware checksum, and try find the ELF file in its cache, and use the yaml from there, if found - thus automating away the scary parts.

I am not sure how that would work with the elf->hex step. Doesn't that simply copy all sections from the elf, including those additional ones? To my knowledge, the location process is part of the linker step that creates the elf file. If not, a custom linker script could definitely do the job.
BTW, exporting symbols from C++ to custom sections is broken with avr-gcc.

Mind you, for this to work, we'd need the scan_packages tool to work on all 3 major platforms, preferably without having to install extra dependencies. So I guess a compiled language like Go, Rust, or Haskell (all of which compile to a single static binary without any other dependencies) would go a long way.

I am not familiar with any of those languages although all of them are interesting. As Kaleidoscope is a C++ firmware, for me it would be most logic to use C++ for such a tool, in order not to run into a Babylonic language problem.

The reason why I used Python for those tools is A) its a nice language that is convenient and simple for trying out things, B) I am used to it.

Oh, and there is another (tiny) reason why Python may be not the worsed choice. Platformio as a possible future candidate for replacing kaleidoscope-builder or even Arduino is entirely written in Python so the interpreter will be there anyway. Python is easy to install on all major platforms.

We could also use Java, since we have that due to the IDE, but... I'd rather move away from having to have the IDE around, rather than tieing ourselves closer to it.

Unfortunately not. I considered Java in another occasion but it turned out that at least on MacOS there is just a Java runtime shipped for the GUI, but no interpreter.

One particularly critical case is the keymap: right now, we transfer all layers back and forth with one command. This is simple on both sides, and is a reasonably bandwidth-efficient operation. Take the Model01 as an example: with 4 layers at 64 keys each, and 3-byte keycodes on average, we're talking about 1024 bytes for the result, and a single keymap.custom to retrieve the keymap. With this RPC, we're looking at remote.poke ADDR N and remote.call ADDR for all 64*4 keys for the call alone, and about 768 bytes for the results. That's almost an order of magnitude more traffic over 9600 baud. This will be noticably slower than the current method. Still within acceptable limits, I guess, but slower still. Updating the whole keymap would be similarly slow.

That's correct.
9600 baud? The Atmega32U4 can go at a much higher serial transfer rate (that is fixed to my knowledge). I read some forum posts that claim that setting the serial baud rate manually is simply ignored by the MCU. But can't remember now where that was.

Anyway, I guess the host's overhead of invoking multiple scripts per key will already be a bottleneck, not sure though. We should have a try to see how slow it actually would be.

For updates, we can send only the differences, since we can address things with finer granularity.
We can use the traditional Focus commands for the few outliers that benefit from bigger amounts of data being transferred (keymap & colormap at this time).

That sounds pretty reasonable.

Additionally, with RPC, it becomes a little harder to do REPL-style poking at the device: we have to make host-side tools smarter first.

Can you explain a bit what that means? I am not familiar with REPL.

@noseglasses
Copy link
Collaborator Author

Here's some related discussion about the serial baud rate of the Arduino Leonardo: https://forum.arduino.cc/index.php?topic=268834.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants