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

Excalibur Support #117

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions config/excalibur/config.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import Config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ (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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@amclain amclain Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


config :nerves, rpi_v2_ack: true

# Use shoehorn to start the main application. See the shoehorn
# docs for separating out critical OTP applications such as those
# involved with firmware updates.

config :shoehorn,
init: [:nerves_runtime, :nerves_pack],
app: Mix.Project.config()[:app]

# Nerves Runtime can enumerate hardware devices and send notifications via
# SystemRegistry. This slows down startup and not many programs make use of
# this feature.

config :nerves_runtime, :kernel, use_system_registry: false

# Authorize the device to receive firmware using your public key.
# See https://hexdocs.pm/nerves_firmware_ssh/readme.html for more information
# on configuring nerves_firmware_ssh.

keys =
[
System.get_env("NERVES_SSH_PUB_KEY", ""),
Path.join([System.user_home!(), ".ssh", "id_rsa.pub"]),
Path.join([System.user_home!(), ".ssh", "id_ecdsa.pub"]),
Path.join([System.user_home!(), ".ssh", "id_ed25519.pub"])
]
|> Enum.filter(&File.exists?/1)

if keys == [],
do:
IO.write(:stderr, """
Warning: No SSH public key found. You will not be able to remotely
connect to the device to access the iex shell or updating firmware.

If you have an SSH public key, you can set NERVES_SSH_PUB_KEY to
the path of the key. Otherwise, you can still flash the firmware
manually.
""")

config :nerves_firmware_ssh,
authorized_keys: Enum.map(keys, &File.read!/1)

# Configure the network using vintage_net
# See https://github.com/nerves-networking/vintage_net for more information
config :vintage_net,
regulatory_domain: "US",
config: [
{"bond0", %{type: VintageNetDirect}}
]

config :mdns_lite,
# The `host` key specifies what hostnames mdns_lite advertises. `:hostname`
# advertises the device's hostname.local. For the official Nerves systems, this
# is "nerves-<4 digit serial#>.local". mdns_lite also advertises
# "nerves.local" for convenience. If more than one Nerves device is on the
# network, delete "nerves" from the list.

host: [:hostname, "xebow"],
ttl: 120,

# Advertise the following services over mDNS.
services: [
%{
name: "SSH Remote Login Protocol",
protocol: "ssh",
transport: "tcp",
port: 22
},
%{
name: "Secure File Transfer Protocol over SSH",
protocol: "sftp-ssh",
transport: "tcp",
port: 22
},
%{
name: "Erlang Port Mapper Daemon",
protocol: "epmd",
transport: "tcp",
port: 4369
}
]

# Use Ringlogger as the logger backend and remove :console.
# See https://hexdocs.pm/ring_logger/readme.html for more information on
# configuring ring_logger.

config :logger, backends: [RingLogger]

# Phoenix config:
# Configures the endpoint
config :xebow, XebowWeb.Endpoint,
http: [port: 80, ip: {0, 0, 0, 0}],
url: [host: "xebow.local", port: 80],
code_reloader: false

config :xebow, settings_path: "/root/settings"

import_config "#{Mix.env()}.exs"
1 change: 1 addition & 0 deletions config/excalibur/dev.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import Config
2 changes: 2 additions & 0 deletions lib/rgb_matrix/animation/pinwheel.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Member

@amclain amclain Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ (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.)

Copy link
Member Author

@doughsay doughsay Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


defp determine_center(leds) do
{%{x: min_x}, %{x: max_x}} = Enum.min_max_by(leds, & &1.x)
{%{y: min_y}, %{y: max_y}} = Enum.min_max_by(leds, & &1.y)
Expand Down
48 changes: 11 additions & 37 deletions lib/xebow.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,46 +4,17 @@ defmodule Xebow do
Keybow kit.
"""

alias Layout.{Key, LED}
alias RGBMatrix.{Animation, Engine}
alias Xebow.Settings
alias Xebow.{Excalibur, Keybow, Settings}

require Logger

@leds [
LED.new(:l001, 0, 0),
LED.new(:l002, 1, 0),
LED.new(:l003, 2, 0),
LED.new(:l004, 0, 1),
LED.new(:l005, 1, 1),
LED.new(:l006, 2, 1),
LED.new(:l007, 0, 2),
LED.new(:l008, 1, 2),
LED.new(:l009, 2, 2),
LED.new(:l010, 0, 3),
LED.new(:l011, 1, 3),
LED.new(:l012, 2, 3)
]

@keys [
Key.new(:k001, 0, 0, led: :l001),
Key.new(:k002, 1, 0, led: :l002),
Key.new(:k003, 2, 0, led: :l003),
Key.new(:k004, 0, 1, led: :l004),
Key.new(:k005, 1, 1, led: :l005),
Key.new(:k006, 2, 1, led: :l006),
Key.new(:k007, 0, 2, led: :l007),
Key.new(:k008, 1, 2, led: :l008),
Key.new(:k009, 2, 2, led: :l009),
Key.new(:k010, 0, 3, led: :l010),
Key.new(:k011, 1, 3, led: :l011),
Key.new(:k012, 2, 3, led: :l012)
]

@layout Layout.new(@keys, @leds)

@spec layout() :: Layout.t()
def layout, do: @layout
@layout_host Layout.new([])

@spec layout(atom) :: Layout.t()
def layout(:host), do: @layout_host
def layout(:keybow), do: Keybow.Layout.layout()
def layout(:excalibur), do: Excalibur.Layout.layout()
Comment on lines +15 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ (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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ€” 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@type animations :: [Animation.t()]
@type animation_params :: %{String.t() => atom | number | String.t()}
Expand Down Expand Up @@ -238,7 +209,10 @@ defmodule Xebow do
end

defp initialize_animation(animation_type) do
Animation.new(animation_type, @leds)
target = Xebow.Application.target()
layout = layout(target)
leds = Layout.leds(layout)
Animation.new(animation_type, leds)
end

defp update_state_with_animation_types(state, animation_types) do
Expand Down
15 changes: 11 additions & 4 deletions lib/xebow/application.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ defmodule Xebow.Application do

alias Xebow.Settings

@leds Xebow.layout() |> Layout.leds()
@leds Xebow.layout(Mix.target()) |> Layout.leds()

if Mix.target() == :host do
defp maybe_validate_firmware,
Expand Down Expand Up @@ -58,14 +58,21 @@ defmodule Xebow.Application do
]
end

def children(_target) do
def children(:keybow) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ (optional)

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

[
# Children for all targets except host
# Starts a worker by calling: Xebow.Worker.start_link(arg)
# {Xebow.Worker, arg},
Xebow.HIDGadget,
Xebow.LEDs,
Xebow.Keyboard
Xebow.Keybow.LEDs,
Xebow.Keybow.Keyboard
]
end

def children(:excalibur) do
[
Xebow.HIDGadget,
Xebow.Excalibur.Keyboard
]
end

Expand Down
Loading