Excalibur Support #117

doughsay · 2020-08-22T20:43:45Z

🎉 ⚔️

Support for other targets, specifically my custom build, Excalibur 🗡️ 😄

This isn't necessarily intended to be the way we do things long term; having one giant repo with all models we will ever support is not the way I want to go. But until we figure out a better pattern, I present here what I had to do in order to support BOTH the Excalibur AND the Keybow at the same time without too much disruption.

Once we have the development/prototyping board, we will likely have to do something similar.

amclain · 2020-08-24T20:15:04Z

I present here what I had to do in order to support BOTH the Excalibur AND the Keybow at the same time without too much disruption.

The big question I have right now is should Xebow officially support Excalibur at this time?

The Keybow's advantage is that it's available off the shelf so people can easily develop and test Xebow on it. The Dev-68 development keyboard we're making is an open hardware design that anyone can order the parts for and build, and several of our core members will have one for development and testing. Excalibur seems to me like it's a prototype that led to Xebow, but that doesn't have all the features at the hardware level that we want (RGB LEDs, the desired scan matrix method, etc.), and only one of them exists in the world for testing Xebow. So my concern with adding Excalibur support to Xebow at this time is that we would be adding code that's instantly "legacy" code. We would need to make sure this code stays compatible with Excalibur as we're building out features, and that could lead us to undesirable design compromises and/or slow us down.

My recommendation is to support the Dev-68 as our next keyboard, so that our two officially supported keyboards are the Keybow and Dev-68. We can use this opportunity to figure out the overlap between the two and experiment with finding the right abstractions. We may also be able to find an architecture we like that inverts the dependency issue this PR introduces that QMK also suffers from (the library containing code for specific keyboards). When we work out a model where a specific keyboard can use Xebow as a dependency and specify its own configuration (like its layout), that may be a good time for Excalibur to support Xebow in my opinion.

doughsay · 2020-08-24T23:35:52Z

I understand and agree with all your points. I just have one counterpoint, which (for me at least) counteracts it all: I have this keyboard and am using it as my main keyboard now, and would like to continue to do so for the time being AND stay up-to-date.

Yes, this is not long term. But can we not just at least have it for the short term? The changes I made here are the exact same that would be needed to support the dev-68; I'm just getting a tiny head-start.

We would need to make sure this code stays compatible with Excalibur as we're building out features, and that could lead us to undesirable design compromises and/or slow us down.

This is precisely my intent: put an alternate layout in the code-base now so that we don't inadvertently do things that make supporting them harder in the future. (for example, the UI CSS hardcodes the keybow width for the outline around the keys. Everything else works "ok" except that. This should be fixed, not just for excalibur but for dev-68 as well.)

As for the immediately legacy code: it's only anything in the excalibur/ folder. Just ignore that if you want and let me maintain it. Look at just the rest of the changes and tell me if you disagree with any of them (again, thinking for the short-term and our intent to support the dev-68 soon-ish)

If you still REALLY don't want this merged, then I will just maintain this as a branch/fork; It'll just be annoying dealing with merge-conflicts constantly...

amclain · 2020-08-25T00:20:52Z

I just have one counterpoint, which (for me at least) counteracts it all: I have this keyboard and am using it as my main keyboard now, and would like to continue to do so for the time being AND stay up-to-date.

I understand why you want Excalibur supported, but in a sense it sounds like asking the community to maintain your personal keyboard. 😉 (If you bribe the other contributors with Excalibur keyboards, you may change my mind.)

As for the immediately legacy code: it's only anything in the excalibur/ folder. Just ignore that if you want and let me maintain it.

That's one of the big reasons I would like to avoid having the Excalibur code in the codebase at this point. I don't feel like it's responsible as a developer to ignore part of the codebase that's affected by a change I'm making. If the Excalibur code lands in the codebase, I'm personally obligated to maintain it. (It also shouldn't be possible to ignore these types of changes as our test coverage improves, as the tests will break.)

If you still REALLY don't want this merged, then I will just maintain this as a branch/fork

A branch/fork would be my preference for now, but I definitely don't need to be the only voice on this issue. If other folks feel like merging in the code, or you feel like it really needs to be merged in, do it. I'm just voicing my opinion so the trade-offs can be considered.

doughsay · 2020-08-25T00:24:32Z

I want the changes OTHER than the changes in the excalibur/ folder reviewed and possibly merged. Can you at least weigh in on that?

amclain · 2020-08-25T18:51:02Z

Sorry, I was just low on bandwidth. Let me do a review...

amclain

I'm not going to block this PR, but I've jotted down some of my thoughts.

amclain · 2020-08-25T18:56:28Z

config/excalibur/config.exs

@@ -0,0 +1,101 @@
+import Config


ℹ️ (optional)

I didn't notice any Excalibur-specific config in here. If that's the case, should we move the shared target config to a place like /config/all_targets.exs?

(I was originally thinking /config/target/config.exs, but in practice I don't think the other targets are going to nest well like that based on how mix works.)

Yes, this file is identical and the duplication is a huge smell. I think I like the "all_targets" idea, I'll try it out and see what I think. That being said, I wanted to change the hostname in this config, but I'm not sure it's the right place to do so. But generally, the fact that all "xebow powered devices" use a hardcoded common hostname is something we need to solve. i.e. I want to be able to plug in the keybow, the excalibur, and the dev-68 at the same time and not have a bunch of hostname conflicts.

the fact that all "xebow powered devices" use a hardcoded common hostname is something we need to solve

Oh! We can set a serial number that's used in the hostname:

The Nerves project generator configures mdns_lite to advertise two hostnames: nerves.local and nerves-1234.local. The latter one is based on the serial number of the device. If you only have one Nerves device on the network, use nerves.local. But, if you have many devices, figure out each hostname from the device's serial number, either by using a mDNS discovery program or by logging into a device via a serial console and typing hostname at the IEx prompt.

https://github.com/nerves-project/nerves_pack#erlang-distribution

amclain · 2020-08-25T19:06:52Z

lib/rgb_matrix/animation/pinwheel.ex

@@ -22,6 +22,8 @@ defmodule RGBMatrix.Animation.Pinwheel do
 %State{tick: 0, speed: 100, leds: leds, center: determine_center(leds)}
 end

+ defp determine_center([]), do: %{x: 0, y: 0}


⚠️ (recommended)

This points out an interesting issue. I assume you did this because the app crashed due to Excalibur not having an RGB matrix. It sounds like the Engine is still running as well even though there are no LEDs to animate. Rather than pass an empty matrix, should Excalibur disable the rendering engine and Xebow skip creating animations? Maybe a config.exs option that sets this?

(Update: I may have noticed later that Engine is not running because Excalibur has a different list of children it starts up.)

No, your initial assumption was right. I do still launch the engine even though the excalibur layout has no LEDs. In general, we DO want to support layouts that don't have LEDs if we want to be a serious alternative to QMK. The problem is that downstream genservers (Xebow itself, and the liveview UI) expect the engine to be there to register with it. The easiest way forward was to just let it run.

amclain · 2020-08-25T19:13:28Z

lib/xebow.ex

+ def layout(:host), do: @layout_host
+ def layout(:keybow), do: Keybow.Layout.layout()
+ def layout(:excalibur), do: Excalibur.Layout.layout()


ℹ️ (optional)

I'm concerned about this code leading to the same issues QMK has with the library knowing too much about each keyboard's implementation. I know this is in your mind too, so I'm just pointing out this code as a reference.

🤔 What if the layout was set in the config.exs of each target so that Xebow doesn't have to have knowledge of all the target layouts?

config :xebow, layout: Keybow.Layout

Yeah, you're right, I don't want this long-term, this just seemed like the easiest option to limit the disruption to the codebase.

I was trying to think through what a good long-term structure would be for multiple targets, and it seems like a generally hard problem. Do we want each other target firmware to be its own project that just uses xebow libraries? Do we want xebow itself to be the project and other target repos just contain loose files that are added to xebow? Should we use git submodules in some way? Git subtree?

For now, I like the idea of just moving this to the config like you suggest; let's see how far that gets us.

amclain · 2020-08-25T20:40:08Z

lib/xebow/application.ex

@@ -58,14 +58,21 @@ defmodule Xebow.Application do
 ]
 end

- def children(_target) do
+ def children(:keybow) do


ℹ️ (optional)

Similar to a previous comment, would it simplify the code to specify the children for each target in the target's config.exs?

amclain · 2020-08-25T20:41:50Z

lib/xebow/excalibur/keyboard.ex

@@ -0,0 +1,284 @@
+require Logger


⚠️ (recommended)

Is this supposed to be outside the module definition? Usually I've seen Logger required inside the module.

I was writing up a big explanation of why I do this, and decided to check myself, and cannot find any supporting evidence of my previous assumptions... 🤣

I will change this and stop doing this pattern.

I'm honestly surprised it works this way. I thought require enabled macros in the required module to work in the receiving module. Now this gives me something to research. 😁

require is lexically scoped, just like alias and import. I tried it in an iex shell and was also surprised. So in this case, the require would be valid for all modules defined in this file. If it were inside the module and there were a second module, then the second module would not have access to the macros.

As I said, the point is moot because we don't put multiple (top-level) modules in files by convention.

amclain · 2020-08-25T20:57:38Z

lib/xebow/excalibur/keyboard.ex

+ defp open_input_pin!(pin_number) do
+ # clear any stuck pins
+ {:ok, pin} = GPIO.open(pin_number, :output, initial_value: 0)
+ :ok = GPIO.write(pin, 0)


⚠️ (recommended)

Forcing a write to an input pin to get the desired state doesn't seem like a good idea to me. Setting the pull-down mode as you've done below seems like the right way to do this.

I believe I saw similar code about clearing stuck pins when doing a Keybow refactor. IIRC we started the scan timer after initializing the pins, essentially throwing away the initial pin state so it wouldn't cause issues.

Another thing this code makes me wonder though: In the boot to BIOS scenario, wouldn't the user be holding down a key on startup, making the key look "stuck" (AFK issue IIRC)?

Is that how you get into BIOS? I tap the key repeatedly, I never hold... does it work if you just hold? 🤯

Besides, I wanted everything in the excalibur/ folder sort of ignored, cause, as you say, I'm the only one with one so it really doesn't matter what kind of hackery is going on in here. I was trying to save you time by suggesting you NOT review this terrible stuff in here...

I was trying to save you time by suggesting you NOT review this terrible stuff in here...

I like thorough reviews. 😜

Seriously though, that's fine, just skip this recommendation for now.

amclain · 2020-08-25T21:16:25Z

test/xebow_test.exs

+
+ test "has a layout for excalibur" do
+ assert %Layout{} = Xebow.layout(:excalibur)
+ end


ℹ️ (optional)

I feel like these tests are more-so formalizing an architectural decision that we admittedly don't really like, rather than testing functionality we want to avoid breaking. Honestly, I think we could remove them.

yes, I was just extrapolating the pattern

amclain · 2020-08-25T21:19:57Z

lib/xebow/excalibur/keyboard.ex

@@ -0,0 +1,284 @@
+require Logger
+
+defmodule Xebow.Excalibur.Keyboard do


ℹ️ (optional)

If this were in the namespace Excalibur.Keyboard, and lib/excalibur, would that help decouple it from the Xebow core code?

Yeah, that might be a good idea...

doughsay · 2020-08-25T23:41:08Z

@amclain thanks for the review! I think there's definitely some cleanup needed here, but I do still like the idea of merging in at least the idea of a decoupled multi-target architecture. I will address some of the feedback in here to hopefully get it to a point where you think it's ok to merge.

doughsay added 7 commits April 28, 2020 17:57

WIP

70fce9c

Merge branch 'master' into excalibur

9ce0e73

Support excalibur as a target

f129ac9

Cleanup layouts

8f45200

Full excalibur layout and keymap

4072353

Make credo happy

3fe45c2

Merge branch 'master' into excalibur

d3c10ae

amclain assigned doughsay Aug 25, 2020

amclain approved these changes Aug 25, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excalibur Support #117

Excalibur Support #117

doughsay commented Aug 22, 2020 •

edited

Loading

amclain commented Aug 24, 2020

doughsay commented Aug 24, 2020

amclain commented Aug 25, 2020

doughsay commented Aug 25, 2020 •

edited

Loading

amclain commented Aug 25, 2020

amclain left a comment

amclain Aug 25, 2020

doughsay Aug 25, 2020

amclain Aug 26, 2020 •

edited

Loading

amclain Aug 25, 2020 •

edited

Loading

doughsay Aug 25, 2020 •

edited

Loading

amclain Aug 25, 2020

amclain Aug 25, 2020

doughsay Aug 25, 2020

amclain Aug 25, 2020

amclain Aug 25, 2020

doughsay Aug 25, 2020

amclain Aug 26, 2020

doughsay Aug 26, 2020

amclain Aug 25, 2020

doughsay Aug 25, 2020

doughsay Aug 25, 2020

amclain Aug 26, 2020

amclain Aug 25, 2020 •

edited

Loading

doughsay Aug 25, 2020

amclain Aug 25, 2020

doughsay Aug 25, 2020

doughsay commented Aug 25, 2020

		@@ -0,0 +1,284 @@
		require Logger

		defmodule Xebow.Excalibur.Keyboard do

Excalibur Support #117

Are you sure you want to change the base?

Excalibur Support #117

Conversation

doughsay commented Aug 22, 2020 • edited Loading

amclain commented Aug 24, 2020

doughsay commented Aug 24, 2020

amclain commented Aug 25, 2020

doughsay commented Aug 25, 2020 • edited Loading

amclain commented Aug 25, 2020

amclain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amclain Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

amclain Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

doughsay Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amclain Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doughsay commented Aug 25, 2020

doughsay commented Aug 22, 2020 •

edited

Loading

doughsay commented Aug 25, 2020 •

edited

Loading

amclain Aug 26, 2020 •

edited

Loading

amclain Aug 25, 2020 •

edited

Loading

doughsay Aug 25, 2020 •

edited

Loading

amclain Aug 25, 2020 •

edited

Loading