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

RoMac+ v4 Support for nice!nano v1 #198

Merged
merged 7 commits into from Sep 24, 2020

Conversation

reizero00
Copy link
Contributor

New Shield: RoMac+ V4

This new version of the RoMac has an encoder. I only concentrated on gaining a functional Bluetooth board with an encoder. Since the nice!nano is typically used with a small lipo battery I did not concentrate on LED/OLED support and left that as is.

@innovaker innovaker added the shields PRs and issues related to shields label Sep 22, 2020
@reizero00
Copy link
Contributor Author

Looks like the build is failing for proton c but I didn't write anything for proton c. I don't own the hardware so I can't test compatibility locally.

@petejohanson petejohanson added the enhancement New feature or request label Sep 23, 2020
@@ -0,0 +1,3 @@
# Uncomment to enable encoder
CONFIG_EC11=y
Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely the source of the current build failures. I don't think I've ever tested the encoder stuff on stm32. Is the encoder always on this RoMac v4? If not, can you comment these out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. For the RoMac+ v4 users have the option of an encoder or a key. So what I will do is comment out those configs as requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I disabled both the EC11 configs for consistency's sake since that's what I saw in other shields. Still builds. I'll just watch the build tests.

@reizero00
Copy link
Contributor Author

Build passes now. All ready for your review @petejohanson

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 for this PR! A few comments/tweaks, but on a whole this looks great!

@@ -0,0 +1,9 @@
# Copyright (c) 2020 Pete Johanson, Richard Jones
Copy link
Contributor

Choose a reason for hiding this comment

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

We are trying to move towards a "single authors file" approach where we can, so if you're comfortable with it, I would ask if you wouldn't mind using headers like:

Suggested change
# Copyright (c) 2020 Pete Johanson, Richard Jones
# Copyright (c) 2020 The ZMK Contributors

And then if you want to, you're welcome to add yourself to the end of the AUTHORS file at the top of the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as a part of 4c09204 and added this to romac_plus.conf for consistency.

@@ -0,0 +1,5 @@
# Copyright (c) 2020 Pete Johanson, Richard Jones
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as a part of 4c09204 and added this to romac_plus.conf for consistency.

if SHIELD_ROMAC_PLUS

config ZMK_KEYBOARD_NAME
default "RoMac_plus-v4"
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a "user friendly name", so can have spaces, etc. Perhaps:

Suggested change
default "RoMac_plus-v4"
default "RoMac+ v4"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay. I updated it here. I think I couldn't use + in bluemicro or something so I applied the same logic. Nice and clean now. See
0ce3686

@@ -0,0 +1,61 @@
/*
* Copyright (c) 2020 Pete Johanson
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as a part of 4c09204

@@ -0,0 +1,28 @@
&spi1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a license header here too? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as a part of 4c09204

sensors = <&left_encoder>;
};

bt_unpair_combo: bt_unpair_combo {
Copy link
Contributor

Choose a reason for hiding this comment

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

This functionality is going away soon. We shouldn't include that for new shields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool I added the functionality in
9fffebd
and enabled it while I cleaned things up in 641524b because I forgot I commented it out lol. Tested the new firmware after the BT changes and it's much better how it can re-pair gracefully!


// nav_layer {
// -----------------------
// | _ | HOME | PGUP |
Copy link
Contributor

Choose a reason for hiding this comment

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

We're trying to include the &bt bindings, at least the "clear" one, in stock keymaps now where we can. See https://zmkfirmware.dev/docs/behavior/bluetooth/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alrighty. Added it to a lower layer to prevent accidental clearing in 9fffebd

It was a bit finnicky if you already had a device paired with the previous firmware (before these changes). I could not simply press the reset button once. I had to use BT_CLR in order to repair the device. With this expectation change I guess some people might have issues.

@@ -0,0 +1,37 @@
/*
* Copyright (c) 2020 Pete Johanson, Richard Jones
Copy link
Contributor

Choose a reason for hiding this comment

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

Header ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as a part of 4c09204

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! Thanks.

@petejohanson petejohanson merged commit 6ecf62a into zmkfirmware:main Sep 24, 2020
1 check passed
MangoIV pushed a commit to MangoIV/zmk that referenced this pull request Dec 18, 2020
tyalie pushed a commit to tyalie/zmk that referenced this pull request Nov 15, 2022
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.

None yet

3 participants