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

Replace specific proxy types I2cProxy and AdcProxy by generic type Proxy #25

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

Conversation

DrTobe
Copy link

@DrTobe DrTobe commented Nov 18, 2021

I have seen that the different proxy types (I2cProxy, SpiProxy and AdcProxy) are essentially the same types. The only difference for the SpiProxy is that should only be initialized with the NullMutex. So I guess this can be reduced to only two types there.

Also, the SpiProxy::_u field had no apparent use so I removed it. Have I overlooked something there?

This is WIP because I have not adjusted the documentation and removed the acquire_adc method yet.

@Rahix
Copy link
Owner

Rahix commented Nov 18, 2021

Also, the SpiProxy::_u field had no apparent use so I removed it. Have I overlooked something there?

Please read #8 for why this is necessary right now. This is also essentially the reason why we currently have different types for each bus - as you noticed this wouldn't be necessary if they are all the same. I have plans on how to build a better solution for this, but this is still in the works...

@DrTobe
Copy link
Author

DrTobe commented Nov 18, 2021

I have understood that the SpiProxy must be its own type which is !Send + !Sync. Therefore, I had only joined I2cProxy and AdcProxy into the generic Proxy type and left the SpiProxy. In general, this just makes the distinction between buses that can be shared between threads/contexts and those which can not.

Still, I do not see the use of the _u: core::marker::PhantomData<*mut ()>.

@Rahix
Copy link
Owner

Rahix commented Nov 21, 2021

I have understood that the SpiProxy must be its own type which is !Send + !Sync. Therefore, I had only joined I2cProxy and AdcProxy into the generic Proxy type and left the SpiProxy.

Ah, sorry, I did not see this during my initial read through your changeset. Joining just the "good" proxies is okay. However, we should really keep the old names available as to not require a breaking release. Please add type-aliases for I2cProxy and AdcProxy so code relying on these names still compiles. Similarly, keep the acquire_i2c() and acquire_adc() methods. You can mark all these as deprecated to encourage people to switch.

Still, I do not see the use of the _u: core::marker::PhantomData<*mut ()>.

That's what makes the SpiProxy !Send. Without it, you can "safely" move a proxy to a different thread still (when constructed with a mutex type which allows this).

…uire_adc method aliases; clean up documentation
@DrTobe
Copy link
Author

DrTobe commented Nov 22, 2021

keep the old names available as to not require a breaking release

Sounds reasonable, done in the commit before

That's what makes the SpiProxy !Send

I suspected this was the intention. But I guess it is not required because the acquire_spi method is only implemented on the BusManager<NullMutex>:

impl<T> BusManager<crate::NullMutex<T>> {
    pub fn acquire_spi<'a>(&'a self) -> crate::SpiProxy<'a, crate::NullMutex<T>> {
        crate::SpiProxy {
            mutex: &self.mutex,
        }
    }
}

So there will only ever be SpiProxy<'a, crate::NullMutex<T>> instances which are !Send + !Sync because of

// from the docs
impl<'a, M> Send for SpiProxy<'a, M>
where
    M: Sync

impl<'a, M> Sync for SpiProxy<'a, M>
where
    M: Sync, 

and the NullMutex is !Sync.

@DrTobe DrTobe changed the title WIP: Replace specific proxy types I2cProxy and AdcProxy by generic type Proxy Replace specific proxy types I2cProxy and AdcProxy by generic type Proxy Nov 24, 2021
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.

None yet

2 participants