-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: Split endpoint to transport and instance #1803
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a whole, I really like it. A couple minor questions/suggestions.
9d7a329
to
fcec2ad
Compare
fcec2ad
to
9c519c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple minor things. On a whole this looks great.
app/include/zmk/endpoints.h
Outdated
*/ | ||
#define ZMK_ENDPOINT_COUNT (ZMK_ENDPOINT_USB_COUNT + ZMK_ENDPOINT_BLE_COUNT) | ||
|
||
bool zmk_endpoint_instance_equals(struct zmk_endpoint_instance a, struct zmk_endpoint_instance b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, Zephyr uses eq
shorthand pretty consistently, any reason not to use that convention as well here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Wasn't aware of that convention. I can update this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also changed the _print()
function to _to_str()
and reordered the arguments to match what looks like Zephyr conventions for that type of function as well.
Changed the endpoints code to rename the existing endpoint types to "transport" and add the concept of "endpoint instances". A transport is the method by which data is sent, while instances allow describing multiple endpoints that use the same transport (e.g. bluetooth profiles) Also added new APIs to get the total number of possible endpoint instances and assign each instance a unique index, which can be used for tracking separate state for each endpoint in other code files.
9c519c5
to
e4ae6b5
Compare
Tested here on my ZMK Uno, and toggling the 3-way switch to toggle the selected endpoint all works as expected, including the output display, which is one of the main consumers of the change to the events. Will review the code, but functionality wise it looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and behavior all looks great, just my one comment about the extra struct field that looks like it can be removed as well.
Addressed the review comments and fixed a missing include. I put the fixes up as a separate commit so you can easily review the difference. (You'll probably want to squash them together when merging this.) |
Guess I missed the nice!view code since that was probably added after I started this. |
This last commit is untested aside from that it builds. It would be good to have someone verify this on a nice!view. (I do have one, but I haven't hooked it up to anything yet, so I'd need to do a bit of work before I could test it myself.) I also took the opportunity to optimize a few functions by passing the large status struct by pointer instead of value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me. I can build with a nice!view here, but don't easily have hardware to test on right now either. Will put out call for testers on Discord for some validation before merging this.
Ok, was able to test this on my ZMK Uno built with a nice!view, and they all build and run perfectly fine. Merging, thanks @joelspadin! |
Changed the endpoints code to rename the existing endpoint types to "endpoint transport" and add the concept of "endpoint instances". A transport is the method by which data is sent, while instances allow describing multiple endpoints that use the same transport (e.g. bluetooth profiles).
Also added new APIs to get the total number of possible endpoint instances and assign each instance a unique index, which can be used for tracking separate state for each endpoint in other code files.