feat(core) Add preferred transport changed event#3320
feat(core) Add preferred transport changed event#3320yrb wants to merge 1 commit intozmkfirmware:mainfrom
Conversation
snoyer
left a comment
There was a problem hiding this comment.
Thanks! I tested with a custom display and it does solve the use-case of notifying when going between &out OUT_BLE and &out OUT_USB while only one of USB or BLE is available (meaning the active endpoint has nothing to change to, as mentioned in the description, and therefore no existing endpoint_changed event is fired).
Hopefully @joelspadin can do an actual review
Added an event for the preferred transport changing, this is primarily to trigger display updates.
bdc1ab8 to
fe3bba3
Compare
|
|
||
| #pragma once | ||
|
|
||
| #include <zephyr/kernel.h> |
There was a problem hiding this comment.
Is this header used for anything? Nothing below looks like it uses this to me.
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
||
| #include <zephyr/kernel.h> |
There was a problem hiding this comment.
This include doesn't seem to be used?
| } | ||
|
|
||
| ZMK_LISTENER(endpoint_listener, endpoint_listener); | ||
| ZMK_SUBSCRIPTION(endpoint_listener, zmk_preferred_transport_changed); |
There was a problem hiding this comment.
I'm a bit torn on this. On one hand, it does feel more elegant to have everything that could update the selected endpoint go through the event listener instead of some things using the event listener and others calling update_current_endpoint() directly. On the other, it's a bit more processing than is strictly necessary, and a different zmk_preferred_transport_changed listener could run before this one and see the updated preferred transport before the selected endpoint is updated to match (which shouldn't be an issue because anything that cares about the selected endpoint should also be listening for zmk_endpoint_changed, but it could potentially lead to something like displays showing inconsistent data for one frame).
I'd be interested to see what @petejohanson thinks of this.
| } | ||
|
|
||
| static int endpoint_settings_commit(void) { | ||
| update_current_endpoint(); |
There was a problem hiding this comment.
I'm not sure when this runs relative to zmk_endpoints_init(), but if it happens afterwards, then we likely need to check whether endpoint_settings_set() changed preferred_transport and send the new event if so.
This could be done by creating a new static variable to store the endpoint that's loaded from settings, then here we would just call zmk_endpoint_set_preferred_transport() with that value, though that would use up 4 bytes of memory just for that, and it would unnecessarily call endpoints_save_preferred() even though the value doesn't need to be written to settings. Alternatively, we could simply check if preferred_transport != DEFAULT_TRANSPORT and fire the change event if so.
Adds an event on preferred transport changing. The primary use of this is to drive display updates in the case the preferred transport changes but the active endpoint does not change.
PR check-list