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

Resolve preserve locality outside of direct keymap bindings #1630

Closed
wants to merge 1 commit into from

Conversation

tokazio
Copy link
Contributor

@tokazio tokazio commented Jan 17, 2023

Seems to resolve #1494, at least resolve #1347

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 working on this! I think this is a good start, but some tweaks needed.

struct zmk_behavior_binding_event event, bool state);

int zmk_run_behavior(struct zmk_behavior_binding *binding, struct zmk_behavior_binding_event event,uint8_t source,bool pressed);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this general functionality is useful, but a few things:

  • This should be in keymap.h, not this header.
  • I don't love the name. We are triggering the pressed/released callbacks, not "running" something.

return zmk_run_behavior(&binding,event,source,pressed);
}

int zmk_run_behavior(struct zmk_behavior_binding *binding, struct zmk_behavior_binding_event event,uint8_t source,bool pressed){
Copy link
Contributor

Choose a reason for hiding this comment

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

binding probably should be const here. Also, be sure to run clang-format on any changed files for consistent formatting.

@tokazio
Copy link
Contributor Author

tokazio commented Jan 25, 2023

Thanks for the review.

I see some delay when the trigger comes from the left part of the keyboard that i doesn't see when the trigger comes from the right part.

delayledkb.mov

xudongzheng pushed a commit to xudongzheng/zmk that referenced this pull request Feb 23, 2023
@xudongzheng
Copy link
Contributor

xudongzheng commented Feb 23, 2023

I see some delay when the trigger comes from the left part of the keyboard that i doesn't see when the trigger comes from the right part.

This delay is expected. Since the connection interval is 7.5ms and connection latency is 30, it can take up to 7.5ms*(30+1)=232.5ms for the peripheral to receive the update from the central.

param = BT_LE_CONN_PARAM(0x0006, 0x0006, 30, 400);

@petejohanson
Copy link
Contributor

Exactly right... We could lower that connection latency, at the expense of increased power consumption on the peripheral side. (side note: it'd be interesting to renegotiate the connection parameters if the power status of the peripheral changes thanks to being plugged in, to improve responsiveness of this exact scenario....)

vladsolokha added a commit to vladsolokha/zmk-config that referenced this pull request Mar 10, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@vladsolokha
Copy link

Thanks for the review.

I see some delay when the trigger comes from the left part of the keyboard that i doesn't see when the trigger comes from the right part.

Can you share how you make that behavior happen in your keymap? The layer switch and underglow change? Is there a way I could do the same and have the rgb colors go back to the original effect when the layer is then switched off or released?

I have some rgb mods on &to layer switches in my keymap here, but I was hoping to do the same for my two main layers 1 and 2. I was hoping to leave them as sticky on tap and momentary on hold with the color changes.

https://github.com/vladsolokha/zmk-config/blob/main/config/splitkb_aurora_sweep.keymap

@tokazio
Copy link
Contributor Author

tokazio commented Mar 14, 2023

You Can find my config here https://github.com/tokazio/zmk-config-sofle
Nothing special
I wanted a 3rd layer with a sticky underglow, on thé combinaison of upper and lower (selected underglow color while next Key press) getting back to first layer After another Key is pressed but with no success, the underglow is reset directly when i release upper/lower

@tokazio
Copy link
Contributor Author

tokazio commented Mar 14, 2023

I see some delay when the trigger comes from the left part of the keyboard that i doesn't see when the trigger comes from the right part.

This delay is expected. Since the connection interval is 7.5ms and connection latency is 30, it can take up to 7.5ms*(30+1)=232.5ms for the peripheral to receive the update from the central.

param = BT_LE_CONN_PARAM(0x0006, 0x0006, 30, 400);

Oh ok, good to know it
Is there a possibilité to force the update when this précise Key is down ?

@xudongzheng
Copy link
Contributor

xudongzheng commented Mar 14, 2023

Is there a possibilité to force the update when this précise Key is down ?

One option is defining a read service/characteristic on the central that you can read from the peripheral. This should have the current LED state.

After a key event such as a press/release on the peripheral happens, you read the state on central from the peripheral (rather than waiting for the central to write to peripheral). You can try to do this immediately or after a few milliseconds to give the central to process and update the state.

I'm not familiar enough with the Bluetooth specification to be certain that this wouldn't be subject to the latency value, but this is the first thing I would try if I needed to reduce the latency without sacrificing battery life.

It's not trivial to implement and I would consider just reducing the latency to a lower value at the cost of battery life.

@petejohanson
Copy link
Contributor

Can you please also rebase this to be sure it is conflict free?

@petejohanson petejohanson self-assigned this May 11, 2023
@tokazio tokazio requested a review from a team as a code owner June 9, 2023 08:41
@tokazio tokazio force-pushed the main branch 3 times, most recently from aba769f to e2b6e68 Compare June 9, 2023 09:30
@caksoylar
Copy link
Contributor

@tokazio in case you missed it, @petejohanson noted a couple items here that needed changing: #1630 (review)

@tokazio
Copy link
Contributor Author

tokazio commented Jun 17, 2023

After some time of use i'm observing that colors, when i push down the upper/lower keys are not the ones i've programmed. Even the effect have changed. I have now the multicolors wave effect. Maybe there is a conflict with the 'persistence' feature of effect/color that is triggered after a given time

@seansfkelley
Copy link

Thanks for opening this; this resolved an issue I was having with underglow on the peripheral side. I also see noticeable lag between the central and peripheral sides, but for my usages (which are infrequent), that's fine.

@petejohanson it looks like your comments are only stylistic. Would you accept a PR with the same behaviors and your style points addressed, or would you rather prioritize something like #2036 instead? (It claims to resolve this issue as well, though I have not tried it.) I can open a cleaned-up PR if so.

seansfkelley added a commit to seansfkelley/zmk that referenced this pull request Mar 11, 2024
…tors.

This commit does two things:

1. Provides default values for all indicator-underglow properties, which
   allows you to omit them entirely if you do not want that indicator.
2. Allows the right side to specify battery indicators.

I changed the phrasing of the battery left/right properties to be
self/other instead.

For (2), note that this currently does not work with the default Glove80
definition for the magic (indicator) key, since the right side does not
received the macro-wrapped `&rgb_ug RGB_STATUS` keypress. See the
usptream issue zmkfirmware#1494 for more.
(I resolved this for my own build by incorporating the changes from
zmkfirmware#1630.)
seansfkelley added a commit to seansfkelley/zmk that referenced this pull request Mar 11, 2024
seansfkelley added a commit to seansfkelley/zmk that referenced this pull request Mar 14, 2024
@petejohanson
Copy link
Contributor

Thanks for opening this; this resolved an issue I was having with underglow on the peripheral side. I also see noticeable lag between the central and peripheral sides, but for my usages (which are infrequent), that's fine.

@petejohanson it looks like your comments are only stylistic. Would you accept a PR with the same behaviors and your style points addressed, or would you rather prioritize something like #2036 instead? (It claims to resolve this issue as well, though I have not tried it.) I can open a cleaned-up PR if so.

I'd rather we clean this up and get it merged, and then pursue #2036 as a next step.

@seansfkelley
Copy link

Sounds good, thanks. It seems like #2409 is currently active and does just that, so I won't add more noisy PRs just yet, but if that stalls out I'll open another PR like this but with focusing on resolving just the stylistic comments.

@caksoylar
Copy link
Contributor

caksoylar commented Aug 14, 2024

My understanding is that this PR is not actually solving the issue besides modifying only macros to invoke behaviors globally. #2409 should solve both global and source locality invocations for every kind of behavior that invokes other behaviors (currently with the exception of sensors not respecting source locality). As such this PR shouldn't be accepted even without the stylistic issues and I don't intend to stall #2409. (If you like to help out, more testing and confirmation that things work as claimed always helps.)

@caksoylar
Copy link
Contributor

Closing as it is incorporated into #2409.

@caksoylar caksoylar closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants