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 ergodash shield #572

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Add ergodash shield #572

merged 2 commits into from
Oct 13, 2021

Conversation

Eyenseo
Copy link
Contributor

@Eyenseo Eyenseo commented Jan 2, 2021

No description provided.

@innovaker innovaker added enhancement New feature or request shields PRs and issues related to shields labels Jan 2, 2021
@innovaker innovaker requested a review from Nicell January 2, 2021 17:40
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.

I have a couple comments to start, and then could you add this shield to the following files, please?

docs/docs/hardware.md: You should add the Ergodash to this list

docs/static/setup.ps1 and docs/static/setup.sh: Ergodash should be added as a split option for both files.

app/boards/shields/ergodash/ergodash.keymap Outdated Show resolved Hide resolved
@Eyenseo
Copy link
Contributor Author

Eyenseo commented Jan 17, 2021

I'll make another change later today. I'll change the Colemak layout to querty so it's easier for people to reuse.

@Eyenseo
Copy link
Contributor Author

Eyenseo commented Jan 17, 2021

@Nicell @petejohanson my last commit is somewhat funky. Is there an issue that tracks this behaviour? Apparently just references is not allowed in a keymap.

@daegalus
Copy link

Is there anything blocking this being merged? I just tested this on my Ergodash build and it works well. I redid the maps for myself but everything mapped correctly and it is working well.

@Eyenseo
Copy link
Contributor Author

Eyenseo commented Mar 12, 2021

@daegalus 🤷

I rebased onto main again removed the hack to compile - that is apparently no longer necessary. Once reviewed I would squash the commits so it can be merged.

@michelv
Copy link

michelv commented Apr 6, 2021

Hi! Ergodash user here, I'll test once I have prepared my nice!nano's for the swap.

From the keymap file though, it looks like it's missing support for those who have more thumb keys (two spacebars per half for example). Or am I misreading the file?

@Eyenseo
Copy link
Contributor Author

Eyenseo commented Apr 6, 2021

@michelv This layout is for the second from the top [1] - I'm not sure how the other layouts differ. But I'm sure that that can be configured with defines :)

[1] https://raw.githubusercontent.com/omkbd/picture/master/ergodash-layout.png

@daegalus
Copy link

daegalus commented Apr 7, 2021

You have to remember that the ergodash only has 4 possible keys in the thumb cluster. Depending on your configuration a 2u key using the center slot will eat up 2 keys and will be set by the same key in the config. You have to play around with it a bit. But the layout is correct.

@daegalus
Copy link

Any updates on getting this merged?

@petejohanson
Copy link
Contributor

Thank you, contributor, for your patience with how long review and merge of boards/shields has taken!

There are three recent refactors/changes to boards and shields that require some attention, and then we can finally get this PR merged!

  1. Hardware Metadata
  2. Pro Micro shield DT naming changes
  3. Split changes for BLE advertising

Hardware Metadata

The Problem

When first developing the process around contributing new shields/boards to ZMK, we failed to recognize that several key files (setup scripts, documentation page of supported hardware, and GH Action build.yml file) required changes, often in the same spot, for every PR. This resulted in immediate merge conflicts for every other PR after one was merged, which is a headache for contributors.

The Fix

By adding discrete metadata files that are located with the boards/shields in question, and using that metadata to generate setup scripts, website hardware list, etc., users can contributing new hardware descriptions without the need to change the same files that other contributors are changing.

Next Steps

First, refer to https://zmk.dev/docs/development/hardware-metadata-files to familiarize yourself with the new metadata file format.

Next, you have two options for fixing up your PR:

  1. If comfortable with git rebase, perform an interactive rebase and revert any changes to build.yml, hardware.md, or the setup scripts setup.sh/setup.ps1, and then add the new metadata YAML file. Then force push your branch. Or,
  2. Create a new branch from an up-to-date main, copy in the files for your new hardware, add the metadata file, then commit and push the new branch. Then, edit your open PR to point to your new branch.

Pro Micro shield DT naming changes

In #876, we have simplified the DT naming for the "nexus node" we expose for pro-micro compatible boards, deprecating the use of pro_micro_a, and renaming pro_micro_d node to simply pro_micro. For pro-micro boards and shields, you'll need to adjust your DT to use the proper names.

Please see https://zmk.dev/docs/development/new-shield#shield-overlays for the updated docs on this.

Split Shield Advertising Changes

In addition, if this is a split PR, please see #658 where we have changed our conventions to remove the the name from the right sides, to prevent users attempting to pair with them and causing split sync issues. This also includes removing the " Left" suffix from the naming on the left side. See the changes in that PR for examples of what to change with your split shield.

Getting Help

If you have any questions about any of these changes, please comment here and tag @zmkfirmware/boards-shields or ask in the #boards-shields Discord channel.

@daegalus
Copy link

daegalus commented Oct 5, 2021

@petejohanson Looks like the PR owner has updated the repo with your requested changes, is there anything else that needs to be done?

app/boards/shields/ergodash/ergodash.keymap Show resolved Hide resolved
app/boards/shields/ergodash/ergodash.zmk.yml Outdated Show resolved Hide resolved
app/boards/shields/ergodash/ergodash.keymap Outdated Show resolved Hide resolved
app/boards/shields/ergodash/ergodash.dtsi Outdated Show resolved Hide resolved
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.

@Eyenseo, thanks for fixing up those last few things. This looks great to me now. One last thing: could you confirm that this config is working on real hardware?

@Eyenseo
Copy link
Contributor Author

Eyenseo commented Oct 13, 2021

@Nicell I'm running this with a debounce and keymap overlay. If that is good enough for you.

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.

@Eyenseo it is! Just want to make sure it's working on real hardware.

@Nicell Nicell merged commit 6f29453 into zmkfirmware:main Oct 13, 2021
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.

6 participants