Implement Observable POC #157
Conversation
Co-authored-by: Adin Ackerman <[email protected]>
src/observable.rs
Outdated
| pub trait IntoObservationToken: Sized + crate::Sealed { | ||
| type Peripheral; | ||
| fn into_ot(self) -> ObservationToken<Self::Peripheral>; | ||
| } |
There was a problem hiding this comment.
I had to do this since we can not do this the trait impl rules
impl<T: Observable> Into<ObservationToken<T>> for T {
fn into(self) -> ObservationToken<T> {
ObservationToken {
_p: PhantomData
}
}
}do you know if there is a better way?
There was a problem hiding this comment.
The most recent version of this concept (with a few name changes and being used in a brand new context) is here. The simple solution to this is to implement From rather than Into. This is a good idea in general (save some rare edge cases).
impl<P> From<P> for ObservationToken<P>
where
P: Observable,
{
fn from(_: P) -> Self {
Self { _p: PhantomData }
}
}Another option - which eliminates the need for calling .into() to directly pass a peripheral as an observation token - is to create a new trait called ObservationLock which is implemented by all P: Observable and ObservationTokens. Interfaces would thus hold impl ObservationLocks rather than concrete ObservationTokens. This is described in this gist.
The former is a solution I am putting to use right now in proto-hal, the latter I suspect is a better solution, but have not implemented quite yet.
Hope this helps!
There was a problem hiding this comment.
Maybe the ObservationLock trait is unneeded since the interfaces can accept an Into<ObservationToken<P>>. Clever!
There was a problem hiding this comment.
If we want the user to be able to destruct the consumer of the ObservationToken how does this affect things?
If the user passed an actual owned peripheral than would it not be best to just not convert it and let the consumer store the peripheral as is until it is destructed?
In that case would ObservationLock make more sense since that better signals what we are doing?
There was a problem hiding this comment.
Ah yes! That was why ObservationLock was good. Since the peripherals would store an impl ObservationLock, the release() would return back whatever was originally passed in.
There was a problem hiding this comment.
The former is a solution I am putting to use right now in proto-hal
Very interesting!
| fn observe<const N: usize>(self) -> (Observed<Self, N>, [ObservationToken<Self>; N]) { | ||
| ( | ||
| Observed { peripheral: self }, | ||
| core::array::from_fn(|_| ObservationToken { _p: PhantomData }), |
There was a problem hiding this comment.
I did some testing and determined this does not introduce any overhead. I recall that being a concern.
src/observable.rs
Outdated
| pub struct Observed<P, const OBSERVER_COUNT: usize> { | ||
| peripheral: P, | ||
| } | ||
|
|
||
| impl<P, const OBSERVER_COUNT: usize> Observed<P, OBSERVER_COUNT> { | ||
| /// Release the observation of this peripheral | ||
| /// | ||
| /// This returns the underlaying perpheral type. Since it is no longer | ||
| /// observed, you are once again free to do what you want with it. | ||
| pub fn release(self, _data: [ObservationToken<P>; OBSERVER_COUNT]) -> P { | ||
| self.peripheral | ||
| } | ||
| } |
There was a problem hiding this comment.
You should definitely constrain P to Observable!
There was a problem hiding this comment.
Sorry for dumb question, but why? The only way to construct an Observed is with a P that is Observable, right? Just curious :)
There was a problem hiding this comment.
Not dumb at all. In all of these cases we want P to be Observable. It may be true that right now it just so happens that the only way to encounter these interfaces is when P is Observable, but I would consider that to be a weak invariance given that we are actively making significant changes that could silently break things. Having these additional constraints may catch other design mistakes.
Up to you, but I prefer erring on the side of caution at the expense of additional verbosity.
There was a problem hiding this comment.
Thanks! Makes sense
examples/comp_w_dac.rs
Outdated
| @@ -59,7 +61,7 @@ fn main() -> ! { | |||
| // * 0V at p1 => 0% duty | |||
| // * VDDA at p1 => 100% duty | |||
| loop { | |||
| dac.set_value(val); | |||
| dac.as_mut().set_value(val); | |||
There was a problem hiding this comment.
Should we even go as far as implementing Deref{Mut} for Observed?
There was a problem hiding this comment.
Yep, Deref will be a really good QoL feature.
Thanks :) No, no tests at all yet |
Alternative to #138
@AdinAck is this somewhere close to what you discribed in #138 ?