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

Refactor events API to avoid heap/dynamic allocations #1722

Merged

Conversation

petejohanson
Copy link
Contributor

@petejohanson petejohanson commented Mar 24, 2023

Ok, this is a semi-major core refactor to address one of the major nits I had with the initial design for the event manager API I added to ZMK, mainly that it depending on dynamic memory allocation for alloc/free of the event structs. As a result, we need to use some large, arbitrary heap pool in ZMK to be sure we have enough allocated for whatever odd combination of events get raised/captured in the system.

This PR moves to stack allocated event structures, and change the expectation that anyone wanting to capture an even should copy it into a locally allocated storage for it's use as needed. See updates to hold-tap, combos, etc that demonstrate this kind of change.

This potentially disrupts any open behavior PRs, so need to consider the timing of the merge of this, but want to get this reviewed for the general approach now, since this I believe is ready, after I found and fixed the final bug in combos I'd introduced.

@petejohanson petejohanson added core Core functionality/behavior of ZMK refactor labels Mar 24, 2023
@petejohanson petejohanson self-assigned this Mar 24, 2023
@petejohanson petejohanson changed the title Refactor events API to avoid heal/dynamic allocations Refactor events API to avoid heap/dynamic allocations Mar 24, 2023
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch 3 times, most recently from be2dc89 to 554ae26 Compare March 25, 2023 17:11
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch 6 times, most recently from 0e01ac3 to f40ff67 Compare April 8, 2023 07:50
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch 2 times, most recently from 1e5b9fe to 51025ac Compare April 16, 2023 19:22
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch from 51025ac to fd29240 Compare May 17, 2023 06:19
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch 3 times, most recently from a827778 to 286a9ba Compare September 7, 2023 06:21
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch 4 times, most recently from 3d64dcb to daba812 Compare October 6, 2023 19:43
@petejohanson petejohanson marked this pull request as ready for review October 6, 2023 21:00
@petejohanson petejohanson requested a review from a team as a code owner October 6, 2023 21:00
@caksoylar
Copy link
Contributor

I have been testing this full time for the last week on a split with keymap including hold-taps, combos and BLE profile change events with no issues. (Also mouse keys as a bonus, with this patch to get it working on this PR.)

Among changes covered here, I haven't tested with sticky keys, key toggles, key repeat, layer state and USB status change events, WPM events, sensor/encoders.

@petejohanson
Copy link
Contributor Author

I have been testing this full time for the last week on a split with keymap including hold-taps, combos and BLE profile change events with no issues. (Also mouse keys as a bonus, with this patch to get it working on this PR.)

Among changes covered here, I haven't tested with sticky keys, key toggles, key repeat, layer state and USB status change events, WPM events, sensor/encoders.

I've just flashed to my revxlp RP2040 last night with a keymap that includes combos, sticky keys, caps word, etc. and will daily that with this change the next week as well. First day of use was uneventful.

app/include/zmk/event_manager.h Outdated Show resolved Hide resolved
app/include/zmk/event_manager.h Outdated Show resolved Hide resolved
app/src/behaviors/behavior_hold_tap.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_hold_tap.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_hold_tap.c Outdated Show resolved Hide resolved
app/src/combo.c Outdated Show resolved Hide resolved
app/src/combo.c Outdated Show resolved Hide resolved
app/src/combo.c Outdated Show resolved Hide resolved
app/src/combo.c Outdated Show resolved Hide resolved
app/src/combo.c Outdated Show resolved Hide resolved
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch 9 times, most recently from cd578eb to d35e98f Compare December 17, 2023 01:11
app/src/behaviors/behavior_sticky_key.c Outdated Show resolved Hide resolved
app/src/combo.c Outdated Show resolved Hide resolved
* Move to local/stack allocated event API that doesn't require
  dynamic allocation/freeing.
* Disable heap, we no longer use alloc/free unless using LVGL.
* Tons of refactors all over to account for the new event approach.
@petejohanson petejohanson force-pushed the core/event-manager-stack-only branch 5 times, most recently from 28b5a6d to da46109 Compare January 14, 2024 06:52
#define ZMK_EVENT_RAISE(ev) \
((__ASSERT((uint8_t *)&(ev).header - (uint8_t *)&ev == 0, \
"header must be first element of event")), \
zmk_event_manager_raise(&(ev).header));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
zmk_event_manager_raise(&(ev).header));
zmk_event_manager_raise(&(ev).header))

If these should be usable in expressions like return ZMK_EVENT_RAISE(ev), then they shouldn't end in a semicolon. That will prevent other things that should work like

result = ZMK_EVENT_RAISE(ev) ? OK : ERROR;

and it also allows

int test(void) {
  return ZMK_EVENT_RAISE(ev)  // no semicolon
}

to compile, which just feels weird.

zmk_event_manager_raise_after((zmk_event_t *)ev, &zmk_listener_##mod);
((__ASSERT((uint8_t *)&(ev).header - (uint8_t *)&ev == 0, \
"header must be first element of event")), \
zmk_event_manager_raise_after(&(ev).header, &zmk_listener_##mod));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
zmk_event_manager_raise_after(&(ev).header, &zmk_listener_##mod));
zmk_event_manager_raise_after(&(ev).header, &zmk_listener_##mod))

#define ZMK_EVENT_RELEASE(ev) zmk_event_manager_release((zmk_event_t *)ev);
((__ASSERT((uint8_t *)&(ev).header - (uint8_t *)&ev == 0, \
"header must be first element of event")), \
zmk_event_manager_raise_at(&(ev).header, &zmk_listener_##mod));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
zmk_event_manager_raise_at(&(ev).header, &zmk_listener_##mod));
zmk_event_manager_raise_at(&(ev).header, &zmk_listener_##mod))


#define ZMK_EVENT_FREE(ev) k_free((void *)ev);
#define ZMK_EVENT_RELEASE(ev) zmk_event_manager_release((zmk_event_t *)&ev);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define ZMK_EVENT_RELEASE(ev) zmk_event_manager_release((zmk_event_t *)&ev);
#define ZMK_EVENT_RELEASE(ev) zmk_event_manager_release((zmk_event_t *)&ev)

Also, I think we want to add the assert and use &ev.header instead of a cast here too?

Comment on lines 80 to 81
((__ASSERT((uint8_t *)&(ev).header - (uint8_t *)&ev == 0, \
"header must be first element of event")), \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend creating a macro like

#define ZMK_ASSERT_IS_EVENT(ev) \
    (__ASSERT((uint8_t *)&(ev).header - (uint8_t *)&ev == 0,                                      \
               "header must be first element of event"))

to simplify this. (Feel free to come up with a better name for this, as "is event" isn't strictly accurate since it only checks the header member, not the type.)

* Fix up some lingering events API tweaks for heap-less event manager.
@petejohanson petejohanson merged commit 644feeb into zmkfirmware:main Jan 14, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants