Skip to content

Conversation

@bugadani
Copy link
Contributor

@bugadani bugadani commented Jul 2, 2025

This PR should enable both #3728 and #3670 without actually integrating them into the executor.

@bugadani bugadani marked this pull request as ready for review July 2, 2025 14:11
@bugadani bugadani force-pushed the executor-idle branch 5 times, most recently from a254602 to 96ccdb0 Compare July 2, 2025 14:40
@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 2, 2025

I guess for s.th. like #3728 embassy-executor's rtos-trace feature (or embassy-rs/embassy#4033) would be a better fit.

While the above might also work for #3670 this "explicit" callback approach feels better suited there

@bugadani
Copy link
Contributor Author

bugadani commented Jul 2, 2025

Yeah the only advantage of this approach is that this is free for executors that don't opt into it (while tracing has a runtime cost to check if the right executor is called), and it's not called "tracing" when you want to inject an esp-wifi yield :)

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 3, 2025

Code looks good - just wondering if it's worth to add a test for this

@bugadani
Copy link
Contributor Author

bugadani commented Jul 3, 2025

What would you like to be tested?

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 3, 2025

What would you like to be tested?

Nothing fancy - just something exercising run_with_callbacks making sure it works the same way as run when the callbacks do nothing - as said I'm not sure it's worth it - approving and leaving that up to you

@MabezDev MabezDev added this pull request to the merge queue Jul 3, 2025
@bugadani bugadani removed this pull request from the merge queue due to a manual request Jul 3, 2025
@bugadani bugadani added this pull request to the merge queue Jul 3, 2025
Merged via the queue into esp-rs:main with commit 583be70 Jul 3, 2025
30 checks passed
@bugadani bugadani deleted the executor-idle branch July 3, 2025 09:45
@Szybet Szybet mentioned this pull request Jul 3, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants