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

Add support for splitkb.com keyboards #627

Closed
wants to merge 1 commit into from

Conversation

leah-splitkb
Copy link
Contributor

This PR adds first-party support for the following splitkb.com keyboards:

  • Aurora Corne
  • Aurora Helix (includes keeb definition, already in upstream QMK too)
  • Aurora Lily58
  • Aurora Sofle V2
  • Aurora Sweep
  • Kyria Rev3

The vial keymap is intended for primary use, with a converter to one of the various RP2040 controllers. vial-minimal is provided for minimal backwards compatibility with atmega32u4 controllers. All keymaps use the same feature set in order to provide a consistent user experience.

This also closes #589.

@azthec
Copy link

azthec commented Jan 2, 2024

I've checked out this branch to build and flash for my Aurora Sweep and for vial-minimal it worked flawlessly, for vial I did get the following failed static assert.

~/Programs/vial-qmk   splitkb-vial  make splitkb/aurora/sweep:vial
QMK Firmware 0.22.2
Making splitkb/aurora/sweep/rev1 with keymap vial
Compiling: quantum/via.c                                                                            [OK]
Compiling: quantum/dynamic_keymap.c                                                                quantum/dynamic_keymap.c:123:1: error: static assertion failed: "Dynamic keymaps are configured to use more EEPROM than is available."
 _Static_assert(DYNAMIC_KEYMAP_EEPROM_MAX_ADDR >= DYNAMIC_KEYMAP_MACRO_EEPROM_ADDR + 100, "Dynamic keymaps are configured to use more EEPROM than is available.");
 ^~~~~~~~~~~~~~
 [ERRORS]
make[1]: *** [.build/obj_splitkb_aurora_sweep_rev1_vial/quantum/dynamic_keymap.o] Error 1
make: *** [splitkb/aurora/sweep:vial] Error 1

@thomasbaart
Copy link
Contributor

If there’s anything we should do to move this along, please do let me know 😌

@lesshonor
Copy link
Contributor

If there’s anything we should do to move this along, please do let me know 😌

  1. The PR has conflicts that need to be resolved.
  2. The vial-minimal keymaps are a faux pas, as the PR template requests that you please stick to one keymap named vial for maintainability. As it currently stands, vial-minimal will not be picked up and validated by CI.

@thomasbaart
Copy link
Contributor

thomasbaart commented Jan 10, 2024

Thank you for the feedback @lesshonor!

Could you please let me know if the following would be acceptable, before we spend more time on it?

We added the vial-minimal keymaps in order to have a compatible keymap for ATmega32u4 chips, but we don't sell those in our shop and don't necessarily want to support them. We can remove the vial-minimal keymaps.

I propose we add a converter flag to each vial keymap's rules.mk file, such as CONVERT_TO=rp2040_ce, for a default target of RP2040. We could further add a note to each keymaps' README that it's only compatible with RP2040 controllers due to size constraints, by design, and that a different converter may be used when using a non-RP2040-ce compatible controller.

Further, we'd overwrite the current Kyria Vial keymap with our own, for consistency within our entire lineup. That would solve the conflicts.

Of note, the target controller elite_c is used for backwards compatibility, which is used in tandem with QMK's CONVERTER functionality. The target for the newest revision boards are RP2040-based controllers.

@lesshonor
Copy link
Contributor

lesshonor commented Jan 10, 2024

Ultimately, I'm not the decision maker here; I don't have merge privileges. I just propose the occasional change and review PRs to point out:

  1. problems that would prevent a merge full stop (in this case, the git conflicts, which are not actually being caused by the Kyria keymap)
  2. when things don't follow the current guidelines for repo submission (in this case, the additional keymaps)
  3. things that will likely cause headaches later for the sole maintainer (AFAIK) of this repo, xyz.

If you're asking me, personally, what I would do:

  • I'd use conditionals in the C code and makefiles to selectively disable features on AVR, so any MCU can be built with a single keymap named "vial" and users with ARM boards don't have to compromise. Submission guidelines aside, users will immediately disregard the keymap name that comes out and says "this has fewer features", not realizing their dusty old atmega32u4 must use it, and if they're downloading off a third-party site that auto-builds firmware from this repo they won't see any READMEs explaining anything.
  • ...but you and/or Leah have both already ignored/disregarded this suggestion, so OK. I'd keep both of the Vial keymaps you've created in the splitkb repo, and request the splitkb keymaps in this repo be deleted to prevent confusion. "I already offer vendor-tested/approved pre-compiled Vial builds with freely-available source code [insert link]". Now you have full control and can explain the differences between "vial" and "vial-minimal" right next to the binary downloads, and if somebody wants to roll their own they can still jump through those hoops.
  • If I only wanted to support RP2040 Vial builds as a vendor, I would provide those on my website and say something like "We don't provide atmega32u4 builds for Vial because far too many features have to be disabled on this weaker hardware—consider upgrading to a Liatris today! If you must use Vial with this MCU despite SplitKB's recommendations, you may be able to find source code created by other customers in places like the Vial-QMK repo."

@thomasbaart
Copy link
Contributor

thomasbaart commented Jan 10, 2024

Thank you very much for the elaborate feedback.

We didn’t set out to ignore the advice, but were rather faced with a decision:

We ultimately arrived at the two keymaps to still fit some functionality in the AVR version. I recall we had trouble using feature flags because we wanted to use RGBLIGHT for space savings on AVR, but RGB matrix on RP2040, and those configurations had trouble coexisting with the new JSON format.

We’ll come up with something and will post an update here by then. Thanks again!

@leah-splitkb
Copy link
Contributor Author

I'd use conditionals in the C code and makefiles to selectively disable features on AVR, so any MCU can be built with a single keymap named "vial" and users with ARM boards don't have to compromise.

To clarify, the issue is that hardware-heavy split keebs are way oversized when running Vial on AVR: bare QMK is already quite close to the limit, and our initial Vial build was 8800 bytes over. We tried quite hard to create a single keymap which used conditionals, but this was simply impossible without disabling hardware features. As-is the AVR build has less than 500 bytes free, and the only way that was possible was by disabling a significant number of non-essential typing features.

The reason it has to be different keymaps is that you can't conditionally switch between vialrgb and qmk_rgblight in vial.json. Using a single keymap would mean that either the RP2040 build wouldn't have per-key rgb, or the AVR build would be way over budget. Either alternative is obviously not really an option.

@xyzz xyzz closed this Jan 21, 2024
@xyzz xyzz reopened this Jan 21, 2024
@thomasbaart
Copy link
Contributor

We've decided it's likely easiest for everyone for us not to push the keymaps into this repository. Instead, we're keeping it public over on our fork, and host the resulting files on our firmware.splitkb.com website.

Thank you very much for the guidance and support along the way, and keep up keeping up! I've asked before, but if there's a way to donate to this project, please let me know and I'll happily contribute that way.

@leah-splitkb leah-splitkb deleted the splitkb-vial branch January 24, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants