Invoke behaviors with different "locality" #547
Conversation
petejohanson
added
enhancement
petejohanson
requested review from
okke-formsma,
joelspadin,
Nicell and
innovaker
petejohanson
force-pushed
the
core/peripheral-behavior-invocation
branch
2 times, most recently
from
6970b68
to
f6e91fd
Compare
app/include/zmk/behavior.h
Outdated
| @@ -6,6 +6,8 @@ | |||
|
|
|||
| #pragma once | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Just whitespace changes in this file.
app/src/split/bluetooth/central.c
Outdated
| } else { | ||
| subscribe_params.notify = split_central_notify_func; | ||
| subscribe_params.value = BT_GATT_CCC_NOTIFY; | ||
| subscribe_params.ccc_handle = attr->handle; | ||
|
|
||
| split_central_subscribe(conn); | ||
|
|
||
| return BT_GATT_ITER_STOP; | ||
| memcpy(&uuid, BT_UUID_DECLARE_128(ZMK_SPLIT_BT_CHAR_RUN_BEHAVIOR_UUID), sizeof(uuid)); |
There was a problem hiding this comment.
Is it intended that split_central_process_connection and this function share the static uuid variable? It looks like everything here overwrites the UUID passed to bt_gatt_discover while split_central_process_connection does not. If that's not intended, maybe put separate static variables into each function to ensure they can't both access the same variable.
I'm not sure I fully understand what this code is doing yet, but if split_central_process_connection is always supposed to be discovering ZMK_SPLIT_BT_SERVICE_UUID, it looks like it actually is re-discovering the last thing you tried to discover.
There was a problem hiding this comment.
This code is cribbed from a discovery example from Zephyr. As I read it, they're trying to reduce memory usage by re-using several variables as they discover/iterate the attributes. Let me review these changes again before we move forward, be sure we're still happy with how this is shaping up.
There was a problem hiding this comment.
@joelspadin Ok, I've cleaned this up a lot, and in the process, I believe found and fixed a split bug that would sometimes cause peripherals not to get subscribed to properly if the central is kept on and the peripheral is reset and reconnected to.
The changes depend on the fix in zmkfirmware/zephyr#3 being merged first, then this can be tested.
| @@ -134,24 +145,50 @@ int zmk_keymap_apply_position_state(int layer, uint32_t position, bool pressed, | |||
| behavior = device_get_binding(binding->behavior_dev); | |||
|
|
|||
| if (!behavior) { | |||
| LOG_DBG("No behavior assigned to %d on layer %d", position, layer); | |||
| LOG_WRN("No behavior assigned to %d on layer %d", position, layer); | |||
There was a problem hiding this comment.
Did this fall out of testing?
There was a problem hiding this comment.
Crept in, yeah. Was easier to spot in log output, and felt like a better level.
petejohanson
force-pushed
the
core/peripheral-behavior-invocation
branch
3 times, most recently
from
e7f2ebe
to
2253530
Compare
|
The code looks fine to me. There are some comments by @joelspadin that haven't been addressed yet @petejohanson. |
petejohanson
force-pushed
the
core/peripheral-behavior-invocation
branch
2 times, most recently
from
c2070cf
to
7af2e6e
Compare
petejohanson
force-pushed
the
core/peripheral-behavior-invocation
branch
from
58a4233
to
68ea026
Compare
petejohanson
force-pushed
the
core/peripheral-behavior-invocation
branch
2 times, most recently
from
52947b7
to
343870e
Compare
|
I turned on USB debugging to see if I could add anything to my previous report. I couldn't glean anything interesting out of the reports when I tried to toggle external power but I did see something weird when trying to use &reset or &bootloader on the central side. Doing that, the logging receives an error: I added to the log message and saw the value of peripherals[payload_wrapper.source].state is 0 (PERIPHERAL_SLOT_STATE_OPEN). At first I thought this was maybe due to the fact that I was only testing central but even when I connected peripheral, I had no luck. |
Sorry you missed this! Related, but not exactly the same, is state synchronization between halves. I would like to get this in as is, and the continue exploring additional work here while also getting some other work moving forward. Looking forward to seeing what you came up w/! |
|
I've just pushed a fix for invoking Regarding external power.... That seems to be working here on my Kyria, but that also has RGB. I will test on my Corne as well. Can you get logs for the peripheral side if you're still having issues w/ the latest on this branch? |
Please retest, I pushed a fix for this.
@Percentnineteen oI've tested this again on my Corne, no underglow enabled, and external power properly enables/disables just fine. Can you please pull and be sure you've got the latest code from this PR? There was a small fix for this I included while ago along w/ a rebase, so be sure the latest sha1/commit after you pull is: |
|
I tested the update and the &reset and &bootloader work now on the central side. However, I tested &ext_power EP_TOG/ON/OFF and they still don't seem to work. I logged the peripheral and got this: I issued EXT_TOG twice. The central side turned off immediately after the first command executed while peripheral stayed on. I assume peripheral should shut off immediately also. Central does not turn on when turning the power back on but I believe that is due to a different issue with reinitializing the OLED (VCC goes high, the display just doesn't come back). I believe I have the latest from this PR. The top commit when I do "git log" is: |
|
@Percentnineteen Hmm.... I suspect maybe only the release, not the press, is getting sent. Can you pull again ans flash the right side and get the logs one more time? Thanks for all the testing! |
|
No problem. I pulled and got this in the logs: |
Yeah, only seeing the releases, not the presses, which is the issue. Can you link to your keymap? Is this invoked from a combo, or hold-tap maybe? |
|
@Percentnineteen Pushed one more small fix. Can you try one more tie? Should only require flashing the peripheral side. |
|
Trying to link to my zmk-config repo but it's a fork of the miryoku repo and GitHub seemingly won't let me share it... No combos - I've just got a layer-tap to activate a layer and have a key mapped to &ext_power EP_TOG. After converting from miryoku's format to default dtsi format looks like this: |
|
Crap! I didn't see the last fix before my last message. I pulled that last fix: After pulling this, it works. Awesome. |
There was a problem hiding this comment.
I mostly reviewed this so that I could familiarize myself with the changes going forward. All the changes make sense to me, and the idea of central, global, and source looks good. Nice touch that global sends to all peripherals, which I presume will make dongle-ing easier in the future. I left a few comments, but I think I answered one myself. Great work!
|
|
||
| switch (locality) { | ||
| case BEHAVIOR_LOCALITY_CENTRAL: | ||
| return invoke_locally(&binding, event, pressed); |
There was a problem hiding this comment.
Is there a reason this doesn't need to be wrapped in #if ZMK_BLE_IS_CENTRAL? Is it because if it's pressed on the peripheral, the position gets sent to the central and never gets sent back to be invoked?
There was a problem hiding this comment.
That's because technically, for a split or non-split, you'd always want to run this locally, and keymaps aren't ever processed anywhere else, so it doesn't need to be conditional in either case.
There was a problem hiding this comment.
Just clicked for me that the #if ZMK_BLE_IS_CENTRAL is being used to identify if the device is a split keyboard, not if it's the central half. Makes a lot more sense. Would #if IS_ENABLED(CONFIG_ZMK_SPLIT) be a better check since only central will ever be running it anyways?
* GATT characteristic allowing passng data + behavior label to invoke the behavior on the peripheral side. * Behaviors have a locality setting to specify where they run. * Build reset/power/RGB on peripheral.
* Use Zephyr auto CCC discovery instead of doing it ourselves. * Split service versus characteristic discovery into dedicated steps in the flow. * Fix for not searching properly when connecting to a peripheral a second time.
* Convert relative effect cycling to absolute effect selection.
* Remove `/omit-if-no-ref/` from the behavior nodes.
* Track peripherals by indexes slot, with all appropiate peripheral state stored in the slot. * Event sources tracked by peripheral slot index.
* Don'd omit unreferenced reset behaviors, so they are always available in split peripherals.
* Add `ZMK_POSITION_STATE_CHANGE_SOURCE_LOCAL` and use it consinstently to fix bug w/ local `&reset`, `&bootloader`, etc.
* Properly check end of behavior device string for null terminator.
petejohanson
force-pushed
the
core/peripheral-behavior-invocation
branch
from
9081617
to
2d91ad8
Compare
* Add strlcpy from public domain version. * Leverage strlcpy to detect truncation of behavior dev strs, and log. * Use `offsetof` for cleaner detection on peripheral side.
With this change, behaviors can now have different "locality":
This should makes split power and RGB management work, at least for the basics.