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

Core event struct/data refactor #615

Merged

Conversation

petejohanson
Copy link
Contributor

This came up from freedback on #547, where @joelspadin had noted it would be nice to be able to copy data from event to type used locally.

A big part of the issue with doing that was the way event manager required that event types mix their data and the first "header" field.

This refactor removes that need, by having the event manager code generate a wrapper/containing type for each event type, leaving the data payload "pure data".

is_ and cast_ prefixes updated appropriately, and we've also made a typedef zmk_event_t to be passed to subscription callbacks.

Once we're happy, I'll rebase #547

* Separate header and data struct for the event.
* Remove duplicate struct in split code.
@petejohanson petejohanson changed the title Core/position event data refactor Core event struct/data refactor Jan 18, 2021
@petejohanson petejohanson force-pushed the core/position-event-data-refactor branch 4 times, most recently from 4478f59 to 803361e Compare January 18, 2021 07:05
* Make it easier to use *just* event payloads by defining the data,
  and then having event manager macros generate "wrapper structs"
* Improve is_*/cast_* APIs to hide details of full event struct.
* Create `zmk_event_t` typedef to pass to event handlers.
* Bring event names inline w/ consistent `zmk_` prefix.
@petejohanson petejohanson force-pushed the core/position-event-data-refactor branch from 803361e to 420bf65 Compare January 18, 2021 07:10
@innovaker
Copy link
Contributor

The last time typedef came up in a PR, I think we decided to avoid them for structs?

@petejohanson
Copy link
Contributor Author

The last time typedef came up in a PR, I think we decided to avoid them for structs?

This is one intended to be treated as opaque, you shouldn't ever be extracting fields from it directly.

Copy link
Contributor

@innovaker innovaker left a comment

Choose a reason for hiding this comment

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

Had a look through and didn't spot anything glaring.

The event names realignment could probably be a separate commit but it's controversial.

I'll defer to @joelspadin to comment on the change in approach as he'll have a better understanding.

E2E tests passed.

@innovaker innovaker added core Core functionality/behavior of ZMK refactor labels Jan 18, 2021
Copy link
Collaborator

@joelspadin joelspadin left a comment

Choose a reason for hiding this comment

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

This looks good to me. I added a few ideas, but I'm not sure how good of ideas they are.

app/include/zmk/event_manager.h Show resolved Hide resolved
app/include/zmk/event_manager.h Show resolved Hide resolved

#define ZMK_EVENT_RELEASE(ev) zmk_event_manager_release((struct zmk_event_header *)ev);
#define ZMK_EVENT_FREE(ev) k_free((void *)ev);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is probably worth documenting somewhere that events must not contain any dynamically allocated memory, or it will leak.

app/src/behaviors/behavior_hold_tap.c Outdated Show resolved Hide resolved
@petejohanson petejohanson force-pushed the core/position-event-data-refactor branch from 2f9b431 to beae120 Compare January 19, 2021 19:24
Copy link
Collaborator

@joelspadin joelspadin left a comment

Choose a reason for hiding this comment

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

Looks good to me with the caveat that the missing return in keymap_listener should probably be fixed, but that's a preexisting issue.

app/src/keymap.c Show resolved Hide resolved
app/src/behaviors/behavior_hold_tap.c Outdated Show resolved Hide resolved
* Use a single `as_foo` generated function to conditionally
  return a certain event type from a generic `zmk_event_t*`
  pointer.
@petejohanson petejohanson force-pushed the core/position-event-data-refactor branch from beae120 to 2bc7df1 Compare January 20, 2021 04:58
@petejohanson
Copy link
Contributor Author

@innovaker Had some good feedback from @joelspadin, with just some minor tweaks since his last review. Ready for final review from you.

Copy link
Contributor

@innovaker innovaker left a comment

Choose a reason for hiding this comment

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

e2e tests passed

@petejohanson petejohanson merged commit 3368a81 into zmkfirmware:main Jan 20, 2021
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.

3 participants