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

refactor(shields): MurphPad #1786

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

lesshonor
Copy link
Contributor

@lesshonor lesshonor commented Apr 27, 2023

  • Added matrix transforms and physical layouts for three configurations. This is a numpad, but notably the first three positions of column 0 are actually tact switches on the back of the PCB. I believe the new transforms make this clearer (the keymap appearance matches QMK Configurator now, too)
  • Tweaked default keymap:
    • more closely resembles the QMK Configurator default map (reduces confusion for users who are new to ZMK; also generally less niche)
    • more useful OOTB for ZMK (immediate access to output selection, easier BT profile switching).
  • Cleaned up formatting a bit.
  • Updated encoder configuration to current ZMK standards.
  • Avoid forcing RGB on for nice_nano.
  • Corrected the number of default LEDs. While some users may opt for less or more, there are exactly eight footprints on the PCB for 5050 SMD LEDs.

Board/Shield Check-list

  • This board/shield is tested working on real hardware
  • General consistent formatting of DeviceTree files
  • Keymaps do not use deprecated key defines (Check using the upgrader tool)

Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

I had noticed the discrepancies when comparing vs. the configurator, looks good to me.

@caksoylar caksoylar added shields PRs and issues related to shields refactor labels Jun 10, 2023
@lesshonor lesshonor force-pushed the murphpad-refactor branch 2 times, most recently from b5e8868 to abda644 Compare October 20, 2023 02:38
@lesshonor lesshonor changed the title refactor: murphpad refactor(shields): MurphPad Oct 20, 2023
@lesshonor lesshonor force-pushed the murphpad-refactor branch 4 times, most recently from b5b4fa0 to e173060 Compare January 9, 2024 14:21
@lesshonor lesshonor force-pushed the murphpad-refactor branch 2 times, most recently from d8b0e5e to 639daad Compare February 11, 2024 15:13
@lesshonor lesshonor force-pushed the murphpad-refactor branch from 639daad to 5794c6f Compare March 1, 2024 01:11
@lesshonor lesshonor force-pushed the murphpad-refactor branch from 5794c6f to a8ee00c Compare April 7, 2024 14:19
@lesshonor lesshonor force-pushed the murphpad-refactor branch from a8ee00c to a0706f8 Compare July 7, 2024 04:56
@lesshonor lesshonor force-pushed the murphpad-refactor branch 2 times, most recently from 571b5ce to 6982f96 Compare August 5, 2024 06:55
* Added matrix transforms and physical layouts for three typical
  configurations.

* Tweaked default keymap to be more immediately useful for ZMK and more
  closely resemble the default keymaps of non-ZMK firmware.

* Board-specific Kconfig settings migrated to main shield file, so they
  will be exposed to end-users through the setup script.

* Tidied formatting and shortened overlong layer labels.

* Aligned encoder configuration with current standards.

* Corrected default number of RGB LEDs.

* Enabled OLED by default in a less authoritarian way.
@lesshonor lesshonor mentioned this pull request Oct 6, 2024
45 tasks
@Nick-Munnich
Copy link
Contributor

Looking at this the only point I want to raise is that it has OLED defaulting to y. Imo OLED/RGB/Display etc. should always be opt-in, rather than opt-out.

@lesshonor
Copy link
Contributor Author

lesshonor commented Oct 6, 2024

Looking at this the only point I want to raise is that it has OLED defaulting to y. Imo OLED/RGB/Display etc. should always be opt-in, rather than opt-out.

I would agree with you on most shields. On this one: yes, you can opt out of the display, but the plate will have an unsightly hole in it unless you modify the plate file and get that alternate version cut. I've probably assisted with some fifty MurphPad builds in one way or another; plenty didn't bother with RGB or encoders, but no one has ever not installed the display...even if assembly errors meant they couldn't get it working.
...That said, I still wanted to facilitate the option just in case—hence "enabling the OLED in a less-authoritarian way".
I'd do the same for the encoder if that was actually possible.

@Nick-Munnich
Copy link
Contributor

Nick-Munnich commented Oct 6, 2024

I would agree with you on most shields. On this one: yes, you can opt out of the display, but the plate will have an unsightly hole in it unless you modify the plate file and get that alternate version cut. I've probably assisted with some fifty MurphPad builds in one way or another; plenty didn't bother with RGB or encoders, but no one has ever not installed the display, even if they couldn't get it working. ...That said, I still wanted to facilitate the option just in case—hence "enabling the OLED in a less-authoritarian way". I'd do the same for the encoder if that was actually possible.

That's a fair argument. I think I'd still prefer explicit for consistency with other devices, though. We need to improve our related documentation on it too. I didn't actually notice that the encoder stuff wasn't commented out, I'd obviously be in favor of doing that too.

All that said I am perfectly happy to be overruled in this case.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

LGTM!

@petejohanson petejohanson merged commit 7dfc6ab into zmkfirmware:main Oct 17, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor shields PRs and issues related to shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants