feature(split): add support for sensors from peripheral #1841

petejohanson · 2023-06-19T05:19:16Z

This commit adds a new GATT characteristics on the peripheral side and wires it up to read sensor values. The central side subscribes to this new characteristics and replays sensor values on its side.

This is building off the awesome work in #728 but updated to leverage the sensor refactors recently merged in #1039

burgessa23 · 2023-06-22T16:37:21Z

working fine on Hillside 52

caksoylar · 2023-06-22T17:38:28Z

Should we update the docs in this PR or do it in another one after merging?

Places to update:

Intro table footnote: https://github.com/zmkfirmware/zmk/blob/main/docs/docs/intro.md?plain=1#L29, https://github.com/zmkfirmware/zmk/blob/main/docs/docs/intro.md?plain=1#L47
Encoders feature page: https://github.com/zmkfirmware/zmk/blob/main/docs/docs/features/encoders.md?plain=1#L8

petejohanson · 2023-06-23T07:29:32Z

Should we update the docs in this PR or do it in another one after merging?

Places to update:

* Intro table footnote: https://github.com/zmkfirmware/zmk/blob/main/docs/docs/intro.md?plain=1#L29, https://github.com/zmkfirmware/zmk/blob/main/docs/docs/intro.md?plain=1#L47

* Encoders feature page: https://github.com/zmkfirmware/zmk/blob/main/docs/docs/features/encoders.md?plain=1#L8

Good catch. Updated here in a new commit.

p3likan0 · 2023-06-24T15:46:25Z

Working on sofle v2!

Thanks for this amazing job. I hope it arrives to master :)

mike1808 · 2023-06-26T18:42:38Z

I tested on my keyboard and there is one big issue: whenever I on a layer the keyboard registers both mappings: both for the current layer and for the default layer.

Upd: after flashing the build from the CI it works perfectly fine. Something is wrong in my local tooling.

PhanKhang · 2023-06-29T15:43:52Z

Tested on SofleV2.1 with nano v2 wired (I don't have any batteries yet). Everything works. Both encoders are working.

petejohanson · 2023-06-29T16:25:54Z

I tested on my keyboard and there is one big issue: whenever I on a layer the keyboard registers both mappings: both for the current layer and for the default layer.

Upd: after flashing the build from the CI it works perfectly fine. Something is wrong in my local tooling.

Fixed, with some help from Mike on Discord.

DiogoDoreto · 2023-06-29T22:06:50Z

Just tested it and can confirm this is working with Waterfowl after cherry-picking my config update from DiogoDoreto@fb7c374

Thanks for your work!

narze · 2023-07-08T13:46:56Z

Tested on Splitkb Aurora Lily58 and it works, thank you!

larryare · 2023-07-22T06:58:35Z

Works on my custom split keyboard flawlessly.

mike1808 · 2023-07-22T08:32:57Z

I'm curious to know what is this pr blocked on? Does it need a review?

petejohanson · 2023-07-22T08:33:51Z

I'm curious to know what is this pr blocked on? Does it need a review?

Yes, the main thing remaining is review, which will happen when others on the team have bandwidth to do so.

lesshonor

I can report that this PR at least partially fixes a bug introduced in 295ed83, after which encoder events in this config began triggering too often.

I still get 4 calls to volume up or down per-step if the configuration uses resolution (as this shield unfortunately still does in main), but switching to steps/triggers-per-rotation works as expected.

I currently have no way to test the split part of this.

e: After another look this morning, applying the changes in 72ca2fa alone seems to have solved the problem. I am not sure if extracting that commit from the feature PR will break anything. Will have to see if I can rope anyone with a fully-assembled keyboard into testing the bugfix.

D0hnut5 · 2023-08-09T21:17:01Z

e: After another look this morning, applying the changes in 72ca2fa alone seems to have solved the problem. I am not sure if extracting that commit from the feature PR will break anything. Will have to see if I can rope anyone with a fully-assembled keyboard into testing the bugfix.

Per lesshonor's roping I tested this on a fully assembled Pillbug Waka. It fixed the issues I was experiencing: every third encoder detent would not send a code and the encoder would not work on layers.

nobiliana · 2023-08-10T02:10:27Z

I can report that this PR at least partially fixes a bug introduced in 295ed83, after which encoder events in this config began triggering too often.

I still get 4 calls to volume up or down per-step if the configuration uses resolution (as this shield unfortunately still does in main), but switching to steps/triggers-per-rotation works as expected.

I currently have no way to test the split part of this.

e: After another look this morning, applying the changes in 72ca2fa alone seems to have solved the problem. I am not sure if extracting that commit from the feature PR will break anything. Will have to see if I can rope anyone with a fully-assembled keyboard into testing the bugfix.

Also as per Lesshonor, this PR was tested on a Pillbug with bb40 shield. Encoder performance was showing 2-5 activations per detent; after reflashing with this PR, I am getting the expected 1 activation per detent (1 in 15 seem to be missed, but that could be hardware) using Steps and Trigger.

Split functionality was not tested.

joelspadin

This looks pretty good to me, except I think we should have better error checking when reading data into the sensor_event structs.

joelspadin · 2023-08-27T15:18:38Z

app/include/zmk/split/bluetooth/service.h

+int zmk_split_bt_position_released(uint8_t position);
+int zmk_split_bt_sensor_triggered(uint8_t sensor_number,


Minor: sensor_event uses sensor_index, while this uses sensor_number. Are these the same thing? If so, making the name consistent would be good.

This also appears to be all the same data as a sensor_event. Could you just pass a pointer to that struct instead?

So.... struct sensor_event was intended as a data exchange format, which is why it is packed. I'm hesitant to have a data exchange struct also be used for public API like this. Thoughts?

joelspadin · 2023-08-27T15:38:22Z

app/src/split/bluetooth/central.c

+    LOG_DBG("[SENSOR NOTIFICATION] data %p length %u", data, length);
+
+    struct sensor_event sensor_event;
+    memcpy(&sensor_event, data, MIN(length, sizeof(sensor_event)));


It would be a good idea to verify that length >= offsetof(struct sensor_event, channel_data). Otherwise, it would be possible to not write anything to channel_data_size, and then the array size will be a garbage value which may cause other code to read past the end of the array.

Also, we are truncating the array data if given more channels than we support, but we aren't clamping channel_data_size accordingly. This means we're protecting against writing past the end of the array, but not reading past it.

Added a check there.

app/src/split/bluetooth/central.c

This commit adds a new GATT characteristics on the peripheral side and wires it up to read sensor values. The central side subscribes to this new characteristics and replays sensor values on its side. Co-authored-by: Peter Johanson <[email protected]>

* Don't accept data for the same behavior on multiple layers more than once, to avoid duplicate/extraneous triggers.

petejohanson added enhancement New feature or request sensors bluetooth Bluetooth related items encoders labels Jun 19, 2023

petejohanson requested review from joelspadin and Nicell June 19, 2023 05:19

petejohanson self-assigned this Jun 19, 2023

petejohanson requested a review from a team as a code owner June 19, 2023 05:19

petejohanson force-pushed the stephen/split-encoder branch 2 times, most recently from 0612d49 to 0da2c45 Compare June 21, 2023 00:12

petejohanson mentioned this pull request Jun 21, 2023

[bounty] feature(split): add split-side encoder support #1743

Closed

petejohanson mentioned this pull request Jun 24, 2023

feature(split): add split-side encoder support #728

Closed

This comment was marked as resolved.

Sign in to view

petejohanson force-pushed the stephen/split-encoder branch 2 times, most recently from 585c7f6 to fb19a3f Compare June 28, 2023 21:46

petejohanson removed the request for review from Nicell June 29, 2023 22:30

petejohanson force-pushed the stephen/split-encoder branch from fb19a3f to 72ca2fa Compare July 22, 2023 08:45

lesshonor approved these changes Aug 9, 2023

View reviewed changes

joelspadin requested changes Aug 27, 2023

View reviewed changes

stephen and others added 3 commits August 27, 2023 14:04

fix(docs): Update docs about split encoder support

e15b4fe

fix(sensors): Only accept data once per behavior.

1672524

* Don't accept data for the same behavior on multiple layers more than once, to avoid duplicate/extraneous triggers.

petejohanson requested a review from joelspadin August 27, 2023 21:08

petejohanson force-pushed the stephen/split-encoder branch from 72ca2fa to 1672524 Compare August 27, 2023 21:13

joelspadin approved these changes Aug 28, 2023

View reviewed changes

petejohanson merged commit a92a496 into zmkfirmware:main Aug 28, 2023
82 checks passed

peterpf added a commit to peterpf/zmk-config that referenced this pull request Aug 28, 2023

Assign functionality to second encoder because of zmkfirmware/zmk#1841

ae3035f

DiogoDoreto mentioned this pull request Oct 9, 2023

feat(shields): add right encoders support to Waterfowl #1961

Merged

lesshonor mentioned this pull request Aug 13, 2024

Feature: Encoder support #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(split): add support for sensors from peripheral #1841

feature(split): add support for sensors from peripheral #1841

petejohanson commented Jun 19, 2023

burgessa23 commented Jun 22, 2023

caksoylar commented Jun 22, 2023

petejohanson commented Jun 23, 2023

This comment was marked as resolved.

p3likan0 commented Jun 24, 2023

mike1808 commented Jun 26, 2023 •

edited

Loading

PhanKhang commented Jun 29, 2023

petejohanson commented Jun 29, 2023

DiogoDoreto commented Jun 29, 2023

narze commented Jul 8, 2023

larryare commented Jul 22, 2023

mike1808 commented Jul 22, 2023

petejohanson commented Jul 22, 2023

lesshonor left a comment •

edited

Loading

D0hnut5 commented Aug 9, 2023

nobiliana commented Aug 10, 2023

joelspadin left a comment

joelspadin Aug 27, 2023

petejohanson Aug 27, 2023

joelspadin Aug 27, 2023

petejohanson Aug 27, 2023

		int zmk_split_bt_position_released(uint8_t position);
		int zmk_split_bt_sensor_triggered(uint8_t sensor_number,

feature(split): add support for sensors from peripheral #1841

feature(split): add support for sensors from peripheral #1841

Conversation

petejohanson commented Jun 19, 2023

burgessa23 commented Jun 22, 2023

caksoylar commented Jun 22, 2023

petejohanson commented Jun 23, 2023

This comment was marked as resolved.

p3likan0 commented Jun 24, 2023

mike1808 commented Jun 26, 2023 • edited Loading

PhanKhang commented Jun 29, 2023

petejohanson commented Jun 29, 2023

DiogoDoreto commented Jun 29, 2023

narze commented Jul 8, 2023

larryare commented Jul 22, 2023

mike1808 commented Jul 22, 2023

petejohanson commented Jul 22, 2023

lesshonor left a comment • edited Loading

Choose a reason for hiding this comment

D0hnut5 commented Aug 9, 2023

nobiliana commented Aug 10, 2023

joelspadin left a comment

Choose a reason for hiding this comment

joelspadin Aug 27, 2023

Choose a reason for hiding this comment

petejohanson Aug 27, 2023

Choose a reason for hiding this comment

joelspadin Aug 27, 2023

Choose a reason for hiding this comment

petejohanson Aug 27, 2023

Choose a reason for hiding this comment

mike1808 commented Jun 26, 2023 •

edited

Loading

lesshonor left a comment •

edited

Loading