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

feature(shields): Add nice!view #1462

Merged
merged 23 commits into from Sep 30, 2022
Merged

Conversation

Nicell
Copy link
Member

@Nicell Nicell commented Sep 21, 2022

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)

@Nicell Nicell added the shields PRs and issues related to shields label Sep 21, 2022
@caksoylar
Copy link
Contributor

Is this definition intended to be used along with other shields using a build definition with multiple shields? If so, does the Github actions workflow support such a definition through build.yaml?

Is this also supposed to be compatible with nice!nano only (or with pro micro boards, for that matter)? I don't see why that should be, but it seems like the case from the SPI node alias.

@Nicell
Copy link
Member Author

Nicell commented Sep 22, 2022

Is this definition intended to be used along with other shields using a build definition with multiple shields? If so, does the Github actions workflow support such a definition through build.yaml?

Exactly! The GitHub actions workflow should work fine according to Pete... I need to test it though. Should just need to update the build.yaml shield prop to have nice_view after the existing one, so user configs should be fine. The repository's build workflow does need to be updated though.

Is this also supposed to be compatible with nice!nano only (or with pro micro boards, for that matter)? I don't see why that should be, but it seems like the case from the SPI node alias.

Should be compatible with all the other Pro Micro boards, I just need to define SPI nodes/aliases for others, which is on my todo.

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.

Some initial thoughts!

app/boards/shields/nice_view/nice_view.conf Outdated Show resolved Hide resolved
app/boards/shields/nice_view/nice_view.overlay Outdated Show resolved Hide resolved
app/boards/shields/nice_view/nice_view.zmk.yml Outdated Show resolved Hide resolved
app/boards/shields/nice_view/nice_view.overlay Outdated Show resolved Hide resolved
app/boards/arm/nice_nano/nice_nano.dtsi Outdated Show resolved Hide resolved
@Nicell
Copy link
Member Author

Nicell commented Sep 26, 2022

Our requires/exposes system doesn't really handle this new type of interconnect (built on top of a complete keyboard, not to create one). We'll need to figure out how we handle this with our build system and our hardware docs page. I've set up stubs and done nothing with them for now for both the build system and docs.

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.

A few follow up thoughts.

I think each shield deserves a readme file as well to explain their use, lacking tooling for this in some setup script.

app/boards/shields/nice_view/Kconfig.defconfig Outdated Show resolved Hide resolved
app/boards/shields/nice_view/nice_view.zmk.yml Outdated Show resolved Hide resolved
app/boards/shields/nice_view_oled/Kconfig.shield Outdated Show resolved Hide resolved
sck-pin = <20>;
mosi-pin = <17>;
miso-pin = <25>;
cs-gpios = <&pro_micro 1 GPIO_ACTIVE_HIGH>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think omit this entirely? Have folks override the cs-gpios property in their keymap for whatever pin they bodged.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'd be nice to allow for this to be a default fallback. I'll likely put in the docs for the nice!view to use this pin if available. (it should be for most since it's on the trrs serial pin)

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as this is clearly documented there, and in the README, that's fine.

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.

I think last thing: can you please add board overrides for the other in-tree nRF52 boards, e.g. nRFMicro, etc?

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.

Sorry, one last thing I noticed after a re-read of this.

Comment on lines 8 to 9
features:
- display
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be removed. The adapter itself doesn't add a display.

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.

Thanks!

@petejohanson petejohanson merged commit 3d3c45b into zmkfirmware:main Sep 30, 2022
@Nicell Nicell deleted the boards/nice-view branch October 3, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shields PRs and issues related to shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants