esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
743 stars 207 forks source link

embassy time driver appears to be unsound #268

Closed TheButlah closed 1 year ago

TheButlah commented 1 year ago

I was attempting to use esp-wifi along with esp32c3-hal and was worried that esp wifi might need peripherals used by the embassy time driver.

To my pleasant surprise, I saw that initializing embassy appears to not need any ownership over the peripheral: https://github.com/esp-rs/esp-hal/blob/096ff3439de8c63aae9e9f4e29c3f3e56dd03b0f/esp32c3-hal/examples/embassy_hello_world.rs#L41

However, I was confused when I realized that I was able to use the embassy timer even without calling the init method. To further my confusion, I found this in the hal code: https://github.com/esp-rs/esp-hal/blob/096ff3439de8c63aae9e9f4e29c3f3e56dd03b0f/esp-hal-common/src/embassy/time_driver_systimer.rs#L21-L26 I am not knowledgeable on the safety of this, but from my inexperienced perspective, this implies that esp-hal is lying to me about the peripherals its using. Instead of passing the alarm peripherals explicitly into the embassy::init() method, they are being manufactured into existance via the dark arts of unsafe.

My question is: Is the current implementation unsound? Can I use alarm0 in esp-wifi or something else that needs the alarm? Is this leaving me vulnerable to race conditions (not just data races!) in the state of the peripheral?

MabezDev commented 1 year ago

Yeah, this is probably an oversight, thanks for bringing it up! We can just change the signature of init depending on the feature flag enabled.