keyboardio / Kaleidoscope

Firmware for Keyboardio keyboards and other keyboards with AVR or ARM MCUs.
http://keyboard.io
GNU General Public License v3.0
756 stars 259 forks source link

Rethinking kaleidoscope-builder #497

Closed algernon closed 4 years ago

algernon commented 5 years ago

For a while now, I've been pondering about replacing the current kaleidoscope-builder, for a number of reasons. I think the time has come when we should no longer push this task further down the roadmap, as the issues with the builder are showing. The most apparent problem is that it doesn't really support multiple keyboards: most of it is hacked in, and the Model01-centric stuff still leak through (try flashing anything else with it, for example... :P).

Before going into specific ideas, I'd like to enumerate the problems I see with kaleidoscope-builder:

algernon commented 5 years ago

Language

So, to have something more usable, we'd need it to be compiled. Preferably down to a single binary. In a language that supports all the major platforms (and the BSDs too!). I think this narrows us down to Go.

Configuration

Configuration should be as declarative as possible, and as much should be pulled from boards.txt and platforms.txt as much is possible. Everything needs to be overrideable though.

The order of parsing would be:

I suggest using Yaml as the config file format.

Hardware support

We need first-class support for different hardware, including messages, how to find the boards, and so on. I think it is ok if we hard-code a set of options, per-board. We can pick some things from boards.txt/platforms.txt, but not everything. We'd need special logic for stuff like the Prog key & reset on the Model01, a different message for putting the Splitography into flash mode (and there the flash is a three-step process after that: erase, upload, start). I think these should be built into the tool. As the tool is part of the repository, if we need to add new stuff, for new hardware, we can do so in the same PR.

Example

board: 
  atreus:
    mcu: teensy
    variant: post-2016
build:
  extra_cflags: -DSOMETHING=foo
  extra_library_dirs:
    - lib
    - /some/other/place
env:
  hardware_path: lib/hardware
  default_sketch: my-own

Most (if not all) paths should be turned absolute. We should probably support referencing environment variables, to make it easier to add stuff like GIT_REVISION and the like.

obra commented 5 years ago

Let's step back a step.

What features does the tool need to have? ᐧ

On Mon, Dec 10, 2018 at 9:44 AM Gergely Nagy notifications@github.com wrote:

Language

So, to have something more usable, we'd need it to be compiled. Preferably down to a single binary. In a language that supports all the major platforms (and the BSDs too!). I think this narrows us down to Go. Configuration

Configuration should be as declarative as possible, and as much should be pulled from boards.txt and platforms.txt as much is possible. Everything needs to be overrideable though.

The order of parsing would be:

  • built-in
  • $XDG_CONFIG_HOME/kaleidoscope/builder.yml
  • $SKETCHDIR/kaleidoscope-builder.yml
  • ./kaleidoscope-builder.yml
  • CLI arguments

I suggest using Yaml as the config file format. Hardware support

We need first-class support for different hardware, including messages, how to find the boards, and so on. I think it is ok if we hard-code a set of options, per-board. We can pick some things from boards.txt/ platforms.txt, but not everything. We'd need special logic for stuff like the Prog key & reset on the Model01, a different message for putting the Splitography into flash mode (and there the flash is a three-step process after that: erase, upload, start). I think these should be built into the tool. As the tool is part of the repository, if we need to add new stuff, for new hardware, we can do so in the same PR. Example

board: atreus: mcu: teensy variant: post-2016build: extra_cflags: -DSOMETHING=foo extra_library_dirs:

  • lib
  • /some/other/placeenv: hardware_path: lib/hardware default_sketch: my-own

Most (if not all) paths should be turned absolute. We should probably support referencing environment variables, to make it easier to add stuff like GIT_REVISION and the like.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/497#issuecomment-445905812, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaFaavEUhYJSHTpJyNDlfTLT52AL_ks5u3p2IgaJpZM4ZKCgj .

algernon commented 5 years ago

It should be able to build and flash sketches, with additional 3rd party plugins and custom flags, for all supported hardware.

obra commented 5 years ago

Nominally, that means that the newish Arduino CLI ought to be pretty close to what we would want, I think? ᐧ

On Mon, Dec 10, 2018 at 10:17 AM Gergely Nagy notifications@github.com wrote:

It should be able to build and flash sketches, with additional 3rd party plugins and custom flags, for all supported hardware.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/497#issuecomment-445916904, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaPn5Smr8WNXp_OqTqViCMsleucAQks5u3qU9gaJpZM4ZKCgj .

algernon commented 5 years ago

Yes and no... It's close enough for building, though I'd like a bit more abstraction over it. For flashing - we need our own thing, so we can have nice messages and prompts.

algernon commented 5 years ago

Following up on our Discord conversation: we could have a set of separate tools, one for building (a wrapper around arduino CLI, if that is working reliably enough by now), another for flashing. We'd get rid of kaleidoscope-builder completely, and just use make targets and small wrappers.

This would make things a little bit easier on windows too, because the wrappers would be small enough to maintain a PowerShell version too. Examples could have their own little Makefiles (that just include a common snippet and apply some configuration of their own if need be), and so on.

algernon commented 5 years ago

Just tried arduino-cli.... it's very far from being usable. It still segfaults on custom boards, and does not support additional/local libraries, making it unsuitable for our purposes. We'll have to wrap stuff a little bit longer, but splitting things up into smaller tools is a good idea regardless.

algernon commented 5 years ago

Over the past few days, I went ahead and made something: keyboardio/Kaleidoscope-Build-Tools#5 & #517. It's a make-based build system, where configuration is in the Makefiles too. I tried to arrange things so that only minimal configuration is required. For Model01-Firmware and most forks that do not use third-party libraries, all that needs to be done is to copy extras/Makefile.example, and put BOARD=model01 in rules.mk, the system does the rest.

The new system has much better support for hardware other than the Model01 (ie, no hardware is special), it's more modular, and has a better structure. It's also easier to special-case operating systems. And it is less code too.

TimNN commented 5 years ago

I'm probably a bit late to the discussion, but has anyone considered a CMake based build system, for example based on Arduino-CMake-NG?

I recently started to set up a build using that, and while things require a bit of manual set up at the moment, overall it's been grate:

  1. Much faster build times. A cold build (using Ninja) takes ~3s, a cached one about 0.5s. (The current build system takes ~13s, even for "cached" builds).
  2. CLion integration. It's not working perfectly, but it works (mostly, autocomplete / intelligent highlighting is slow / sluggish).
  3. Easy unit tests (based on the virtual keyboard, thanks for that!), which make implementing complex plugins much nicer.
  4. Less set up (IMO). It only requires CMake + Arduino installed. Then one just needs to checkout one git repo & submodules anywhere and is good to go.

The main disadvantage is that (currently) each Kaleidoscope source file is listed explicitly, but I personally don't mind that.


I assume that Ardunino-IDE based build will continue to be supported? If so, the CMake based build system can easily be supported along side whatever the official build system is, it only needs access to {platform,boards}.txt and variants/* (plus the actual source files).


The only change necessary to make Kaleidoscope compatible with the CMake build was changing model01.build.core=arduino:arduino to just model01.build.core=ardunino, however I have no idea what exactly that is doing.

obra commented 5 years ago

Hi Tim,

I'd love to know what kind of performance you see with the https://github.com/keyboardio/Kaleidoscope/tree/f/builder-improvements branch of Kaleidoscope and ccache installed. A couple things we were doing were unintentionally completely invalidating the caching that was supposed to take effect. I'm seeing sub-second rebuild times

I... haven't been able to get virtual builds to work usefully...ever. I'd love to hear more about what -is- working for you with virtual builds and tests.

If that change to the core name doesn't break Arduino builds, I'd be happy to take a PR for it.

Automatic IDE integration is certainly something that's desirable.

@noseglasses has done a lot of work on an Arduino-CMake based build system. Both @algernon and I have been hurt pretty badly by CMake based build systems in the past, so are generally pretty leery of it.

Since it is a core goal to maintain compatibility with the Arduino IDE, part of what worries me about something like Arduino-CMake-Ng is that it replaces Arduino's build system with something that's not-quite-the-same.

On Sat, Mar 2, 2019 at 9:39 AM Tim Neumann notifications@github.com wrote:

I'm probably a bit late to the discussion, but has anyone considered a CMake based build system, for example based on Arduino-CMake-NG https://github.com/arduino-cmake/Arduino-CMake-NG?

I recently started to set up a build using that, and while things require a bit of manual set up at the moment, overall it's been grate:

  1. Much faster build times. A cold build (using Ninja) takes ~3s, a cached one about 0.5s. (The current build system takes ~13s, even for "cached" builds).
  2. CLion integration. It's not working perfectly, but it works (mostly, autocomplete / intelligent highlighting is slow / sluggish).
  3. Easy unit tests (based on the virtual keyboard, thanks for that!), which make implementing complex plugins much nicer.
  4. Less set up (IMO). It only requires CMake + Arduino installed. Then one just needs to checkout one git repo & submodules anywhere and is good to go.

The main disadvantage is that (currently) each Kaleidoscope source file is listed explicitly, but I personally don't mind that.

I assume that Ardunino-IDE based build will continue to be supported? If so, the CMake based build system can easily be supported along side whatever the official build system is, it only needs access to {platform,boards}.txt and variants/* (plus the actual source files).

The only change necessary to make Kaleidoscope compatible with the CMake build was changing model01.build.core=arduino:arduino to just model01.build.core=ardunino, however I have no idea what exactly that is doing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/497#issuecomment-468942168, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaKB7BS5iV_b5euGVYHMnVngvgPvsks5vSrc_gaJpZM4ZKCgj .

TimNN commented 5 years ago

I'd love to know what kind of performance you see with the https://github.com/keyboardio/Kaleidoscope/tree/f/builder-improvements branch of Kaleidoscope and ccache installed.

That sounds pretty awesome, I'll take a look tomorrow or later this week. (Although the other two benefits still make a CMake based build worth it for me).

I'd love to hear more about what -is- working for you with virtual builds and tests.

So, I haven't tried (yet) to get the full virtual keyboard working (I'm actually only depending on the virtual keyboard headers right now). I've set up a mock / fake keyboard that kind of mimics the Kaleidoscope event loop (to the extent that I needed it in testing). It mainly does three things right now:

This allows me to write tests like this:

TEST_F(TapModTest, tmKeyReleasedShort_statePressedDelayed) {
  // Run one event loop iteration, where the "tm1" was toggled on ("Down").
  cycle({D(tm1)});  // "tm1" refers to a key of my plugin
  // Check that all recorded key events are as expected. In this case that
  // my plugin mapped "toggled on tm1" to "toggled on Key_E" ("Expect Down").
  verify({ED(Key_E)});
  // Another iteration, releasing the key ("Up").
  cycle({U(tm1)});
  // Verify that the real key event was consumed, and that another event was
  // generated ("Expect Held" == IS_PRESSED | WAS_PRESSED).
  verify({Consumed, EH(Key_E)});
  // This function is specific to my plugin and verifies its internal state.
  verify_state(State::PRESSED_DELAYED, State::IDLE);
}

You can find more test cases and and the fake keyboard here: TimNN/caleidoscope/tests.

If that change to the core name doesn't break Arduino builds, I'd be happy to take a PR for it.

At least in my version of Arduino all the board only have *.build.core=arduino. If arduino:arduino was not a deliberate change for Kaleidoscope, I guess just using arduino may work. I don't suppose you remember what you were thinking when writing https://github.com/keyboardio/Arduino-Boards/commit/0d48bf861864264dfdcee54ed5ffd94effb7bb4f back in 2016? ;).

Anyway, I'll try switching to arduino and see if the official build system still works. If so, I'll send a PR.

Since it is a core goal to maintain compatibility with the Arduino IDE, part of what worries me about something like Arduino-CMake-Ng is that it replaces Arduino's build system with something that's not-quite-the-same.

That's perfectly fine :). If compatibility with the Arduino IDE is being maintained, I'm perfectly happy since it doesn't seem much of an effort for me to maintain the CMake based build separately from the official build system.

noseglasses commented 5 years ago

I really appreciate the work that @obra spend on improving the kaleidoscope-builder in the recent days. I used ninja together with my own CMake-based build system before. it's lightning fast when it comes to rebuilds. But I think that after Jesse's improvements, build times shouldn't be a motivation for an alternative build system anymore.

For firmware testing with a virtual x86 system i wrote a python based keyboard simulator that wraps the virtual hardware. But after several month of absence from the firmware business until recently, I did not yet get around to adapt it to the latest changes in the firmware. I am pretty sure that it needs some overhaul.

The reason why I implemented these projects is that I was working on a pretty complex plugin that i intended to port from QMK to kaleidoscope. It requires additional build steps that kaleidoscope-builder is not designed to support. Also it is way to complex to get it working with build-flash-test cycles on a real hardware. I needed to know what was precicely going on inside the firmware, thereby having full control over the exact timing of key input. The same system can be used as a test harness. I implemented an alternative build system, a python based firmware simulator and a test framework because i needed these tool and there was no proper alternative.

I don't agree with Jesse when it comes to CMake as a build system framework. I use it since almost 15 years now so I know all of its pros and cons - and it has a lot of cons. But it is extremely portable and self contained. But i definitely agree with him about that it is not desirable to maintain something additional that replaces an existing and working build system. But that issue is independent from CMake.

Once a future kaleidoscope-builder replacement will be able to build and run other arbitrary build steps, using CMake will probably not be necessary anymore.

@TimNN, it's funny that you came asking pretty much the same questions like I did around one and a half year ago :smile:

TimNN commented 5 years ago

I know I'm probably a bit, but I still wanted to briefly report back

obra commented 5 years ago

Hi Tim,

On Thu, Mar 14, 2019 at 1:32 PM Tim Neumann notifications@github.com wrote:

I know I'm probably a bit, but I still wanted to briefly report back

Much appreciated.

-

Switching arduino:arduino to just arduino does not work. The specification https://github.com/arduino/Arduino/wiki/Arduino-IDE-1.5-3rd-party-Hardware-specification#referencing-another-core-variant-or-tool explains what the arduino: does: It is a "vendor ID", basically telling the builder to look in the core Arduino sources rather the Kaleidoscope directories for the core.

If I'm understanding right, that sounds like this might be a bug in the CMake build process, then. Is that right?

-

The build system improvements (which are already in master, I saw), are awesome, great work! I'll still be sticking with CMake for now due to the CLion integration & the ability to run unit tests. (I honestly can't imagine developing any moderately complex plugin with only the actual device to test on).

The lack of working unit tests is indeed crazy and frustrating and something we need to improve.

Is your setup with unit tests and cmake in git somewhere? I'd love to -try- it.

algernon commented 4 years ago

Closing this. I don't think we want to be going with something custom - as in, we do not want to rebuild kaleidoscope-builder. We do need to replace kaleidoscope-builder, but better build that on solid foundation. The CMake-based system mentioned above would be my prime candidate right now.

noseglasses commented 4 years ago

FYI, when searching for a way to pass special compile flags only to the sketch I just stumbled about this file in arduino-cli https://github.com/arduino/arduino-cli/blob/master/legacy/builder/create_cmake_rule.go

It seems they support exporting CMakeLists.txt, or will do so soon. I find their versioning system a bit confusion and therefore was not able to tell which version of arduino-cli/arduino-builder is used in the latest Arduino release.