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 Keyhive Variant of Sofle RGB #1233

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

JWhitleyWork
Copy link

Board/Shield Check-list

  • This board/shield is tested working on real hardware
  • Definitions follow the general style of other shields/boards upstream (Reference)
  • .zmk.yml metadata file added
  • Proper Copyright + License headers added to applicable files (Generally, we stick to "The ZMK Contributors" for copyrights to help avoid churn when files get edited)
  • General consistent formatting of DeviceTree files
  • Keymaps do not use deprecated key defines (Check using the upgrader tool)
  • &pro_micro used in favor of &pro_micro_d/a if applicable
  • If split, no name added for the right/peripheral half
  • Kconfig.defconfig file correctly wraps all configuration in conditional on the shield symbol
  • .conf file has optional extra features commented out
  • Keyboard/PCB is part of a shipped group buy or is generally available in stock to purchase (OSH/personal projects without general availability should create a zmk-config repo instead)

Tested and working (currently typing on it) on my Keyhive variant of the Sofle RGB. Much borrowed from the QMK implementation for this keyboard.

@petejohanson
Copy link
Contributor

Tested and working (currently typing on it) on my Keyhive variant of the Sofle RGB. Much borrowed from the QMK implementation for this keyboard.

If you referenced the QMK code extensively to implement this, we can't really accept this PR, since the license is GPL there, and this is arguably a "derivative work" at that point.

@petejohanson
Copy link
Contributor

See https://zmk.dev/docs/development/clean-room for more details.

@JWhitleyWork
Copy link
Author

@petejohanson This may require some clarification. I specifically referenced only the file keyhive/qmk_firmware@a388b3e#diff-9134896d5f7bbd3575e3197e4752653dff4a420459009bca10ad94f09e909d57 which is part of a fork of QMK which was not contributed upstream, nor does it have the flowerbox at the top containing a reference to the GPL license. All other code in this PR was developed via examinig the board itself with a multimeter. If this is still an issue, I can contact the original developer and get written permission to re-license those few lines under MIT.

@petejohanson
Copy link
Contributor

@petejohanson This may require some clarification. I specifically referenced only the file keyhive/qmk_firmware@a388b3e#diff-9134896d5f7bbd3575e3197e4752653dff4a420459009bca10ad94f09e909d57 which is part of a fork of QMK which was not contributed upstream, nor does it have the flowerbox at the top containing a reference to the GPL license. All other code in this PR was developed via examinig the board itself with a multimeter. If this is still an issue, I can contact the original developer and get written permission to re-license those few lines under MIT.

Thanks for the clarification! It would be good to contact the keyhive developer to double check, since there may still be an assumption about GPL there.

@maximus5684
Copy link

@petejohanson I have received written confirmation from the original author to re-license the bit of code that I referenced under MIT. Please review and merge if satisfied.

@JWhitleyWork
Copy link
Author

Sorry, wrong account. Same person, though. Please continue.

Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Just want to confirm, this version of the sofle rgb is special to Keyhive? Could you mention how it's different from the normal sofle rgb?

Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Few more items, otherwise it looks good!

@Nicell Nicell added enhancement New feature or request shields PRs and issues related to shields labels May 6, 2022
Copy link

@researcherx00 researcherx00 left a comment

Choose a reason for hiding this comment

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

works well apart from LEDs

@Nicell Nicell self-requested a review June 29, 2022 20:14
@JWhitleyWork
Copy link
Author

This has been hanging out for over 9 months. Can someone review and/or merge it?

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.

Looks good to me. The comments I left are regarding copyright years that need updating and the deprecated Kconfig name replacement for central role.

@maximus5684
Copy link

All issues were addressed but it's been so long that the build process has changed. I would like to eventually get this merged but I'll work on fixing the build when I can.

Copy link
Contributor

@lesshonor lesshonor left a comment

Choose a reason for hiding this comment

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

The suggestions I'm proposing here should get the shield compiling again as of d7d9eed, assuming I haven't made any unfortunate typos.

Comment on lines +9 to +15
&spi1 {
compatible = "nordic,nrf-spim";
status = "okay";
mosi-pin = <6>;
// Unused pins, needed for SPI definition, but not used by the ws2812 driver itself.
sck-pin = <5>;
miso-pin = <7>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Migration to the pinctrl subsystem as part of the Zephyr 3.2 upgrade, and the &spi3 bus per #1746.

Suggested change
&spi1 {
compatible = "nordic,nrf-spim";
status = "okay";
mosi-pin = <6>;
// Unused pins, needed for SPI definition, but not used by the ws2812 driver itself.
sck-pin = <5>;
miso-pin = <7>;
&pinctrl {
spi3_default: spi3_default {
group1 {
psels = <NRF_PSEL(SPIM_MOSI, 0, 6)>;
};
};
spi3_sleep: spi3_sleep {
group1 {
psels = <NRF_PSEL(SPIM_MOSI, 0, 6)>;
low-power-enable;
};
};
};
&spi3 {
compatible = "nordic,nrf-spim";
status = "okay";
pinctrl-0 = <&spi3_default>;
pinctrl-1 = <&spi3_sleep>;
pinctrl-names = "default", "sleep";

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. For consistency, indents on this file should probably be four spaces.
  2. Please add a duplicate file named nice_nano.overlay, so just in case someone builds one of these with a nice!nano v1, the RGB will work just the same.

label = "LEFT_ENCODER";
a-gpios = <&pro_micro 21 (GPIO_ACTIVE_HIGH | GPIO_PULL_UP)>;
b-gpios = <&pro_micro 20 (GPIO_ACTIVE_HIGH | GPIO_PULL_UP)>;
resolution = <4>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resolution = <4>;
steps = <80>;

The change from resolution to steps is part of the major encoder refactor that occurred in June 2023.

80 may not be the correct value for your encoder(s)—you will need to test and verify—but you will need to update your shield accordingly for a chance at acceptance.

label = "RIGHT_ENCODER";
a-gpios = <&pro_micro 20 (GPIO_ACTIVE_HIGH | GPIO_PULL_UP)>;
b-gpios = <&pro_micro 21 (GPIO_ACTIVE_HIGH | GPIO_PULL_UP)>;
resolution = <4>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resolution = <4>;
steps = <80>;


sensors {
compatible = "zmk,keymap-sensors";
sensors = <&left_encoder &right_encoder>;
Copy link
Contributor

@lesshonor lesshonor Nov 16, 2023

Choose a reason for hiding this comment

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

Suggested change
sensors = <&left_encoder &right_encoder>;
sensors = <&left_encoder &right_encoder>;
triggers-per-rotation = <20>;

See previous comment about the June 2023 encoder refactor.

Comment on lines +34 to +44
config LVGL_VDB_SIZE
default 64

config LVGL_DPI
default 148

config LVGL_BITS_PER_PIXEL
default 1

choice LVGL_COLOR_DEPTH
default LVGL_COLOR_DEPTH_1
Copy link
Contributor

Choose a reason for hiding this comment

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

Kconfig changes necessitated by the Zephyr 3.2 upgrade.

Suggested change
config LVGL_VDB_SIZE
default 64
config LVGL_DPI
default 148
config LVGL_BITS_PER_PIXEL
default 1
choice LVGL_COLOR_DEPTH
default LVGL_COLOR_DEPTH_1
config LV_Z_VDB_SIZE
default 64
config LV_Z_DPI
default 148
config LV_Z_BITS_PER_PIXEL
default 1
choice LV_COLOR_DEPTH
default LV_COLOR_DEPTH_1

Comment on lines +36 to +37
label = "KSCAN";

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label = "KSCAN";

This label has been deprecated.

oled: ssd1306@3c {
compatible = "solomon,ssd1306fb";
reg = <0x3c>;
label = "DISPLAY";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label = "DISPLAY";

This label has been deprecated.


led_strip: ws2812@0 {
compatible = "worldsemi,ws2812-spi";
label = "WS2812";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label = "WS2812";

This label has been deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request shields PRs and issues related to shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants