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(hog): Cleanup ble message sending #2363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

khoek
Copy link
Contributor

@khoek khoek commented Jul 5, 2024

We deduplicate a bunch of code removing about 80 lines (not clear from the diff due to dependence on other PR), fixing a very minor copy-paste typo in the process. No functional change.

Stacked on top of #2257 since we use a struct from there. Tested on an Adv360.

@khoek khoek requested a review from a team as a code owner July 5, 2024 17:12
@khoek khoek marked this pull request as draft July 5, 2024 18:14
@khoek khoek marked this pull request as ready for review July 6, 2024 09:18
@Nick-Munnich Nick-Munnich added core Core functionality/behavior of ZMK bluetooth Bluetooth related items refactor usb labels Aug 2, 2024
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 the PR! Seems like a very reasonable approach. See my one comment about some additional cleanup that's likely possibly with this change.

} __packed;
} __packed;

struct zmk_hid_report {
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like this idea... I do wonder: Do we really need to keep around types like struct zmk_hid_mouse_report with this change?

@petejohanson petejohanson self-assigned this Sep 6, 2024
@petejohanson
Copy link
Contributor

@khoek Any chance you are still interested in working on this?

@khoek
Copy link
Contributor Author

khoek commented Sep 24, 2024

@petejohanson Sorry about the radio silence, I forgot about your question, and remember interpreting it as slightly more contemplative at the time. Sure, I'll have a play around and see if I can get rid of them.

By the way while I have your attention: I've been having a bunch of trouble with applications correctly interpreting some taps coming out of hold-tap because of their very fast duration (coincidentally the same phenomenon which triggered the race condition bug which got me poking around in the first place)---mainly this is happening in e.g. video game engines which poll the key states themselves instead of handling events, and I guess the key is being press-released within a single frame. Anyhow, is there a natural way to integrate a configurable tiny delay into the current way ZMK works to alleviate this problem?---if that would be a bit awkward, is this more motivation for me to take another look at unblocking the main thread and adding a message queue for outbound USB messages (of course queuing a message to be sent later after some delay is then very cheap to implement in that setting)?

@khoek
Copy link
Contributor Author

khoek commented Sep 24, 2024

By the way, besides the upsides about queuing multiple messages instead of waiting which I evangelized in that other thread, the only downside I could think of was that, naively implemented, there would be a tiny latency slowdown which people interested in maximum responsiveness wouldn't like. So I just wanted to note that I was attentive to that and I'd be changing the USB send function to immediately commit a USB transaction if there wasn't a pending operation in progress, and only queue in the alternative case (of if e.g. the message was to be transmitted after some delay).

@petejohanson
Copy link
Contributor

@petejohanson Sorry about the radio silence, I forgot about your question, and remember interpreting it as slightly more contemplative at the time. Sure, I'll have a play around and see if I can get rid of them.

By the way while I have your attention: I've been having a bunch of trouble with applications correctly interpreting some taps coming out of hold-tap because of their very fast duration (coincidentally the same phenomenon which triggered the race condition bug which got me poking around in the first place)---mainly this is happening in e.g. video game engines which poll the key states themselves instead of handling events, and I guess the key is being press-released within a single frame. Anyhow, is there a natural way to integrate a configurable tiny delay into the current way ZMK works to alleviate this problem?---if that would be a bit awkward, is this more motivation for me to take another look at unblocking the main thread and adding a message queue for outbound USB messages (of course queuing a message to be sent later after some delay is then very cheap to implement in that setting)?

You'd need to modify the hold-tap behavior to add a devicetree configurable release-delay and implement a sleep for that amount in the decide code for hold-tap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bluetooth Bluetooth related items core Core functionality/behavior of ZMK refactor usb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants