search

Rahix / shared-bus

Crate for sharing buses between multiple devices
Apache License 2.0
129 stars 34 forks source link

Add an atomic-check mutex for RTIC #17

Closed ryan-summers closed 3 years ago

ryan-summers commented 3 years ago

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.

irwineffect commented 3 years ago

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 commented 3 years ago

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

ryan-summers commented 3 years ago

@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 commented 3 years ago

[...], 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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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.

ryan-summers commented 3 years ago

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 commented 3 years ago

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

ryan-summers commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Yeah, that sounds like a good idea!