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

Add documentation for esp-hal-embassy on underlying embassy-time-driver's tick-hz #2985

Open
doge-gif opened this issue Jan 16, 2025 · 7 comments
Labels
documentation Improvements or additions to documentation

Comments

@doge-gif
Copy link

Bug description

When using esp-hal-embassy, one cannot set embassy-time's tick rate.
This is due to esp-hal-embassy hardcoding the value, which embassy-time says not to do in libraries.

When using embassy-time from libraries, you should not enable any tick-* feature, to allow the end user or the driver to pick.

To Reproduce

  1. Start a blank project
  2. Add following to cargo.toml's dependency:
esp-hal-embassy = { version = "^0.6.0", features = ["esp32-c3"] }
# anything other than tick-hz-1_000_000
embassy-time = { version = "^0.4.0", features = ["tick-hz-2_000_000"] }

minimal repro: TBD

Expected behavior

We should be able to (within a reasonable extent) change the underlying tick rate.

Environment

  • Target device: ESP32-C3, but really any device
  • Crate name and version: esp-hal-embassy 0.6.0

Notes

I haven't deciphered the hal codebase yet (I'm a newcomer to rust overall), so I don't know if this is due to a hardware limitation, or just simple oversight.

@doge-gif doge-gif added bug Something isn't working status:needs-attention This should be prioritized labels Jan 16, 2025
@Dominaezzz
Copy link
Collaborator

Doesn't esp-hal count as the end user in this case? 🤔

@MabezDev
Copy link
Member

When using embassy-time from libraries, you should not enable any tick-* feature, to allow the end user or the driver to pick.

Whilst true for most libraries, this library also contains the time driver that is configured to fire at 1000hz, which is why we set it so. Setting anything other than this, and using the same driver is wrong.

So why do you want to change the time driver's hz?

@MabezDev MabezDev removed the status:needs-attention This should be prioritized label Jan 16, 2025
@bugadani
Copy link
Contributor

bugadani commented Jan 16, 2025

We also don't support configuring generic timer queue capacities using their respective embassy-time features. This is at least party intentional, as we have a configuration system in place that is a bit more flexible than a list of cargo features.

We technically could make the tick rate configurable, but that would mean potentially changing the resolution of unrelated timers as well.

Doesn't esp-hal count as the end user in this case? 🤔

You could argue that way as well :) There are chips that only support a single frequency, and you can't configure those as well. The existence of these features don't imply they are useful as well for every system.

@doge-gif
Copy link
Author

Thanks for the inputs!

Doesn't esp-hal count as the end user in this case? 🤔

We can say so - or if the project structure allows, we can have a set of predefined usable frequencies with compile-time checking.
Though as I said, I haven't read the code (and I'm quite foreign to rust) so I'm not sure if it's possible.

this library also contains the time driver that is configured to fire at 1000hz, which is why we set it so.

Just to confirm, is it 1MHz or 1kHz? Because the current value is 1MHz (1_000_000), and seems oddly high for an interrupt timer and not a timer for time-keeping.
My suspicion was 1MHz was the timer for timekeeping resolution (1us), and 1kHz is the tick rate derived from it, used for RTOS purposes.

We also don't support configuring generic timer queue capacities using their respective embassy-time features. This is at least party intentional, as we have a configuration system in place that is a bit more flexible than a list of cargo features.
We technically could make the tick rate configurable, but that would mean potentially changing the resolution of unrelated timers as well.

Makes sense. And I do understand that it will be a PITA for the hal to change the base timer.


I forgot to mention my use case - I was looking for a way to get a 100ns resolution time counter in embassy for precise time measurements. (meaning 10MHz in embassy-time)
But that was a miscalculation on my side - 1us was an order of magnitude good enough.


I guess I should change the title to somewhere in the lines of "add notes on embassy-time's tick rate when used with esp-hal-embassy" ?

@Dominaezzz
Copy link
Collaborator

esp-hal-embassy is also entirely optional, you can write an embassy-time-driver as well using the timers in the hal and tweak it how ever you see fit.
I guess the down side there is you miss out on the executor stuff. (Copy+Paste is a decent workaround)

@bugadani
Copy link
Contributor

Just to confirm, is it 1MHz or 1kHz? Because the current value is 1MHz (1_000_000), and seems oddly high for an interrupt timer and not a timer for time-keeping.
My suspicion was 1MHz was the timer for timekeeping resolution (1us), and 1kHz is the tick rate derived from it, used for RTOS purposes.

The timer interrupts are not periodic. The underlying timers are ticking at 1MHz, the time driver is firing interrupts as timers expire, and only when timers expire.

@doge-gif
Copy link
Author

Thanks, I'll update the title then!

Looks like I've got a lot to learn!
At first I disliked how everything was hidden from the end-user, compared to traditional RTOSes, but I'm REALLY enjoying esp32 + embassy. It's such a breeze using async in an embedded controller!

Sincere thanks to all of you who made this literally awesome development experience possible!

@doge-gif doge-gif changed the title esp-hal-embassy shouldn't hard-code embassy-time-driver's tick-hz Add documentation for esp-hal-embassy on underlying embassy-time-driver's tick-hz Jan 16, 2025
@MabezDev MabezDev added documentation Improvements or additions to documentation and removed bug Something isn't working labels Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Todo
Development

No branches or pull requests

4 participants