rust-embedded / rust-embedded/cortex-m
Make SYST.has_wrapped() take &self
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 1k
- Forks
- 202
- Avg merge
- 6d 2h
- Merged PRs (30d)
- 2
Description
While I understand the intention in making SYST.has_wrapped() take &mut self because of the side effect, I believe the trouble this causes are bigger than any pain it aims to save.
For example, implementing a shared clock, because of &mut requirement one now has to use a mutex before accessing has_wrapped(). The locking operation infers a non-negligible and unwelcome CPU load especially in a time-sensitive and oft-called method.
The register operation csr.read() backing has_wrapped() being inherently atomic in it's operation guarantees that the side effect will always be observed by a single thread. Also, this side-effect is mostly non-deterministic, i.e. one does not "expect" a particular has_wrapped() return value in any situation. It's something that has to be unconditionally checked by every thread and will thus not lead to a race condition.
In a sense, this should be similar to reading an event from a lock-free queue. &mut is more about enforcing exclusivity than mutability. In the case of has_wrapped the exclusivity is unwarranted.
Thus I contend that has_wrapped() should be changed to take &self. It could also be made unsafe to make one think about how it works and retain the original intention behind the current &mut.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by locating SYST::has_wrapped() and the csr.read() operation it uses, then inspect its callers for assumptions about mutable access and thread safety. Evaluate whether changing the receiver to &self or making the method unsafe preserves the documented side effect, and add or update tests showing the intended behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- embedded-iot
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100