Better Mutex

[Rahix]

We're currently relying on the mutex type from bare-metal which is actually being removed upstream so now is a good time to think about adding a similar mechanism here and maybe improving it. For starters, I'd want the mutex to actually hand out &mut _ references because 99% of use-cases need this.

It would be interesting to explore the concept of a critical-section handle a bit more:

• Maybe ISRs should optionally get a cs-handle as an argument as nothing could interrupt them?
• Even further, maybe main should get a cs-handle in the beginning? Interrupts are disabled so it would be sound. Then, later an interrupt::enable() function should consume the handle when enabling interrupts for the first time. Is there a problem I'm missing?

[couchand]

I've been thinking about your latter two questions a lot, and came to roughly the same conclusion. I've been avoiding writing cs-based APIs for some things based on the (almost certainly misguided) concern that I already know interrupts are disabled, but it's exactly the two cases you laid out here.

One note: I've never used them, but I'm dimly aware that you can have re-entrant ISRs by manually re-enabling interrupts in an ISR, so the function to enable interrupts by consuming a CS should be callable there, too, but I think that shakes out for free.

I'd want the mutex to actually hand out &mut _ references because 99% of use-cases need this.

I would recommend making the API follow exactly from other places rather than trying to be too clever. Principle of least surprise, do one thing and do it well, and all that.

[Rahix]

Actually, just noticed something:

interrupt::free(|cs1| {
    interrupt::free(|cs2| {
        interrupt::hypothetical_enable(cs1);

        some_mutex.lock(&cs2);
    });
});

I don't think this is going to work unfortunately ...

[couchand]

Or even:

fn main(cs1: CriticalSection) -> ! {
    interrupt::free(|cs2| {
        interrupt::hypothetical_enable(cs1);

        some_mutex.lock(&cs2);
    });

    // ..
}

But maybe it would be a new privileged type, such as:

pub struct CriticalPrologue {
    cs: CriticalSection,
}

impl Deref for CriticalPrologue {
    type Target = CriticalSection;
    fn deref(&self) -> &Self::Target { &self.cs }
}

pub fn hypothetical_enable(cp: CriticalPrologue) {
    // ...
}

[Rahix]

Yeah, different kinds of tokens was also what I was thinking about but not sure if that's a good idea ... Also, it really feels like Deref-abuse to do that but I don't see a different solution here :/

[Rahix]

Actually, no:

fn main(other_cs: CriticalPrologue) {
    interrupt::free(|cs| {
        interrupt::enable(other_cs);

        // now what
    });
}

[couchand]

Would this be too heavy-handed a solution?

pub struct CriticalPrologue<'a>(&'a CriticalSection);

impl<'a> Deref for CriticalPrologue<'a> {
    type Target = CriticalSection;
    fn deref(&self) -> &Self::Target { &self.0 }
}

mod interrupt {
    pub fn free<T, F: FnOnce(&CriticalSection) -> T + 'static>(f: F) -> T {
        f(&CriticalSection)
    }
}

The error message isn't great but it would seem to provide the right guarantee.

Playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6ac82256d10cf44359bef766ef4a1a71

[Rahix]

When commenting out the bad main in your example, it still does not compile?

TBH, I don't see how there even could be a borrow-checker based solution here, I think this problem is inherent. If I'm not mistaken, this is pretty much analogous to the reason why RefCell cannot be statically checked.

And while there might be other dirty tricks to prevent a function call below a certain other function, I don't think that's worth it. The common pattern in most embedded rust applications, to just do all initialization inside an interrupt::free() works well enough IMO.

[couchand]

When commenting out the bad main in your example, it still does not compile?

Oh my, I really thought I had checked that. No wonder I was so confused how that worked -- it didn't.

The common pattern in most embedded rust applications, to just do all initialization inside an interrupt::free() works well enough IMO.

All this brouhaha to try and save a single cli. What a waste! :)

[K4LCIFER]

Note: rust-embedded/bare-metal#46

[Rahix]

@K4LCIFER, yeah, true. As of #195, we already enable the dependency on critical-section by default so I think it is sensible to go ahead and drop bare-metal entirely now. Would you be willing to send a PR?

[K4LCIFER]

Would you be willing to send a PR?

Unfortunately, I don't think my rust skills are sufficient to tackle something of that magnitude yet. But I could potentially take a look at some point.

[Rahix]

If you want to give it a shot, I am happy to guide you through it :)

All you have to do is essentially remove bare-metal from Cargo.toml and then work through the compiler errors. We may need to remove some functions or change them to use critical-section types instead. You can ask whenever you are unsure about anything.

The nice thing about Rust is that once it compiles again, you can be pretty sure that you got everything right ;)