-
Notifications
You must be signed in to change notification settings - Fork 2
Add basic Embassy support #53
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
base: main
Are you sure you want to change the base?
Conversation
|
@qwandor I must ask to wait with a merge as I've just discovered some strange issues with this PR: a couple of fresh new GD32F130C8 are starting to reboot after a few alarm cycles on any timer/channel instance. I tried to fully erase my GD32F130G8's flash, and uploaded an example, now it's reproducing on this chip as well. Moreover, openocd is not able to program the GD32F130G8 anymore, however probe-rs is working just fine. Alas, no meaningful ideas what's going on so far, I'll keep investigation. |
qwandor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird! Good luck. I haven't finished reviewing this yet, but some comments in the meantime...
|
@qwandor Thank you for pointing those out, I've fixed the issues you mentioned. Also I realized that my MCUs are restarting due to the free watchdog timer timeout. According to the GD32F1x0 user manual, the watchdog timer is always running which makes me confused why the examples were running before without chip reset. |
src/embassy/time_driver.rs
Outdated
| #[cfg(feature = "time-driver-tim14")] | ||
| const ALARM_COUNT: usize = 1; | ||
|
|
||
| fn timer() -> &'static RegisterBlock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function shouldn't be safe, because it can allow creating aliases to timer register blocks which may already be owned by other parts of the program.
Or better, rather than constructing the reference here, use the Timer passed in to EmbassyTimeDriver::init, as that way you know you have ownership of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm passing it to the driver's EmbassyTimeDriver::init without borrowing, so it owns the Timer, hence, it's safe to use internally within the driver (as far as someone else will not decide to access it in the unsafe manner of course).
If it's still a must to change this approach, I will ask your advice on how to do it properly...I could not simply add it as the driver's struct field as I need to create a static instance of it using the time_driver_impl macro before initialization...Option looks like an overkill and most likely would not be beneficial for performance, and making it global would require some mutex/atomic pointer for safety guarantees which will also affect the code performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit to your branch to do this safely. I'm pretty sure atomics will be just as fast as non-atomics on a single-core system like this, so I think performance is unlikely to be an issue. Likewise accessing a CriticalSectionMutex is free as long as you are already in a critical section.
src/embassy/time_driver.rs
Outdated
| fn now(&self) -> u64 { | ||
| let r = timer(); | ||
| let period = self.period.load(Ordering::Relaxed); | ||
| compiler_fence(Ordering::Acquire); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using Ordering::Acquire for the period load operation on the previous line have an equivalent effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though Acquire usually slower AFAIK, and we are using the now method a lot as a core of our "ticker", so I've tried to follow the Embassy implementation for STMs and use the compile-time assurance counter read will not be reordered.
a63c4e6 to
87345fd
Compare
87345fd to
eb1492a
Compare
|
Hi @qwandor |
Cleaned up some other minor issues too.
|
@Serhii-the-Dev Let me know if my changes work for you, if so I'll go ahead and merge this. |
Resolves #49
Embassy time driver implementation is mostly a copy-pasta of the STM32 driver except it's not implementing RTC functionality required for low-power/deep-sleep scenarios in order to keep it simple (AFAIK current HAL implementation also lacks of RTC support).
This PR brings a set of new features:
embassy- enables Embassy support in generaltime-driver-tim<1, 2, 14>- enables Embassy time driver support for a specified general-purpose timer instanceBoth features are required to use the Embassy executor, so any of the
time-driver-tim<1, 2, 14>features enables theembassyfeature in general.Despite that the driver uses
unsafeaccess to the selected timer's registry, in order to guard an access to aTIMERxperipheral, the driver initialization requires to move atimer<1, 2, 14>so it couldn't be used anywhere else causing undefined behavior, ex:After that the
p.timer1could not be moved, to create aTimer::timer1, for example.Please take into account that Embassy support comes with a price, as a binary relying on the
embassyfeature and using theasyncexecutor would have additional ~2KiB added to it's size (also an extra memory would be required for the tasks arena):