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

Add optional binding_convert_central_state_dependent_params callback to the behavior driver API. #626

Merged

Conversation

petejohanson
Copy link
Contributor

Based on discussions in Discord, this adds a new (optional) behavior driver callback, that allows behaviors to convert a relative binding invocation (e.g. "Toggle Power" or "Next Profile", into an absolute one.

This is mostly important for behaviors that, after #547, won't only be executed locally, but maybe "globally" on central and peripheral, we can ensure all devices converge to the same final state, not be out of sync.

I've only implemented this so far for ext power and RGB, since the "profile select" only matters for central side anyways.

@petejohanson petejohanson added enhancement New feature or request core Core functionality/behavior of ZMK behaviors labels Jan 22, 2021
@petejohanson petejohanson self-assigned this Jan 22, 2021
@okke-formsma
Copy link
Collaborator

okke-formsma commented Jan 22, 2021

Instead of doing the implementation this way, why not do the secondary behavior call from the 'relative' behavior?

Create a new binding and event struct and call behavior_keymap_binding_pressed(&new_binding, new_event);. Hold_tap and other behaviors are already doing this when they need to invoke another behavior.

I've been playing with making the behavior calls "just an event" (#532) which would make this process even simpler.

@petejohanson petejohanson force-pushed the core/binding-to-absolute-refactor branch from ebf9011 to 9fe6967 Compare January 28, 2021 18:29
@petejohanson petejohanson changed the title Add optional binding_to_absolute callback to the behavior driver API. Add optional binding_convert_central_state_dependent_params callback to the behavior driver API. Jan 28, 2021
@petejohanson
Copy link
Contributor Author

@Nicell are you able to test the RGB changes here?

Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

I see this only adds absolute calls for on/off of RGB. Are you planning to have absolute calls for hue, saturation, brightness, speed, and effect?

Confirmed that toggling still works on my nice60.

@petejohanson
Copy link
Contributor Author

I see this only adds absolute calls for on/off of RGB. Are you planning to have absolute calls for hue, saturation, brightness, speed, and effect?

Confirmed that toggling still works on my nice60.

Thank you! Totally missed this, and we can use the set color stuff from @mcrosson to do that, can't we?

@Nicell
Copy link
Member

Nicell commented Jan 30, 2021

Yeah, we can use it for hue, saturation, and brightness, but we'll need something else for speed and effect.

@petejohanson petejohanson requested a review from Nicell February 2, 2021 19:53
Copy link
Collaborator

@okke-formsma okke-formsma left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Noticed some bad logic that you've inherited from me. Other than that looks pretty good. HSB refactor seems nice.

Comment on lines 354 to 355
if (color.h == 0 && direction < 0) {
return color;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is sufficient, and it also doesn't support wrapping around. Instead of this, after updating the hue with += direction * CONFIG_ZMK_RGB_UNDERGLOW_HUE_STEP, if the result is less than 0, add 360.

struct zmk_led_hsb zmk_rgb_underglow_calc_sat(int direction) {
struct zmk_led_hsb color = state.color;

if (color.s == 0 && direction < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (color.s == 0 && direction < 0) {
if (color.s - CONFIG_ZMK_RGB_UNDERGLOW_SAT_STEP < 0 && direction < 0) {

Something like this is better. Or even setting to zero if it's less than 0 after the fact. If the step is more than 1, this won't catch all cases (which I doubt many people would use 1).

struct zmk_led_hsb zmk_rgb_underglow_calc_brt(int direction) {
struct zmk_led_hsb color = state.color;

if (color.b == 0 && direction < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Same idea here as above.

* Allow each behavior to map a relative binding, e.g. "toggle",
  to an absolute one, e.g. "on", before being invoked.
@petejohanson petejohanson force-pushed the core/binding-to-absolute-refactor branch from 03a29f3 to 52d81ea Compare February 9, 2021 04:50
@petejohanson
Copy link
Contributor Author

@Nicell Ok, fixed up the calc functions, proper looping for hue, proper clamping for saturation and brightness. Thoughts? Tested fine on my left half of my Kyria.

app/src/rgb_underglow.c Outdated Show resolved Hide resolved
app/src/rgb_underglow.c Show resolved Hide resolved
app/src/rgb_underglow.c Show resolved Hide resolved
@petejohanson petejohanson force-pushed the core/binding-to-absolute-refactor branch from 52d81ea to 24f8fd3 Compare February 9, 2021 06:02

struct led_hsb hsb = {hue, sat, brt};
struct zmk_led_hsb hsb = state.color;
hsb.h = state.animation_speed;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hsb.h = state.animation_speed;
hsb.h = state.animation_step;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed.

* Public type for HSB led color.
* New API for calculating "next" HSB based on current
  state.
* Update behavior to convert the increment/decrement
  commands to absolute command as well.
@petejohanson petejohanson force-pushed the core/binding-to-absolute-refactor branch from 24f8fd3 to f48faa3 Compare February 9, 2021 06:17
@petejohanson petejohanson merged commit 2af794e into zmkfirmware:main Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviors core Core functionality/behavior of ZMK enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants