feat(endpoints): add "no endpoint" value#3140
feat(endpoints): add "no endpoint" value#3140joelspadin wants to merge 3 commits intozmkfirmware:mainfrom
Conversation
joelspadin
force-pushed
the
endpoint-updates
branch
2 times, most recently
from
ce45e34 to
a075a66
Compare
| peripheral 0 <dbg> zmk: security_changed: Security changed: FD:9E:B2:48:47:39 (random) level 2 | ||
| peripheral 0 <dbg> zmk: split_svc_pos_state_ccc: value 1 | ||
| peripheral 0 <dbg> zmk: split_svc_select_phys_layout_callback: Selecting physical layout after GATT write of 0 | ||
| peripheral 0 <dbg> zmk: split_svc_update_indicators_callback: Raising HID indicators changed event: 0 |
There was a problem hiding this comment.
I'm not really sure why this changed, but split_svc_update_indicators_callback is running once before zmk_hid_indicators_process_report now.
| LOG_DBG("USB is preferred and ready"); | ||
| return ZMK_TRANSPORT_USB; | ||
| } | ||
| if (is_ble_ready()) { |
There was a problem hiding this comment.
To implement the "no fallback" option later, we would simply disable this block and the similar one in the BLE case.
|
I definitely agree with the renaming of the functions, and the introduction of the new functions. Could you perhaps separate that out into a different commit, though? Would make it much easier to review imo. |
petejohanson
left a comment
petejohanson
left a comment
There was a problem hiding this comment.
On a whole this looks good. One minor comment from my first pass at this.
app/include/zmk/endpoints_types.h
Outdated
| * The method by which data is sent. | ||
| */ | ||
| enum zmk_transport { | ||
| ZMK_TRANSPORT_NONE, |
There was a problem hiding this comment.
I can't easily check from my phone, but IIRC this enum value gets saved to settings when a user user sets a preferred endpoint with the behavior, so this new value should get added to the end of this enum if so.
There was a problem hiding this comment.
I opted to keep this as value 0, because "none" is the most reasonable value for anything that is implicitly initialized to 0. Instead, I changed the setting name and added an upgrade routine if the old setting is present.
Tested on my nrf5340dk. With "endpoints/preferred" set to 1, endpoint_settings_load_preferred_v1() gets called, translates the value to 2, deletes the old setting, and writes to "endpoints/preferred2". (This also resulted in endpoint_settings_load_preferred_v2() being called, but I wasn't sure if I could rely on that happening, so I made sure preferred_transport is set in both cases.) On a subsequent power cycle, the v2 function is called and the v1 function is not.
There was a problem hiding this comment.
NONE == 0 makes sense and I get why changing the enum value is bad in theory but what would be the actual consequences in this case it you just did it without adding the whole migration system?
Are we talking about bricking keyboards? or just some one-off inconvenience such as shutting down having saved USB -> 0 and rebooting loading 0 -> NONE so we need to manually press &out OUT_xxx the first time around before being able to type again (or need to flash settings_reset if there's no output selection at all in the keymap)?
There was a problem hiding this comment.
Everyone who had set their keyboard to BLE would find their keyboard now preferring USB.
Everyone who had set their keyboard to USB would find their keyboard nonfunctional unless they happened to have &out ... on their keyboard and pressed it, because it would switch from preferring USB to preferring nothing. They would need to add this binding or do a full settings reset.
There was a problem hiding this comment.
(You are correct about the behavior for if USB was preferred, but I would hardly consider that to be just an inconvenience. That's basically signing us up for a lot of people asking for help on Discord.)
There was a problem hiding this comment.
but I would hardly consider that to be just an inconvenience. That's basically signing us up for a lot of people asking for help on Discord.
That's true, maybe I'm personally too tolerant to having to fiddle with the &out keys :)
This adds ZMK_TRANSPORT_NONE, which can be set as the preferred endpoint transport if you wish to prevent the keyboard from sending any output. More usefully, it also is used to indicate that the preferred endpoint is not available and it could not fall back to an available one. To go along with this, many endpoint functions are renamed for consistency, and a few new functions are added: - zmk_endpoint_get_preferred_transport() returns the value that was set with zmk_endpoint_set_preferred_transport(). - zmk_endpoint_get_preferred() returns the endpoint that will be used if it is available. This endpoint always has the same transport as zmk_endpoint_get_preferred_transport(). - zmk_endpoint_is_connected() is a shortcut to check if the keyboard is actually connected to an endpoint. This change is based on zmkfirmware#2572 but without the option to disable endpoint fallback. It does refactor code to allow adding that feature later.
joelspadin
force-pushed
the
endpoint-updates
branch
from
a075a66 to
8f15c04
Compare
joelspadin
changed the title
Adding ZMK_TRANSPORT_NONE at the start of enum zmk_transport results in the preferred transport setting no longer representing the same values when it is saved with earlier firmware and loaded with newer firmware. To fix this, the "endpoints/preferred" setting is now deprecated and replaced by "endpoints/preferred2". If the old setting is present, it is interpreted as the old enum type, upgraded to the new type, and saved immediately to the new setting. The old setting is then deleted. To avoid this happening again in the future, enum zmk_transport now has explicit values assigned to identifier, and a comment is also added to explain that existing values must not be changed.
joelspadin
force-pushed
the
endpoint-updates
branch
from
8f15c04 to
1e984a9
Compare
petejohanson
left a comment
petejohanson
left a comment
There was a problem hiding this comment.
LGTM, with one minor tweak we should make around preferred if ZMK_USB isn't set, since we're fixing this up anyways. Thanks for working on this!
The default value for preferred_transport is now set to USB only if CONFIG_ZMK_USB is enabled. If not, it falls back to BLE if CONFIG_ZMK_BLE is enabled, then to "none" if nothing is enabled.
4 participants
This adds ZMK_TRANSPORT_NONE, which can be set as the preferred endpoint transport if you wish to prevent the keyboard from sending any output. More usefully, it also is used to indicate that the preferred endpoint is not available and it could not fall back to an available one. To go along with this, many endpoint functions are renamed for consistency, and a few new functions are added:
zmk_endpoint_get_preferred_transport() returns the value that was set with zmk_endpoint_set_preferred_transport().
zmk_endpoint_get_preferred() returns the endpoint that will be used if it is available. This endpoint always has the same transport as zmk_endpoint_get_preferred_transport().
zmk_endpoint_is_connected() is a shortcut to check if the keyboard is actually connected to an endpoint.
This change is based on #2572 but without the option to disable endpoint fallback. It does refactor code to allow adding that feature later.
Since this changes the meanings of
enum zmk_transportvalues that are saved to settings, the setting for storing the preferred transport is renamed. If the old setting exists, it is interpreted using the old enum values and translated to the new ones. The old setting is then deleted, and the upgraded value is written to the new setting immediately.