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

Updated encoder config docs & standardization of existing shields #1970

Conversation

petejohanson
Copy link
Contributor

@petejohanson petejohanson commented Oct 18, 2023

  • Update new shield guide for new sensor/encoder settings.
  • Add DTS section to encoder config docs.
  • Update existing in-tree shields/boards to use the new encoder configuration consistently.

@petejohanson petejohanson added documentation Improvements or additions to documentation encoders labels Oct 18, 2023
@petejohanson petejohanson self-assigned this Oct 18, 2023
@petejohanson petejohanson force-pushed the sensors/sensor-config-follow-up-fixes branch from c07e769 to d1f120a Compare October 18, 2023 01:45
@petejohanson petejohanson marked this pull request as ready for review October 18, 2023 02:34
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.

Can we also add a pointer to docs/features/encoders.md to the config page, for users that need to adjust the encoder properties in the keymap? Right now it says

See the New Keyboard Shield documentation for how to add or modify additional encoders to your shield.

but I feel like config page is better for adjusting already configured encoders.

@caksoylar
Copy link
Contributor

This would close #1860.

@caksoylar
Copy link
Contributor

Also a bit unrelated but since we are touching encoder definitions in all shields: Should we get rid of the recommendation to set CONFIG_EC11_TRIGGER_GLOBAL_THREAD=y by making it default? I don't know the nuances of when you wouldn't want that, but I don't think I have seen a config with it not used.

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.

  1. ...I was surprised to see a PR which was ostensibly for docs incorporate changes to shields and boards.
  2. The suggested changes for MurphPad are at odds with my outstanding PR, which has been tested on an actual PCB and stock encoder provided by the vendor/creator. (I am also unsure if there is some reason its keymap was not modified—does the sensors node actually belong in the $shieldname.overlay file?)

@petejohanson
Copy link
Contributor Author

  1. ...I was surprised to see a PR which was ostensibly for docs incorporate changes to shields and boards.

Yeah, the PR title/summary got auto-populated by my first commit, and then I got to the follow up work. I will update accordingly.

2. The suggested changes for MurphPad are at odds with [my outstanding PR](https://github.com/zmkfirmware/zmk/pull/1786), which has been tested on an actual PCB and stock encoder provided by the vendor/creator. (I am also unsure if there is some reason its keymap was not modified—does the `sensors` node actually belong in the `$shieldname.overlay` file?)

So, we have a mix of approaches, probably in part because we weren't very diligent about how to structure things when encoder work got added.

IMHO, the "correct" way to do this is to add the sensors node in the overlay, and give it a label, e.g. sensors: sensors { so that it can be referred to in keymaps and updated as necessary.

So if you only installed encoders in certain positions, you might do:

&sensors {
  sensors = <&position_one_encoder>;
};

for instance. I didn't want to "rock the boat" too much as part of this refactor, but I'm certainly open to it if we want to be consistent so that future shields/boards that might copy others get it "right".

@petejohanson petejohanson changed the title fix(docs): Updated encoder config docs. Updated encoder config docs & standardization of existing shields Oct 20, 2023
@petejohanson petejohanson force-pushed the sensors/sensor-config-follow-up-fixes branch 3 times, most recently from 251f746 to f67ca66 Compare October 21, 2023 18:37
@petejohanson
Copy link
Contributor Author

Also a bit unrelated but since we are touching encoder definitions in all shields: Should we get rid of the recommendation to set CONFIG_EC11_TRIGGER_GLOBAL_THREAD=y by making it default? I don't know the nuances of when you wouldn't want that, but I don't think I have seen a config with it not used.

I want to explore this for sure, not sure it's in scope for this or not, since really we need to get the docs/reference designs fixed ASAP to avoid future confusion.

I didn't create that as the default because the convention for generic sensor drivers is to not do so... What may make sense is to leave the driver Kconfig as is, but in ZMK's Kconfig override the default for our use. Thoughts?

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.

LGTM, just noting the missing tweak for murphpad.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one doesn't seem to have triggers-per-rotation defined (sensors node is in the keymap)

Copy link
Collaborator

@joelspadin joelspadin 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 aside from the issue with murphpad missing triggers-per-rotation.

@petejohanson petejohanson force-pushed the sensors/sensor-config-follow-up-fixes branch from f67ca66 to 3598005 Compare November 3, 2023 18:37
petejohanson and others added 2 commits November 3, 2023 11:42
* Update new shield guide for new sensor/encoder settings.
* Add DTS section to encoder config docs.

Co-authored-by: Cem Aksoylar <[email protected]>
* Update existing boards/shields for new `steps` and
  `triggers-per-rotation` set up.
@petejohanson petejohanson force-pushed the sensors/sensor-config-follow-up-fixes branch from 3598005 to 703de1a Compare November 3, 2023 18:42
@petejohanson petejohanson merged commit 34c8b3f into zmkfirmware:main Nov 3, 2023
@lesshonor lesshonor mentioned this pull request Nov 14, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation encoders
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants