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

refactor: replace zmk,matrix_transform with zmk,matrix-transform #2102

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

lesshonor
Copy link
Contributor

@lesshonor lesshonor commented Jan 8, 2024

  • While functionally equivalent, the hyphenated form of this property is more consistent with other ZMK properties and adheres to DTS style guidelines.
  • ...Since I'm already touching a number of files, might as well update links to use Zephyr 3.2 documentation instead of 2.5 where appropriate.

This PR intentionally refrains from updating any boards or shields to minimize disruption, as some forks and/or utilities may currently expect/require the underscore form (Keymap Editor being one).

@lesshonor lesshonor force-pushed the docs/matrix-transform branch from 543f684 to 280d612 Compare January 8, 2024 14:36
@joelspadin
Copy link
Collaborator

I think it may be confusing if the docs say one thing but all the code does something different. Can we check if it works to update the code to use hyphens and then build a user config repo that still uses an underscore when selecting a non-default transform? If we can confirm that it isn't a breaking change and the user config still selects the correct tranform, then we can update all the existing boards/shields to match this new documentation too.

@lesshonor lesshonor changed the title docs: suggest zmk,matrix-transform over zmk,matrix_transform refactor: replace zmk,matrix_transform with zmk,matrix-transform Jan 9, 2024
@lesshonor
Copy link
Contributor Author

lesshonor commented Jan 9, 2024

Can we check if it works to update the code to use hyphens and then build a user config repo that still uses an underscore when selecting a non-default transform?

Sure. Here's what happens when you force &five_column_transform on the default (six-column) corne keymap.

n.b. The CI failure is unrelated to my changes:

Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (400) Bad Request: number of artifacts reached artifact limit for job

@caksoylar
Copy link
Contributor

To analyze your results, using zmk,matrix_transform = &five_column_transform; in a keymap with the keyboard definition having zmk,matrix-transform = &something is working as expected -- as it results in excess array initializer errors with a 6 col keymap -- right?

@lesshonor
Copy link
Contributor Author

To analyze your results, using zmk,matrix_transform = &five_column_transform; in a keymap with the keyboard definition having zmk,matrix-transform = &something is working as expected -- as it results in excess array initializer errors with a 6 col keymap -- right?

Correct, as the user config repo commit describes. The 1000+ additional warning lines spewed out during the build action when there are too many elements are a much more obvious tell than having to painstakingly count the number of elements in two inconveniently-formatted devicetree nodes.

@caksoylar
Copy link
Contributor

I think best would be to test and confirm that the transform choice was effective, but absent that the build warnings seem like a good sign.

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.

Docs updates LGTM, thanks for updating the links as well -- they all seem to be live.

(I think it is a good idea in general too, given it is backwards compatible.)

@lesshonor
Copy link
Contributor Author

lesshonor commented Jan 12, 2024

I think best would be to test and confirm that the transform choice was effective, but absent that the build warnings seem like a good sign.

I've done that; I personally use this keymap, which draws on this module (and you're welcome to examine its last build)—but how do you propose I demonstrate the results? The number of nodes in the keymap file isn't modified by the selected transform, and there are no errors or warnings if it technically has too few keys. You can see zmk,matrix-transform and zmk,matrix_transform in the assembled devicetree of my test...
If you can tell me what you'd like to see, perhaps I could figure out how to show it?

@caksoylar
Copy link
Contributor

caksoylar commented Jan 12, 2024

I've done that; I personally use this keymap, which draws on this module (and you're welcome to examine its last build)—but how do you propose I demonstrate the results?

Maybe I missed it, but I didn't know that you had already tested it on a keyboard. Verbal confirmation is enough for me.

I guess you could write a unit test but I (personally) wouldn't think that is necessary.

* While functionally equivalent, the hyphenated form of this property
  is more consistent with other ZMK properties and adheres to DTS style
  guidelines.

* Additionally, update links to use Zephyr 3.2 documentation instead
  of 2.5 where appropriate.
@lesshonor lesshonor force-pushed the docs/matrix-transform branch from 7465064 to b959096 Compare January 24, 2024 02:41
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!

@joelspadin joelspadin merged commit 6b54701 into zmkfirmware:main Jan 24, 2024
7 checks passed
@lesshonor lesshonor deleted the docs/matrix-transform branch January 24, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants