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

embassy time driver appears to be unsound #268

Closed
TheButlah opened this issue Nov 20, 2022 · 1 comment · Fixed by #290
Closed

embassy time driver appears to be unsound #268

TheButlah opened this issue Nov 20, 2022 · 1 comment · Fixed by #290
Assignees
Labels
bug Something isn't working

Comments

@TheButlah
Copy link
Contributor

TheButlah commented Nov 20, 2022

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:

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:

embassy_time::time_driver_impl!(static DRIVER: EmbassyTimer = EmbassyTimer {
alarms: Mutex::new([ALARM_STATE_NONE; ALARM_COUNT]),
alarm0: unsafe { Alarm::<_, 0>::conjure() },
alarm1: unsafe { Alarm::<_, 1>::conjure() },
alarm2: unsafe { Alarm::<_, 2>::conjure() },
});

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?

@TheButlah TheButlah changed the title embassy feature appears to be unsound embassy time driver appears to be unsound Nov 20, 2022
@MabezDev
Copy link
Member

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.

@MabezDev MabezDev self-assigned this Nov 21, 2022
@jessebraham jessebraham added the bug Something isn't working label Nov 21, 2022
MabezDev added a commit that referenced this issue Dec 8, 2022
- Rename timg feature to timg0 to better refect which TG is being used
- Use the time_driver::TimerType in the signature of init to fix #268
- Update examples
- Fix CI features
- Add timg0 cfg to build.rs
jessebraham pushed a commit that referenced this issue Dec 8, 2022
- Rename timg feature to timg0 to better refect which TG is being used
- Use the time_driver::TimerType in the signature of init to fix #268
- Update examples
- Fix CI features
- Add timg0 cfg to build.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants