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

feat(shield): add support of BabyvBle shield #967

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

Conversation

MakerJake01
Copy link

@MakerJake01 MakerJake01 commented Oct 5, 2021

Hello, this is my first pr and shield for zmk. The board is working on my pcb. I think I have filled out the checklist properly. I used current shield files as the base for the main structure of my files. Eye oh designs has approved the board's name. DFTBA.

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
  • 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
  • .conf file has optional extra features commented out

@dxmh dxmh added enhancement New feature or request shields PRs and issues related to shields labels Oct 5, 2021
Copy link
Collaborator

@dxmh dxmh left a comment

Choose a reason for hiding this comment

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

Hey! I took a quick first glance over the shield and left a few small comments.

app/boards/shields/babyvble/Kconfig.defconfig Outdated Show resolved Hide resolved
app/boards/shields/babyvble/babyvble.conf Outdated Show resolved Hide resolved
app/boards/shields/babyvble/babyvble.zmk.yml Outdated Show resolved Hide resolved
app/boards/shields/babyvble/babyvble.overlay Outdated Show resolved Hide resolved
These changes have been recommended by dxmh.
@MakerJake01
Copy link
Author

Thank you for the feedback I have made the changes!

Copy link
Collaborator

@dxmh dxmh left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes so quickly! It’s a tidy looking shield. 👍

I have a couple questions for @petejohanson:

  1. Is SHIELD_babyvble ok to be mixed case? I’m not sure if uppercase is required for any technical reason but may be best for consistency with other shields and similar references.

config SHIELD_babyvble

  1. Is there a standard/preference for the copyright headers that shields should use, with regards to The ZMK Contributors vs named individuals?

* Copyright (c) 2021 The ZMK Contributors

* Copyright (c) 2021 Pete Johanson, Richard Jones

@MakerJake01
Copy link
Author

Based on pr #978 I have changed the copyright to "The ZMK Contributors" in the documents that have a copyright section.

@MakerJake01 MakerJake01 requested a review from dxmh October 26, 2021 16:57
Copy link
Collaborator

@dxmh dxmh left a comment

Choose a reason for hiding this comment

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

Thanks @MakerJake01! I’ve suggested just a few minor changes below for consistency with other shields. We’ll then need @petejohanson or @Nicell to review.

Also, I couldn’t quite tell from the link in the metadata file - what is the general availability of this keyboard? Is it open-source, is there a GB planned, or is it stocked somewhere?

app/boards/shields/babyvble/Kconfig.defconfig Outdated Show resolved Hide resolved
app/boards/shields/babyvble/Kconfig.shield Outdated Show resolved Hide resolved
app/boards/shields/babyvble/babyvble.conf Outdated Show resolved Hide resolved
MakerJake01 and others added 2 commits October 26, 2021 17:04
Co-authored-by: Dom H <dom@hxy.io>
@MakerJake01
Copy link
Author

@dxmh At the moment I have been selling the extra PCBs that I have bought. I plan on doing something either in-stock, GB, or open-source. It's up in the air and I don't have a site that I update often. Most of my updates are in my discord server so to me it made the most sense to have that as the link.

@MakerJake01
Copy link
Author

MakerJake01 commented Dec 15, 2021

I'm starting to think I might need a .dtsi file. I will make one of those very soon. Edit: after some checking, it sounds like it's more need/helpful for split keyboards but as this is not a split I won't make one.

@MakerJake01 MakerJake01 requested a review from dxmh August 29, 2022 12:20
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.

After rebasing on main, you'll definitely need to install and run pre-commit to deal with formatting issues (tabs, trailing whitespace, etc). Squashing your commits would also be a very good idea.

Comment on lines +16 to +17
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 is now deprecated.

#include <behaviors.dtsi>
#include <dt-bindings/zmk/keys.h>
#include <dt-bindings/zmk/bt.h>
#include <dt-bindings/zmk/mouse.h>
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
#include <dt-bindings/zmk/mouse.h>

...Funnily enough, now that d7d9eed has been merged this would actually build. But you aren't using any mouse behaviors in this keymap, so there isn't much reason to include it.

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.

4 participants