Skip to content
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(split): Transfer arbitrary tagged data from central to peripheral #2036

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ReFil
Copy link
Contributor

@ReFil ReFil commented Nov 24, 2023

Currently the only mechanism for data transfer for secondary features (underglow and backlighting primarily) is through the behaviour invocation process, this can only hander user initiated changes, so automatic state changes are missed and the halves state can go out of sync if the central is changed with the peripheral disconnected. Some new features currently in PR initalise a new characteristic for central-> peripheral data transfer however for infrequently updated things abstracting it away means there's not a variety of implementations in the split source files.
This introduces a new mechanism by which a data tag, size value and up to 16 bytes of data can be transferred from central to peripheral(s). I tried 32 bytes but ran into some MTU limitation. The data tags are handled in an enum and, in a similar manner to the zephyr sensor channels, additional tags can be defined separately from the current ones and passed to this system without issue. There are currently 5 tags defined, one each for underglow and backlighting, one for the upcoming HID indicators work and one for keymap state (which could be used for passing layer information to a peripheral display widget). the fifth is a marker to show the start of the unallocated ranges for custom usages. That way any future custom uses automatically get "bumped down" as new upstream uses get added seamlessly without any conflicts.
This also implements this feature for both RGB underglow and single colour backlighting for a more seamless lighting experience.

@ReFil ReFil requested a review from a team as a code owner November 24, 2023 18:48
@ReFil
Copy link
Contributor Author

ReFil commented Dec 19, 2023

Rebased and changed the HID Indicators code to use this system too

@ReFil
Copy link
Contributor Author

ReFil commented Jan 2, 2024

This should resolve #1494 for RGB and backlighting

@ReFil
Copy link
Contributor Author

ReFil commented Jan 10, 2024

This has been rebased to solve the merge conflict as well as adding the ability to send keymap layer state to the peripherals for peripheral display widgets/rgb indications

@ReFil
Copy link
Contributor Author

ReFil commented Jan 10, 2024

This could be expanded to include a peripheral side listener for the remaining global locality behaviours (reset, bootloader and ext power) with the intention of superceding the locality system, keen to hear other people's opinions on this idea

@ReFil
Copy link
Contributor Author

ReFil commented Feb 12, 2024

Rebased after Zephyr 3.5 update, need to do some testing to confirm nothing broke

@ReFil
Copy link
Contributor Author

ReFil commented Mar 25, 2024

Tested all 4 data channels and confirmed, RGB state, backlight state, HID indicators and keymap state all get successfully transmitted to the peripheral , a display widget or custom RGB underglow implementation can subscribe to the zmk_split_data_xfer_event event and process information from the central. automatic state changes are transferred as well. By disabling *_AUTO_OFF_IDLE on the peripheral for RGB and backlighting the lights stay in sync throughout all state changes on the central be it automated or manual.
The zmk_split_peripheral_status_event event is now triggered correctly however it usually triggers before the connection has had time to process. As a result the central often fails to update the peripheral on reconnection after a power cycle. Not sure how to solve this in a good way, would appreciate any suggestions on this

b3n-l added a commit to b3n-l/zmk that referenced this pull request Jun 18, 2024
commit c0a3d49
Author: ReFil <[email protected]>
Date:   Mon Feb 12 13:44:56 2024 +0000

    fix(split): Update events and init function

commit f417665
Author: ReFil <[email protected]>
Date:   Wed Jan 10 13:39:18 2024 +0000

    feat(split): Add keymap state sending

commit 574ee4e
Author: ReFil <[email protected]>
Date:   Tue Dec 19 12:23:06 2023 +0000

    feat(split): Use split data transfer for hid indicators

commit a7fb050
Author: ReFil <[email protected]>
Date:   Fri Nov 24 18:32:47 2023 +0000

    feat(backlight): Use data transfer framework

commit 8d953ab
Author: ReFil <[email protected]>
Date:   Thu Nov 23 23:17:26 2023 +0000

    feat(rgb): Use data transfer framework

commit 5fc212f
Author: ReFil <[email protected]>
Date:   Thu Nov 23 23:13:35 2023 +0000

    fix(split): change peripheral to use work

commit ef8ab95
Author: ReFil <[email protected]>
Date:   Tue Nov 21 11:55:45 2023 +0000

    feat(split): Add data transfer event

commit ff7e0b7
Author: ReFil <[email protected]>
Date:   Tue Nov 21 09:40:56 2023 +0000

    feat(split): central to peripheral communication

commit 6ed14e9
Author: ReFil <[email protected]>
Date:   Mon Nov 20 13:03:56 2023 +0000

    feat(split): raise status changed events on central

commit 2ee76be
Author: German Gutierrez <[email protected]>
Date:   Mon May 13 23:43:35 2024 +0200

    fix(soft_off): central waits 100ms in split if hold_time enabled

commit f0b20c1
Author: Joel Spadin <[email protected]>
Date:   Fri Oct 13 13:07:55 2023 -0500

    feat(boards): Add nRF52 high voltage DC/DC config

    Added a Kconfig option to enable SOC_DCDC_NRF52X_HV for nice_nano_v2
    and mikoto. According to Nordic's documentation, the DC/DC regulator is
    more efficient than the LDO regulator, so this is enabled by default.

    The following boards do not support this mode and were not changed:

    - nice_nano
    - nice60
    - nrfmicro_11, nrfmicro_13
    - nrf52840_m2
    - bluemicro840

    I could not find schematics to confirm whether the following boards
    support this mode:

    - bt60_v1, bt60_v2
    - bt65_v1
    - bt75_v1
    - corneish_zen_v1, corneish_zen_v2
    - pillbug
    - puchi_ble_v1
    - s40nc

commit 8f5c7bb
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Feb 26 05:52:14 2024 +0000

    chore(deps): bump pre-commit/action from 3.0.0 to 3.0.1

    Bumps [pre-commit/action](https://github.com/pre-commit/action) from 3.0.0 to 3.0.1.
    - [Release notes](https://github.com/pre-commit/action/releases)
    - [Commits](pre-commit/action@v3.0.0...v3.0.1)

    ---
    updated-dependencies:
    - dependency-name: pre-commit/action
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <[email protected]>

commit 7d1f84e
Author: Horu <[email protected]>
Date:   Tue May 14 03:47:33 2024 +0700

    chore: fix typos in various places

commit 4dfc45d
Author: ReFil <[email protected]>
Date:   Fri May 3 19:17:09 2024 +0100

    feat(docs): Document example toggle-mode implementation

    ---------

    Co-authored-by: Cem Aksoylar <[email protected]>

commit 2423136
Author: ReFil <[email protected]>
Date:   Fri May 3 08:22:05 2024 +0100

    fix(boards): Fix pulls on ZMK uno toggle switch

    The devicetree pulls always add on to the extra pulls configured by toggle mode, so these should not have pulls defined in the devicetree. Saved ~200uA avg on another board with a 3t toggle switch

commit af90882
Author: Peter Johanson <[email protected]>
Date:   Wed May 1 11:01:32 2024 -0700

    fix: Initialize sideband kscan in APPLICATION.

    * In order to be sure the rest of the system is fully ready before
      intializing, because init may result in immediate events being
      triggered when used with toggle direct kscan inner devices.

commit d1ad347
Author: German Gutierrez <[email protected]>
Date:   Mon Apr 29 18:22:40 2024 +0200

    fix: shortening keymap_soft_off behavior node

    * Shorten the soft off node in order for it to work across splits.

commit 0d3a4b7
Author: Jarryd Tilbrook <[email protected]>
Date:   Thu Apr 25 22:26:26 2024 +0800

    fix(underglow): Correctly set underglow state

    This fixes a bug introduced in zmkfirmware#2244

commit 4d56685
Author: Pablo Martínez <[email protected]>
Date:   Thu Apr 25 10:55:42 2024 +0200

    fix(rgb): auto-off logic

commit f4a070a
Author: Sadek Baroudi <[email protected]>
Date:   Sun Apr 21 12:37:47 2024 -0700

    fix(boards): nrf boards missing SPI in pinctrl and dtsi, requiring users to manually define in their shield definitions if they wanted to use SPI

commit 16e92cf
Author: Peter Johanson <[email protected]>
Date:   Thu Apr 18 06:38:46 2024 +0000

    fix(behaviors): Add multiple soft-off instances properly.

    * Properly pass the node id for the unique
      soft-off behavior instance when defining it.

commit e22bc76
Author: Keeley Hoek <[email protected]>
Date:   Sun Apr 7 07:05:51 2024 -0400

    fix(hid): Correct off-by-one buffer overflow with NKRO

commit a9021de
Author: Cem Aksoylar <[email protected]>
Date:   Tue Apr 9 10:25:33 2024 -0700

    fix(docs): Add wakeup-source to split new shield example

commit dfc6dc8
Author: Cem Aksoylar <[email protected]>
Date:   Tue Feb 27 11:50:54 2024 -0800

    fix(docs): Make clear the matrix transform example is incomplete

commit 7a51a46
Author: Cem Aksoylar <[email protected]>
Date:   Tue Feb 27 11:41:46 2024 -0800

    feat(docs): Add pointer to shields folder in new shield docs

commit 849eca7
Author: Xudong Zheng <[email protected]>
Date:   Fri Feb 23 16:59:48 2024 -0500

    refactor(underglow): fix uninitialized variable warning
@skewty
Copy link

skewty commented Jun 29, 2024

It has been over 3 months and it seems no review has taken place (don't see any review comments). Is something still pending?

@ReFil
Copy link
Contributor Author

ReFil commented Jul 1, 2024

It has been over 3 months and it seems no review has taken place (don't see any review comments). Is something still pending?

From my perspective it's ready for a preliminary review

@skewty
Copy link

skewty commented Jul 3, 2024

@petejohanson you were assigned. Will you be the person doing the review?

@infused-kim
Copy link
Contributor

@skewty I am not one of the maintainers, but have been involved in the project for a while and have submitted a few PRs, so here are my 2 cents...

(Which are my opinions alone and do not represent the views of the project or maintainers. I could also be completely wrong about everything here adn they might disagree with it all.)

The zmk maintainer team is very small, the userbase is quite big and the amount of PRs submitted is a lot. There is no way they could review everything and schedule each PR based on the time the PR has been open.

I imagine they have an internal roadmap of the improvements they want to get in and not all the PRs match that roadmap.

On top of that, each feature has a cost, because most of the time the original person submitting it, won't be maintaining it. Updating zephyr, the underlying RTOS, frequently requires a lot of modifications to the code base. Once a feature is merged, all of that work is dumped on the maintainers.

Additionally, a lot of features are submitted by developers who are not familiar with zephyr and zmk. So many features work, but are not implemented in the correct and maintainable way. I'm not saying it's the case with this PR, but it's just one of the things that must be considered.

So, considering all of this, I would recommend you learn how to maintain your own fork with all of the PRs you want to use. It's really not very difficult. Urob's guide is a great intro:
https://gist.github.com/urob/68a1e206b2356a01b876ed02d3f542c7

That's the beauty of open source. You can be independent and include all teh features you want without relying on the maintainers prioritizing the features you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request split
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants