Skip to content

Comments

Remove dependency on embassy-executor#594

Merged
tullom merged 12 commits intoOpenDevicePartnership:v0.2.0from
tullom:remove-embassy-executor-deps
Dec 2, 2025
Merged

Remove dependency on embassy-executor#594
tullom merged 12 commits intoOpenDevicePartnership:v0.2.0from
tullom:remove-embassy-executor-deps

Conversation

@tullom
Copy link
Contributor

@tullom tullom commented Nov 20, 2025

All of our services should be able to run agnostic of an async executor. While we use embassy, the ability to use tokio for testing in std environments should be supported.

All tasks that should be spawned in a thread have been moved to each subsystem's respective task namespace, so that users know to spawn all async functions within that namespace.

@tullom tullom requested review from a team as code owners November 20, 2025 21:49
@tullom tullom self-assigned this Nov 20, 2025
@tullom tullom added enhancement New feature or request BREAKING CHANGE Marks breaking changes labels Nov 20, 2025
@tullom tullom moved this to In progress in Embedded Controller Nov 20, 2025
@tullom tullom marked this pull request as draft November 20, 2025 21:58
@jeffglaum jeffglaum moved this from In progress to In review in Embedded Controller Nov 21, 2025
@tullom tullom force-pushed the remove-embassy-executor-deps branch from 9bae58c to f0bdca1 Compare November 21, 2025 23:37
@tullom tullom requested review from asasine and kurtjd November 21, 2025 23:38
@tullom tullom marked this pull request as ready for review November 21, 2025 23:43
Copy link
Contributor

@kurtjd kurtjd left a comment

Choose a reason for hiding this comment

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

Didn't scrutinize every detail fully but looks good to me. Though I'm curious if in the examples it's necessary to mark the tasks with the ! type. The compiler complained about a void/() return type?

@tullom
Copy link
Contributor Author

tullom commented Nov 22, 2025

Didn't scrutinize every detail fully but looks good to me. Though I'm curious if in the examples it's necessary to mark the tasks with the ! type. The compiler complained about a void/() return type?

No, however I think that there are unintended consequences if a task that should not terminate, terminates. It might lead to UB, which is why we should panic instead of silently failing.

@tullom tullom changed the base branch from main to v0.2.0 November 24, 2025 18:38
@jerrysxie jerrysxie force-pushed the remove-embassy-executor-deps branch from f0bdca1 to 73ce52f Compare November 24, 2025 18:48
@jerrysxie
Copy link
Contributor

@tullom since we will be making quite a few API breaking changes in v0.2.0, let's keep a running list of breaking changes so that we can publish a changelog later when it is merged back into main.

@tullom tullom merged commit de4622c into OpenDevicePartnership:v0.2.0 Dec 2, 2025
14 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Embedded Controller Dec 2, 2025
williampMSFT added a commit that referenced this pull request Dec 16, 2025
#594 moved the responsibility for declaring async tasks from
embedded-services to the application layer. To support this, some tasks
that needed to be generic over a type (which is not directly supported
by embassy) made into macros that emit a function that uses a concrete
type. The application layer is required to call that macro and then
implement a task that invokes the generated function.

However, in the case of the keyboard service, there appears to be a typo
causing the name of the type identifier being passed into the macro to
not match the name of the type identifier that is actually used in the
macro, resulting in a compilation error whenever a user tries to
actually invoke the macro. This change fixes the typo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BREAKING CHANGE Marks breaking changes enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants