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

Implement Observable POC #157

Draft
wants to merge 12 commits into
base: staged-pac
Choose a base branch
from

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Dec 21, 2024

Alternative to #138

@AdinAck is this somewhere close to what you discribed in #138 ?

Co-authored-by: Adin Ackerman <adinackerman@gmail.com>
Comment on lines 4 to 7
pub trait IntoObservationToken: Sized + crate::Sealed {
type Peripheral;
fn into_ot(self) -> ObservationToken<Self::Peripheral>;
}
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 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@AdinAck AdinAck Dec 21, 2024

Choose a reason for hiding this comment

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

Maybe the ObservationLock trait is unneeded since the interfaces can accept an Into<ObservationToken<P>>. Clever!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The former is a solution I am putting to use right now in proto-hal

Very interesting!

Copy link
Contributor

@AdinAck AdinAck left a comment

Choose a reason for hiding this comment

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

This looks great! Have you run any integration tests?

src/observable.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@AdinAck AdinAck left a comment

Choose a reason for hiding this comment

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

Every occurrence of P should be P: Observable.

src/observable.rs Outdated Show resolved Hide resolved
@@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we even go as far as implementing Deref{Mut} for Observed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, Deref will be a really good QoL feature.

@usbalbin
Copy link
Contributor Author

This looks great! Have you run any integration tests?

Thanks :) No, no tests at all yet

Comment on lines -139 to +142
impl Channel<stm32::$adc> for $pin {
impl<P> Channel<stm32::$adc> for P
where P: stasis::EntitlementLock<Resource = $pin>
{
Copy link
Contributor Author

@usbalbin usbalbin Dec 28, 2024

Choose a reason for hiding this comment

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

This does not work. Both Channel and EntitlementLock (or rather P i suppose) are from outside this crate. There are also errors about conflicting implementations with the other opamps below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the simplest solution is to make a local trait like AdcChannel which requires embedded_hal::adc::Channel and then it would work.

I'm not very familiar with what you're doing here though. I'll think on it.

You could also do concrete implementations since this is macro generated anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could also do concrete implementations since this is macro generated anyway.

Yeah that would probably be the simplest solution. I'll give it a go

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.

2 participants