keyboardio / Kaleidoscope-Bundle-Keyboardio

A Kaleidoscope distribution for the Keyboardio Model 01 and other keyboards.
Other
17 stars 24 forks source link

Need more instructions for how to setup build environment #22

Closed rljacobson closed 4 years ago

rljacobson commented 4 years ago

Hi! I've made some custom hardware to drive the Tardis keyboard. I was expecting to have to write the firmware from scratch, but Kaleidoscope has 90% of what I need, plus a ton of extra stuff, and the code is so well structured that it's pretty easy to write the code to support my hardware. But there are a few issues:

  1. The Tardis keyboard hardware has support for 251 keys, but only 244 switches are installed. Electronically, they are organized into 8 rows and 32 columns. This violates the assumption of MatrixAddr that every address corresponds to a key, i.e. that rows*columns = number of keys. The simplest fix breaks the static assert, making the code less safe.
  2. My implementation of the key matrix scanner is very different from Kaleidoscope's assumption that rows&columns correspond to GPIO pins. I would need 40 pins just for the key matrix! Instead I bit-bang control data to six (!!) daisy chained shift registers via SPI.
  3. The shift registers not only control the key matrix scanner but also the LEDs, which does not seem to be the assumption of Kaleidoscope. I can provide a separate interface to the LEDs and scanner, but they will ultimately be coupled. One consequence: the scanner frequency is an integer multiple of the LED frequency or vice versa.
  4. There are 148 LEDs, all of which are monochrome red, which can function as indicators on windowed keys, e.g. caps lock, or can be used for custom purposes. I can achieve a very fast SPI frequency, which allows me to vary the brightness of the LEDs by PWM. This doesn't fit the LED model of Kaleidoscope. One solution is to treat any nonzero color vector as "on" and zero as "off." Alternatively, I could convert to grayscale, varying the brightness according to value (in the HSV sense of the word).
  5. I'm using the "Blue Pill," which has an ARM Cortex-M3 based microcontroller, not AVR. But it appears the vast majority of Kaleidoscope is MCU agnostic. It's not clear to me where the dependencies are that make Kaleidoscope for AVR MCUs.
  6. You guys seem to be on the verge of a dramatic hardware API change, and it's not clear when and on which branch I should write a new hardware implementation.
  7. How do I set up the build environment? This is very mysterious. Most of the build scripts appear to assume the device is the Model 01. There are bits and pieces of build-related stuff all over the place. It's not clear how everything fits together.

The code base itself is pretty easy to understand, so I could just copy the Kaleidoscope code that's relevant to my project, make all the changes I want, and be done with it. But I think my code would be more useful if myself and other people could use it as part of a feature rich, actively maintained framework. And there may be some value to potential future developers in having an example of a device implementation for which the off-the-shelf classes can't be used.

Do you have any recommendations?

algernon commented 4 years ago

Hi! I've made some custom hardware to drive the Tardis keyboard. I was expecting to have to write the firmware from scratch, but Kaleidoscope has 90% of what I need, plus a ton of extra stuff, and the code is so well structured that it's pretty easy to write the code to support my hardware.

That's a very interesting board!

  1. The Tardis keyboard hardware has support for 251 keys, but only 244 switches are installed. Electronically, they are organized into 8 rows and 32 columns. This violates the assumption of MatrixAddr that every address corresponds to a key, i.e. that rows*columns = number of keys. The simplest fix breaks the static assert, making the code less safe.

rows*columns doesn't have to be equal to the number of keys - we already have a number of keyboards in the tree which have less keys than what their matrix would suggest: Splitography, ErgoDox, Atreus and Atreus2.

MatrixAddr is for addressing potential keys. If your keyboard has less than what it can address, no problem: just ignore those positions! As you can see, the linked keyboards have their PER_KEY_DATA set up so that unused keys are XXX or dflt, both of which mean the position will never generate any events.

  1. My implementation of the key matrix scanner is very different from Kaleidoscope's assumption that rows&columns correspond to GPIO pins. I would need 40 pins just for the key matrix! Instead I bit-bang control data to six (!!) daisy chained shift registers via SPI.

That's ok, we have boards with different scanners too: the Model01's scanner also controls the LEDs for example, the ErgoDox has an extender, so having another way to scan is no biggie. Especially with the hardware API changes coming in keyboardio/Kaleidoscope#695, a new scanner will be even easier to add and support.

  1. The shift registers not only control the key matrix scanner but also the LEDs, which does not seem to be the assumption of Kaleidoscope. I can provide a separate interface to the LEDs and scanner, but they will ultimately be coupled. One consequence: the scanner frequency is an integer multiple of the LED frequency or vice versa.

The Model01 works similarly: the scanner controls the LEDs too, so this is nothing new :)

I suppose you were looking at ATMegaScanner, and things within the Kaleidoscope repo mostly? KeyboardioScanner is what the Model01 uses.

  1. There are 148 LEDs, all of which are monochrome red, which can function as indicators on windowed keys, e.g. caps lock, or can be used for custom purposes. I can achieve a very fast SPI frequency, which allows me to vary the brightness of the LEDs by PWM. This doesn't fit the LED model of Kaleidoscope. One solution is to treat any nonzero color vector as "on" and zero as "off." Alternatively, I could convert to grayscale, varying the brightness according to value (in the HSV sense of the word).

Yeah, this is something where we'd need to figure something out on the Kaleidoscope side to properly support. Until then, in worst case, you can make a keyboard-specific temporary API to control the LEDs. That won't be compatible with our existing LED modes, but most of those make no sense on monochrome leds anyway.

  1. I'm using the "Blue Pill," which has an ARM Cortex-M3 based microcontroller, not AVR. But it appears the vast majority of Kaleidoscope is MCU agnostic. It's not clear to me where the dependencies are that make Kaleidoscope for AVR MCUs.

It will be clearer once keyboardio/Kaleidoscope#695 is merged, but the primary AVR dependencies are - currently - Hardware::attachToHost, Hardware::detachFromHost, all the EEPROM stuff, and a few plugins (like FirmwareDump, HostPowerManagement, and USBQuirks). Pretty much everything else should be MCU agnostic, indeed.

We have an out-of-tree hardware plugin, for the Dygma Raise at Dygmalab/Kaleidoscope-Hardware-Raise, which uses an ARM MCU (SAMD21) as well. A large part of keyboardio/Kaleidoscope#695 is aimed at being able to better support ARM.

  1. You guys seem to be on the verge of a dramatic hardware API change, and it's not clear when and on which branch I should write a new hardware implementation.

For the short term, master would be fine, but as you're using an ARM MCU, the hardware/new-api branch is probably a better choice, even though it's a bit in flux until it gets merged. Nevertheless, once it lands, whatever branch or state you based your implementation on, I'll happily help port it to whatever we ended up with.

  1. How do I set up the build environment? This is very mysterious. Most of the build scripts appear to assume the device is the Model 01. There are bits and pieces of build-related stuff all over the place. It's not clear how everything fits together.

The trick is in kaleidoscope-builder, which is an underdocumented, fragile piece of ancient shell script (sorry, this one's on me!). You can look at the examples under examples/Devices, where the non-Model01 devices set up stuff for themselves. Namely, their .kaleidoscope-builder.conf: you'll need to set up BOARD to be able to compile the sketch, and flash and flash_over_usb to be able to flash via Kaleidoscope's tooling.

This is a part of the firmware I want to improve in the not too distant future, but we didn't get there yet.

As for setting it up from scrach... that's not super-involved, you basically clone this repo to ~/Arduino/hardware/keyboardio, and make changes there. Or clone it anywhere, and tell Arduino about how to find it. Eg, you can set BOARD_HARDWARE_PATH in ~/.kaleidoscope-builder.conf to wherever you cloned the repo to (it will still need to be called "keyboardio", symlink works), and the builder will pass the appropriate parameters to Arduino tools.

The code base itself is pretty easy to understand, so I could just copy the Kaleidoscope code that's relevant to my project, make all the changes I want, and be done with it. But I think my code would be more useful if myself and other people could use it as part of a feature rich, actively maintained framework. And there may be some value to potential future developers in having an example of a device implementation for which the off-the-shelf classes can't be used.

<3

Do you have any recommendations?

If you can share what code you came up with so far, I'm happy to help in porting it to the new APIs in keyboardio/Kaleidoscope#695, and help patch up the missing bits and pieces.

You can also hop on our Discord, where we're happy to answer questions too.

rljacobson commented 4 years ago

Awesome, you told me everything I wanted to hear!

I suppose you were looking at ATMegaScanner, and things within the Kaleidoscope repo mostly? KeyboardioScanner is what the Model01 uses.

Yes, KeyboardioScanner.h isn’t in the Kaleidoscope repo.

That won't be compatible with our existing LED modes, but most of those make no sense on monochrome leds anyway.

Right, that’s true. Do you have a separate mechanism for controlling indicator lights like caps lock? That’s the most natural place for these LEDs—they literally are indicator lights, but they’re on every key above the midline, so they could be used in other ways, too. If there isn’t already a separate mechanism for indicator lights, I should probably write a plugin implementing it.

rows*columns doesn't have to be equal to the number of keys - we already have a number of keyboards in the tree which have less keys than what their matrix would suggest: Splitography, ErgoDox, Atreus and Atreus2.

Ah, yes, but consider this code in MatrixAddr.h:

static constexpr uint8_t upper_limit = rows__ * cols__;

static constexpr uint8_t invalid_state = 255;

static_assert(rows * cols < 255,
              "The number of rows and columns provided to instantiate \n"
              "MatrixAddr<rows, cols> exceeds the supported total number \n"
              "of 255 keys");

The static assert already fails, as 8*32 = 256 > 255. Then there’s the upper_limit, which is one past the last address (since addresses are zero based). In my case, the last possible address (7, 31) is offset = 255, but invalid_state = 255. In fact, upper_limit is of type uint8_t, so actually setting upper_limit = rows*cols wraps around to 0 when 256 is coerced back to uint8_t, i.e. upperlimit = 0. Which means MatrixAddr.isValid() always returns false. There’s more bad news:

// Range is a helper class that is used in range based for loops
// over all possible KeyAddr values.
//
struct Range {
  typedef ThisType Iterator;
  static constexpr ThisType begin() {
    return ThisType(uint8_t(0));
  }
  static constexpr ThisType end() {
    return ThisType(ThisType::upper_limit);
  }
};

Since upper_limit = 0 in my case, we always have ThisType.begin()==ThisType.end(), and any range-based for loops are always skipped. Surprisingly I think we’re ok if we have a loop that starts later than iter = ThisType.begin() and goes until iter == ThisType.end(), because iter eventually wraps around to 0 == ThisType.end().† But, in this case we would still iterate over the invalid_state, and I’m not sure if that’s ok.

I’m not sure, though. According to the ISO standard, there are no integral operators on anything smaller than an integer, so the uint8_ts are promoted to ints before they are incremented, and also before they are compared. Does the standard require that the intermediate result be narrowed back to uint8_t?

The default constructor initializes MatrixAddr to invalid_state, which I think is only used to signal UnknownKeyswitchLocation. On the Tardis keyboard, 255 is a valid address, but since it is not populated by a keyswitch, that’s probably ok… ?

You can still have MatrixAddr representable as a single byte even for the case(s) of rows*cols=256, I think. The fix is to have upper_limit = min(rows*cols, 255). This would also accommodate devices with weird row x column schemes. For example, the LED matrix on the Tardis keyboard has 40 columns and 20 rows, but it only has 148 physical LEDs, not 40 x 20 = 800. (In my case, there’s no way around using a lookup table to convert logical addresses to pin numbers since the pin numbers are all over the place.) The downside is, you don’t have a static mechanism to enforce the fact that there cannot be 256 keys. Some alternative fixes:

the primary AVR dependencies are - currently - Hardware::attachToHost, Hardware::detachFromHost, all the EEPROM stuff, and a few plugins (like FirmwareDump, HostPowerManagement, and USBQuirks).

These are hurdles that can be overcome—especially if the Dygma Raise's repo has code that can be ported to your new hardware API.

If you can share what code you came up with so far, I'm happy to help in porting it to the new APIs

You are a saint. What I’ll do is check out that new branch and code up the hardware-specific stuff for the Tardis keyboard based on the code I see there. Once I have something solid, I’ll check back in with you and we can go from there. I am going to go ahead and open up a ticket for my Tardis implementation to track my progress if that’s ok with you. If you’d prefer to have that issue on my own fork for now, feel free to just close the issue I opened.

Thanks for being so helpful!

rljacobson commented 4 years ago

I forgot to mention PlatformIO (docs). It strikes me as a great (perhaps partial) replacement for your current build system.

This is cross platform code builder and library manager with platforms like Arduino or MBED support. They took care of toolchains, debuggers, frameworks that work on most popular platforms like Windows, Mac and Linux. It supports more than 200 development boards along with more than 15 development platforms and 10 frameworks. So most of popular boards are covered. They’ve done hard work in organizing and managing hundreds of libraries that can be included in to your project. Also lots of examples allow you to start developing quickly.

noseglasses commented 4 years ago

As its author I would like to add that MatrixAddr is just a template class that satisfies an (not very well documented) interface and works for most keyboards. @rljacobson, you could easily specialize it for your needs or even write your own non-templated KeyAddr class and use it with the soon to be merged new Kaleidoscope hardware-API that @algernon is currently working on.

If you find a way to fix the given MatrixAddr in a way that it also works for your (very interesting) keyboard without breaking anything or adding unnecessary overhead, this would be cool and PRs are most appreciated.

And even if that's not possible and you choose to implement a specialization of MatrixAddr, you are very welcome to submit a PR and we could give the default MatrixAddr template one ore several control template parameters (bools/ints) with default values to make both, the default version and your specialization available without breaking existing code.

rljacobson commented 4 years ago

If you find a way to fix the given MatrixAddr in a way that it also works for your (very interesting) keyboard without breaking anything or adding unnecessary overhead, this would be cool and PRs are most appreciated.

I think this should be possible. I'll revisit this issue when I get to that point in my implementation. :)

obra commented 4 years ago

I'm closing this for now, as it's more of a discussion than a specific bug in the bundle. Totally fine if you reopen it as you get further.