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

Make sure systimer period is loaded and reset #322

Closed
wants to merge 1 commit into from

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Oct 31, 2023

This fixes some obscure problem that was first reliably discovered in #300 but I think I also have seen it on ESP32-C3's static-ip example (but there it magically disappeared)

While I cannot really explain what is happening (I thought the problem was the scheduler starting to switch tasks while not all needed interrupts were enabled - which is not the problem) this seems to reliably eliminate the problem and is also an improvement generally

Turned out alarm0.set_period only sets the period in systimer's registers but doesn't load them by writing to timer_compX_load.
(Probably something that should be addressed ultimately in the HAL)

When a yield_task is executed, this means that the switched to task might not be given a full time-slice and also triggers the bug seen in the linked PR.

This PR additionally disables interrupts globally until all the interrupts are configured (which on itself already would fix the problem)

I also found a way to reproduce the problem on current main (at the time of writing):

  • in tasks.rs comment #[cfg(coex)]
  • in preemt/mod.rs make sure to allocate three stacks
  • run BLE example (or probably any other) with both, wifi and ble, enabled: cargo run --release --example ble --features=ble,wifi

let alarm0 = systimer.into_periodic();
alarm0.set_period(TIMER_DELAY.into());
// make sure the period is loaded and reset - should be done in esp-hal ultimately
Copy link
Member

Choose a reason for hiding this comment

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

How hard is the fix in esp-hal, why don't we try and fix it there before the next release?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because Jesse said the next release is today :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the fix would be trivial - just pull the code out of configure and call it there and in set_period .... but what Daniel said
I can create a PR in esp-hal and if it gets in that is fine and we remove it here - if not it will stay here a few weeks

Copy link
Member

Choose a reason for hiding this comment

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

If we can get a PR merged in the next few hours then we can include it in today's releases. Otherwise will have to wait until next round.

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Oct 31, 2023

Closing this and replacing it with #324
Apparently, the HAL already does the timer_compX_load .... the fact that this alone already fixed things is probably because it changed timing slightly

@bjoernQ bjoernQ closed this Oct 31, 2023
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.

4 participants