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 an atomic-check mutex for RTIC #17

Merged
merged 3 commits into from
Apr 20, 2021
Merged

Add an atomic-check mutex for RTIC #17

merged 3 commits into from
Apr 20, 2021

Conversation

ryan-summers
Copy link
Contributor

This PR adds an AtomicCheck "mutex" which does a simple atomic check to ensure that a bus collision does not occur and panics if one does. It is then the user's responsibility to ensure that collisions do not occur (e.g. by using RTIC to manage resources properly). This eliminates any need to disable interrupts and supports high levels of concurrency.

Note that I've currently feature-gated this behind cortex-m right now.

There's also an issue when targetting cortex M0 cores I believe (and any armv6 I think) because they don't provide atomic support.

fn create(v: BUS) -> Self {
Self {
bus: core::cell::UnsafeCell::new(v),
busy: core::sync::atomic::AtomicBool::from(false),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this currently won't work for Armv6. We can add another feature armv6 to add a shim layer for atomic operations. An example of this is in https://github.com/ryan-summers/shared-bus-rtic/blob/master/src/lib.rs#L153-L175 and was originally taken from bbqueue.

#[macro_export]
macro_rules! new_atomic_check {
($bus_type:ty = $bus:expr) => {{
let m: Option<&'static mut _> = $crate::cortex_m::singleton!(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is the only reason that the AtomicCheckMutex is gated by cortex-m. Is there another singleton type that we could use here instead?

@irwineffect
Copy link

What would a user need to do wrong in order for this to panic? Perhaps some documentation would be helpful with examples of the right and wrong way to manage resources?

@Rahix
Copy link
Owner

Rahix commented Sep 11, 2020

@ryan-summers, do you have an example of an RTIC application that uses this?

@ryan-summers
Copy link
Contributor Author

@ryan-summers, do you have an example of an RTIC application that uses this?

Yes, but the application is currently closed-source (will likely open-source within a number of months). I'm going on vacation for two weeks tomorrow, but can rig up an example project to demonstrate this one.

What would a user need to do wrong in order for this to panic? Perhaps some documentation would be helpful with examples of the right and wrong way to manage resources?

I should definitely add documentation for this - a thorough document describing the correct and incorrect way to do this is outlined in https://docs.rs/shared-bus-rtic/0.2.2/shared_bus_rtic/ - I'll port them over here. The gist is that all resources that are on the same bus need to be within a single container struct, which is an RTIC resource. This offloads the complexity of concurrency from shared-bus to RTIC and uses the atomic bool as a backup.

@Rahix
Copy link
Owner

Rahix commented Oct 23, 2020

[...], but can rig up an example project to demonstrate this one.

@ryan-summers, do you, by any chance, have such an example now? :) I'm a bit hesitant to merge this PR without knowing what dowstream use would look like.

@ryan-summers
Copy link
Contributor Author

@Rahix I've just gotten a sample project together at https://github.com/ryan-summers/shared-bus-example. I'll work on cleaning up the docs here now

@ryan-summers
Copy link
Contributor Author

It's also worth noting that this bus manager makes it safe to use SPI devices as well as long as the CS pin is contained within the shared resource structure

@Rahix
Copy link
Owner

Rahix commented Apr 20, 2021

Hey @ryan-summers,

I'm super sorry for forgetting about this PR once again... Somehow I didn't notice that you did share an example now and even added the documentation for it here.

I think I am going to merge it as it is now and we can deal with the outstanding issues in the future. In particular I would like to see this decoupled from cortex-m so it can work for other platforms as well (as you mentioned, though I don't know of an immediate solution) and to maybe reconsider how we should name this type.

Overall I like the approach, initially I was thinking that more of the boilerplate should be part of the macro, but in the end I prefer the way you designed it: It makes it much cleared to a reader what is going on.

@Rahix Rahix changed the title Adding atomic-check mutex for RTIC Add an atomic-check mutex for RTIC Apr 20, 2021
@Rahix Rahix merged commit 6732410 into Rahix:main Apr 20, 2021
@ryan-summers
Copy link
Contributor Author

No worries! I had honestly forgotten about this PR as well. I was recently looking at the cortex-m singleton and think it would actually be quite easy to re-implement here.

@Rahix
Copy link
Owner

Rahix commented Apr 20, 2021

The problem is that it still depends on cortex_m::interrupt::free() if I am not mistaken?

@ryan-summers
Copy link
Contributor Author

Ah yeah you're right, but that's still just a mutex, which we're using elsewhere in shared-bus, so I don't think that would be too onerous.

I guess the problem is we need a mutex for the mutex... It's a bit cyclical

@Rahix
Copy link
Owner

Rahix commented Apr 20, 2021

I was thinking we can maybe rig up something using another Atomic flag variable. Though this would be more generally useful (its own crate?) and maybe something like once_cell already has a clean solution which we can use. Needs more investigation :)

@ryan-summers
Copy link
Contributor Author

Something like https://github.com/ryan-summers/shared-bus-rtic/blob/master/src/lib.rs#L158-L196 (which was taken originally from bbqueue) really should be its own crate and there's been discussion on Matrix around this too - this code is getting copy-pasted a lot between crates.

@Rahix
Copy link
Owner

Rahix commented Apr 20, 2021

Yeah, that sounds like a good idea!

@Rahix Rahix linked an issue Apr 20, 2021 that may be closed by this pull request
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.

Is it possible to use this library with RTIC?
3 participants