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 RwLock to embassy-sync #3932

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

AlixANNERAUD
Copy link

@AlixANNERAUD AlixANNERAUD commented Feb 28, 2025

Fixes #1394

The implementation is very similar to Mutex

@AlixANNERAUD AlixANNERAUD marked this pull request as draft February 28, 2025 14:53
* Implement `RawRwLock` trait with methods for read and write locking
* Implement `RawRwLockImpl` struct with atomic state and waker
* Implement `RawRwLockImpl::lock_read`, `RawRwLockImpl::try_lock_read`, and `RawRwLockImpl::unlock_read` methods
* Implement `RawRwLockImpl::lock_write`, `RawRwLockImpl::try_lock_write`, and `RawRwLockImpl::unlock_write` methods
@AlixANNERAUD AlixANNERAUD marked this pull request as ready for review February 28, 2025 22:28
Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I have some question specifically around the critical section raw rw lock, which I don't understand how can work in a single executor.

let mut state = self.state.borrow_mut();

while *state & WRITER != 0 {
// Spin until the writer releases the lock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't spinning effectively prevent this to be used within the same executor as it would deadlock here? Same in the lock_write().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, my initial approach wasn’t ideal. I completely refactored it by using a Mutex as a locking mechanism for the inner state and adhered to the same overall pattern as Mutex.

@AlixANNERAUD AlixANNERAUD requested a review from lulf March 17, 2025 19:09
@mschnell1
Copy link

mschnell1 commented Mar 19, 2025

This still seems open.
I supposedly would like to use it some day soon. Are there any severe problems ?

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give an example for needing to use the map()? Won't

let g = lock.read().await;
do_stuff(g.deref());

allow you to 'calling methods on the contents of the RwLockReadGuard without moving out of the guard' as stated in the comment?

@mschnell1
Copy link

mschnell1 commented Mar 19, 2025

To manage the rather complex state of an embedded device I drafted this code:

    #[derive(Debug, Default, Clone)]
    pub struct DisplayState {
       ...
    }

    use crate::api::api::RwLock;  // switching std / embassy
    use lazy_static::lazy_static;
    lazy_static! {
        static ref DISPLAY_STATE: RwLock<DisplayState> = RwLock::new(DisplayState::default());
    }

    pub fn global_check_display_field(a: &DisplayField) -> bool {
        let ds = DISPLAY_STATE.read().unwrap();
        ds.check_display_field(a)
    }

    impl PartialEq for DisplayField {
        fn eq(&self, other: &Self) -> bool {
            if !(self.content == other.content) {
                return false;
            };
            if !global_check_display_field(self) || !global_check_display_field(other) {
                return false;
            }
            true
        }
    }

The many information sections in DISPLAY_STATE can be read by certain "threads" of the firmware and written by others. hence RwLock seemed the way to go.

Right now I don't see the necessity of using more than the.read()and .write() functions.

@AlixANNERAUD
Copy link
Author

AlixANNERAUD commented Mar 21, 2025

I've just transposed the mutex pattern to RwLock for consistency but I can remove map guard if it is not necessary.

@mschnell1
Copy link

Thanks a lot
looking forward to the release....

@lulf
Copy link
Member

lulf commented Mar 31, 2025

I've just transposed the mutex pattern to RwLock for consistency but I can remove map guard if it is not necessary.

Yeah, please remove it. If there is a use case for it, it can be added later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

embassy-sync: Add RwLock
3 participants