-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(behavior) Add &sleep behavior #649
Conversation
app/include/zmk/activity.h
Outdated
enum zmk_activity_state zmk_activity_get_state(); | ||
int activity_set_state(enum zmk_activity_state state); |
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.
int activity_set_state(enum zmk_activity_state state); | |
int zmk_activity_set_state(enum zmk_activity_state state); |
To keep in line with the rest of our shared functions, I'm thinking this should be prefixed with zmk_
.
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.
Changed
// Second half of || statement is to ensure a user using &sleep doesn't lose sleep state when | ||
// the idle or sleep timers expire | ||
if (inactive_time > MAX_SLEEP_MS || activity_state == ZMK_ACTIVITY_SLEEP) { |
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.
I'm wondering if this handler is even called when ZMK is in deep sleep. I'd imagine it shouldn't, but if it does, why don't we just clear the timer? Seems unnecessary to run this while we're in sleep.
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.
I didn't see a great spot to reset the timer and if the timer code does run at all in sleep mode... we're still going to need the 2nd half of the condition to ensure it's never going to wake up when asleep.
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.
What I'm thinking is this handler shouldn't even be called once you're in deep sleep. Have you tested it without this additional conditional? From my understanding when we go into deep sleep, timer-based handlers are no longer run since we don't set up any timer wakeups and therefore don't have any timers running.
app/src/behaviors/behavior_sleep.c
Outdated
static const struct behavior_driver_api behavior_sleep_driver_api = { | ||
.binding_released = on_keymap_binding_released, | ||
}; |
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.
.binding_pressed
should be defined so you don't get an error in logs when pressing this key. You can have a function that just returns an opaque response like this: https://github.com/zmkfirmware/zmk/blob/main/app/src/behaviors/behavior_rgb_underglow.c#L54
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.
Added
|
||
static int on_keymap_binding_released(struct zmk_behavior_binding *binding, | ||
struct zmk_behavior_binding_event event) { | ||
return activity_set_state(ZMK_ACTIVITY_SLEEP); |
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.
return activity_set_state(ZMK_ACTIVITY_SLEEP); | |
#if IS_ENABLED(CONFIG_ZMK_SLEEP) | |
return activity_set_state(ZMK_ACTIVITY_SLEEP); | |
#endif | |
return ZMK_BEHAVIOR_OPAQUE; |
This is so we don't sleep when sleep isn't turned on and we press this key.
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.
Added
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.
Few additional tweaks needed.
app/dts/behaviors/sleep.dtsi
Outdated
|
||
/ { | ||
behaviors { | ||
sleep: behavior_sleep { |
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.
Please add the /omit-if-no-ref/
here like other behaviors were updated to include.
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.
Added
|
||
LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL); | ||
|
||
static int behavior_sleep_init(const struct device *dev) { return 0; }; |
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.
Add a wrapper #if HAS_DEVICE_STATUS_OKAY(DT_DRV_COMPAT)
like other drivers do now, so this doesn't compile if the DT is omitted.
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.
Added
@mcrosson did you see the review items on this? |
Yes, I thought it was agreed this was 'on hold' until after locality landed? I was planning on working on any necessary changes once locality landed. |
0550b30
to
f9699eb
Compare
Had some spare brain cycles and time ; updated according to the above feedback |
@@ -34,7 +34,8 @@ int raise_event() { | |||
(struct zmk_activity_state_changed){.state = activity_state})); | |||
} | |||
|
|||
int set_state(enum zmk_activity_state state) { | |||
int activity_set_state(enum zmk_activity_state state) { |
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.
I think you have to rename all of these activity_set_state
s to zmk_activity_set_state
.
// Second half of || statement is to ensure a user using &sleep doesn't lose sleep state when | ||
// the idle or sleep timers expire | ||
if (inactive_time > MAX_SLEEP_MS || activity_state == ZMK_ACTIVITY_SLEEP) { |
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.
What I'm thinking is this handler shouldn't even be called once you're in deep sleep. Have you tested it without this additional conditional? From my understanding when we go into deep sleep, timer-based handlers are no longer run since we don't set up any timer wakeups and therefore don't have any timers running.
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.
A few follow ups.
|
||
static const struct behavior_driver_api behavior_sleep_driver_api = { | ||
.binding_released = on_keymap_binding_released, | ||
.binding_pressed = on_keymap_binding_pressed, |
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 should have a .locality
set:
.binding_pressed = on_keymap_binding_pressed, | |
.binding_pressed = on_keymap_binding_pressed, | |
.locality = BEHAVIOR_LOCALITY_GLOBAL, |
|
||
``` | ||
&sleep | ||
``` |
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.
Please document this as being a global behavior, that will affect all components of a split. See other global behaviors, e.g. RGB, for an example docs.
@mcrosson Any plans to wrap this up, fix current conflicts, etc? |
I was trying to implement soft power off/lockout in the application code, happened to almost reimplement this. However, regardless of whether I called Edit: Oh I think I just found it, first introduced by pr 211 at In my code, the behavior is triggered upon pressing the key. My code is based on current latest main branch (94789a0). |
Arguably obsoleted by #2085 and related PRs. |
I recall someone requesting a &sleep behaviour in addition to &soft_off, to use &sleep for power conserving and &soft_off for transportation. For that reason I would say that it's not obsolete yet. |
Add a new behavior that'll allow users to put their keyboard to sleep ahead of timeouts. This is meant to be used as a binding on a keymap.