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 esp-wifi to ease using external task scheduler #3106

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Feb 6, 2025

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

This is my attempt at using Ariel OS threads instead of the slim scheduler that ships with esp-wifi.

Mostly, this refactors the preempt & timer modules:

  • the timer module is split into

    • time, providing the generic time functions)
    • radio, this has the previous radio isr setup stuff
    • preempt::timer, containing the task switching related timer setup
  • all scheduler setup / teardown is now handled in preempt::setup() and preempt::disable().

  • the radio ISRs are configured independently from that (the radio module now exposes the setup_radio_isr()/disable_radio_isr()

Basically, all task switching / scheduling code is split into preempt.
With that in place, replacing those guts with e.g., the Ariel scheduler is actually quite simple, by just disabling the preempt module and providing the functions elsewhere.

This is WIP, please ignore the OptionalPeripherals commit, and Im just importing the ariel code into preempt_custom.rs for local testing. I expect that integration to change. I've tested this with the old scheduler from esp-hal, and the hacked-in Ariel preempt_custom.rs`, on an s3 devkit.

I hope the refactoring and moving code is not too intimidating.
I was struggling a bit before the refactor. E.g., setup_timer_isr() was a bit overloaded, and it took a while to see what's used by the scheduler and whats needed elsewhere.
IIUC, at least this is not user-facing.

Let me know what you think, if this direction might be doable!

update I've rebased this on current main, and dropped the ariel support for now.

Testing

Describe how you tested your changes.

// Time keeping
pub const TICKS_PER_SECOND: u64 = 1_000_000;

/// Current systimer count value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function was identical for riscv and xtensa. the comments are a mix. I don't know which statements are true for either architecture.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, this change will probably make it simpler to just throw all this out in favour of esp_hal::time::* :)

@kaspar030 kaspar030 force-pushed the v0.23.1+ariel-os+preeempt-refactor+ariel-threads branch from 8dd4e1c to 92139a7 Compare February 6, 2025 22:00
@bjoernQ bjoernQ added the skip-changelog No changelog modification needed label Feb 7, 2025
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 7, 2025

Very nice! Lots of things I wanted to do for quite some time - from my side this looks already mergeable

@kaspar030 kaspar030 marked this pull request as ready for review February 7, 2025 09:11
@kaspar030 kaspar030 changed the title refactor esp-wifi to ease using external task scheduler (wip) refactor esp-wifi to ease using external task scheduler Feb 7, 2025
@kaspar030 kaspar030 force-pushed the v0.23.1+ariel-os+preeempt-refactor+ariel-threads branch from 92139a7 to 02e25cc Compare February 7, 2025 09:29
Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Thanks!

@kaspar030
Copy link
Contributor Author

Should I rebase this to main?

@bugadani
Copy link
Contributor

bugadani commented Feb 7, 2025

No need, it will merge cleanly, just needs a +1 from someone else, ideally @bjoernQ :)

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but will let Bjoern have the final say before merging. Thanks!

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

Thanks!

@bjoernQ bjoernQ enabled auto-merge February 7, 2025 09:53
@kaspar030
Copy link
Contributor Author

Thanks for the quick review!

@bjoernQ bjoernQ added this pull request to the merge queue Feb 7, 2025
Merged via the queue into esp-rs:main with commit 235ee62 Feb 7, 2025
27 checks passed
@kaspar030 kaspar030 deleted the v0.23.1+ariel-os+preeempt-refactor+ariel-threads branch February 7, 2025 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants