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.

Sign up for GitHub

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 11 commits into
base: staged-pac
Choose a base branch
from
Draft

Implement Observable POC #157

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
312436d
Implement Observable POC
usbalbin Dec 21, 2024
845448e
Fix comp_w_dac example
usbalbin Dec 21, 2024
d1c6c22
Use ObservationLock
usbalbin Dec 22, 2024
46d43fe
Add more trait bounds
usbalbin Dec 22, 2024
1e332f9
impl Deref for Observed
usbalbin Dec 22, 2024
fa03e3b
Add Observed things to opamp
usbalbin Dec 22, 2024
97fd4e4
Add rough doc of Observable::observe
usbalbin Dec 22, 2024
c9c5695
Update doc
usbalbin Dec 23, 2024
34aa3c6
Add example of using pin.observe
usbalbin Dec 23, 2024
595723c
Fix opamp for g431 etc.
usbalbin Dec 23, 2024
8fa58c9
Fix example
usbalbin Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions examples/comp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use rt::entry;

#[entry]
fn main() -> ! {
use hal::comparator::{ComparatorExt, ComparatorSplit, Config, Hysteresis, RefintInput};
use hal::comparator::{refint_input, ComparatorExt, ComparatorSplit, Config, Hysteresis};
use hal::gpio::GpioExt;
use hal::prelude::OutputPin;
use hal::rcc::RccExt;
Expand All @@ -31,16 +31,16 @@ fn main() -> ! {

let pa1 = gpioa.pa1.into_analog();
let pa0 = gpioa.pa0.into_analog();
let comp1 = comp1.comparator(&pa1, pa0, Config::default(), &rcc.clocks);
let comp1 = comp1.comparator(pa1, pa0, Config::default(), &rcc.clocks);
let comp1 = comp1.enable();

// led1 pa1 will be updated manually when to match comp1 value
let mut led1 = gpioa.pa5.into_push_pull_output();

let pa7 = gpioa.pa7.into_analog();
let comp2 = comp2.comparator(
&pa7,
RefintInput::VRefintM12,
pa7,
refint_input::VRefintM12,
Config::default()
.hysteresis(Hysteresis::None)
.output_inverted(),
Expand Down
120 changes: 77 additions & 43 deletions src/comparator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use crate::gpio::{
use crate::gpio::gpioc::{PC0, PC1};
use crate::gpio::gpioe::{PE7, PE8};
use crate::gpio::gpiof::PF1;
use crate::observable::IntoObservationToken;
use crate::rcc::{Clocks, Rcc};
use crate::stm32::{COMP, EXTI};

Expand Down Expand Up @@ -140,7 +141,7 @@ pub enum Hysteresis {

/// Comparator positive input
pub trait PositiveInput<C> {
fn setup(&self, comp: &C);
fn setup(comp: &mut C);
}

/// Comparator negative input
Expand All @@ -153,21 +154,21 @@ pub trait NegativeInput<C> {
/// Does this input rely on dividing Vrefint using an internal resistor divider
///
/// This is only relevant for `RefintInput` other than `RefintInput::VRefint`
fn use_resistor_divider(&self) -> bool;
const USE_RESISTOR_DIVIDER: bool = false;

fn setup(&self, comp: &C);
fn setup(comp: &mut C);
}

macro_rules! positive_input_pin {
($COMP:ident, $pin_0:ident, $pin_1:ident) => {
impl PositiveInput<$COMP> for &$pin_0<Analog> {
fn setup(&self, comp: &$COMP) {
impl PositiveInput<$COMP> for $pin_0<Analog> {
fn setup(comp: &mut $COMP) {
comp.csr().modify(|_, w| w.inpsel().bit(false));
}
}

impl PositiveInput<$COMP> for &$pin_1<Analog> {
fn setup(&self, comp: &$COMP) {
impl PositiveInput<$COMP> for $pin_1<Analog> {
fn setup(comp: &mut $COMP) {
comp.csr().modify(|_, w| w.inpsel().bit(true));
}
}
Expand Down Expand Up @@ -208,11 +209,7 @@ macro_rules! negative_input_pin_helper {
impl NegativeInput<$COMP> for $input {
const USE_VREFINT: bool = false;

fn use_resistor_divider(&self) -> bool {
false
}

fn setup(&self, comp: &$COMP) {
fn setup(comp: &mut $COMP) {
comp.csr().modify(|_, w| unsafe { w.inmsel().bits($bits) });
}
}
Expand Down Expand Up @@ -245,30 +242,54 @@ negative_input_pin! {
COMP7: PD15<Analog>, PB12<Analog>,
}

#[derive(Copy, Clone, Eq, PartialEq)]
pub enum RefintInput {
pub mod refint_input {
/// VRefint * 1/4
VRefintM14 = 0b000,
#[derive(Copy, Clone)]
pub struct VRefintM14;

/// VRefint * 1/2
VRefintM12 = 0b001,
#[derive(Copy, Clone)]
pub struct VRefintM12;

/// VRefint * 3/4
VRefintM34 = 0b010,
#[derive(Copy, Clone)]
pub struct VRefintM34;

/// VRefint
VRefint = 0b011,
#[derive(Copy, Clone)]
pub struct VRefint;
macro_rules! impl_vrefint {
($t:ty, $bits:expr, $use_r_div:expr) => {
impl super::RefintInput for $t {
const BITS: u8 = $bits;
const USE_RESISTOR_DIVIDER: bool = $use_r_div;
}

impl crate::observable::Observable for $t {}
impl crate::Sealed for $t {}
};
}

impl_vrefint!(VRefintM14, 0b000, true);
impl_vrefint!(VRefintM12, 0b001, true);
impl_vrefint!(VRefintM34, 0b010, true);
impl_vrefint!(VRefint, 0b011, false);
}

pub trait RefintInput {
const BITS: u8;
const USE_RESISTOR_DIVIDER: bool;
}

macro_rules! refint_input {
($($COMP:ident, )+) => {$(
impl NegativeInput<$COMP> for RefintInput {
impl<REF: RefintInput> NegativeInput<$COMP> for REF {
const USE_VREFINT: bool = true;
const USE_RESISTOR_DIVIDER: bool = <REF as RefintInput>::USE_RESISTOR_DIVIDER;

fn use_resistor_divider(&self) -> bool {
*self != RefintInput::VRefint
}

fn setup(&self, comp: &$COMP) {
fn setup(comp: &mut $COMP) {
comp.csr()
.modify(|_, w| unsafe { w.inmsel().bits(*self as u8) });
.modify(|_, w| unsafe { w.inmsel().bits(<REF as RefintInput>::BITS) });
}
}
)+};
Expand All @@ -289,11 +310,7 @@ macro_rules! dac_input_helper {
impl<ED> NegativeInput<$COMP> for &dac::$channel<{ dac::$MODE }, ED> {
const USE_VREFINT: bool = false;

fn use_resistor_divider(&self) -> bool {
false
}

fn setup(&self, comp: &$COMP) {
fn setup(comp: &mut $COMP) {
comp.csr().modify(|_, w| unsafe { w.inmsel().bits($bits) });
}
}
Expand Down Expand Up @@ -371,37 +388,48 @@ pub struct Comparator<C, ED> {

pub trait ComparatorExt<COMP> {
/// Initializes a comparator
fn comparator<P: PositiveInput<COMP>, N: NegativeInput<COMP>>(
fn comparator<P, N, PP, NP>(
self,
positive_input: P,
negative_input: N,
config: Config,
clocks: &Clocks,
) -> Comparator<COMP, Disabled>;
) -> Comparator<COMP, Disabled>
where
PP: PositiveInput<COMP>,
NP: NegativeInput<COMP>,
P: IntoObservationToken<Peripheral = PP>,
N: IntoObservationToken<Peripheral = NP>;
}

macro_rules! impl_comparator {
($COMP:ty, $comp:ident, $Event:expr) => {
impl ComparatorExt<$COMP> for $COMP {
fn comparator<P: PositiveInput<$COMP>, N: NegativeInput<$COMP>>(
self,
positive_input: P,
negative_input: N,
fn comparator<P, N, PP, NP>(
mut self,
_positive_input: P, // TODO: Store these
_negative_input: N, // TODO: Store these
config: Config,
clocks: &Clocks,
) -> Comparator<$COMP, Disabled> {
positive_input.setup(&self);
negative_input.setup(&self);
) -> Comparator<$COMP, Disabled>
where
PP: PositiveInput<$COMP>,
NP: NegativeInput<$COMP>,
P: IntoObservationToken<Peripheral = PP>,
N: IntoObservationToken<Peripheral = NP>,
{
PP::setup(&mut self);
PP::setup(&mut self);
// Delay for scaler voltage bridge initialization for certain negative inputs
let voltage_scaler_delay = clocks.sys_clk.raw() / (1_000_000 / 200); // 200us
cortex_m::asm::delay(voltage_scaler_delay);
self.csr().modify(|_, w| unsafe {
w.hyst()
.bits(config.hysteresis as u8)
.scalen()
.bit(N::USE_VREFINT)
.bit(NP::USE_VREFINT)
.brgen()
.bit(negative_input.use_resistor_divider())
.bit(NP::USE_RESISTOR_DIVIDER)
.pol()
.bit(config.inverted)
});
Expand All @@ -415,13 +443,19 @@ macro_rules! impl_comparator {

impl Comparator<$COMP, Disabled> {
/// Initializes a comparator
pub fn $comp<P: PositiveInput<$COMP>, N: NegativeInput<$COMP>>(
pub fn $comp<P, N, PP, NP>(
comp: $COMP,
positive_input: P,
negative_input: N,
config: Config,
clocks: &Clocks,
) -> Self {
) -> Self
where
PP: PositiveInput<$COMP>,
NP: NegativeInput<$COMP>,
P: IntoObservationToken<Peripheral = PP>,
N: IntoObservationToken<Peripheral = NP>,
{
comp.comparator(positive_input, negative_input, config, clocks)
}

Expand Down
3 changes: 3 additions & 0 deletions src/gpio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! General Purpose Input / Output
use core::marker::PhantomData;

use crate::observable::Observable;
use crate::rcc::Rcc;
use crate::stm32::EXTI;
use crate::syscfg::SysCfg;
Expand Down Expand Up @@ -379,6 +380,8 @@ macro_rules! gpio {
}
}

impl<MODE> Observable for $PXi<MODE> { }

impl<MODE> $PXi<MODE> {
/// Configures the pin to operate as a floating input pin
pub fn into_floating_input(self) -> $PXi<Input<Floating>> {
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ pub mod exti;
pub mod flash;
pub mod gpio;
pub mod i2c;
pub mod observable;
pub mod opamp;
pub mod prelude;
pub mod pwm;
Expand Down
75 changes: 75 additions & 0 deletions src/observable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
use crate::Sealed;
use core::marker::PhantomData;

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 •
edited
Loading

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!


/// A struct to hold peripherals which are to be observed.
///
/// This prevents the observed peripheral from being consumed. Thus
/// preventing things like a an observed gpio pin changing mode or an opamp from
/// being disabled. This makes sure the underlaying peripheral will not
/// change mode into something that is not compatible with what ever may be observing it.
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
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You should definitely constrain P to Observable!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for dumb question, but why? The only way to construct an Observed is with a P that is Observable, right? Just curious :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Makes sense


/// A struct to represent a registered observation of a peripheral of type `P`
///
/// The existence of this type guarantees that the observed peripheral will not
/// change mode into something that is not compatibe with what ever is observing it
pub struct ObservationToken<P> {
_p: PhantomData<P>,
}

/// A trait providing an interface to make peripherals observed
///
/// See [`Observed`] and [`ObservationToken`]
pub trait Observable: Sized {
fn observe<const N: usize>(self) -> (Observed<Self, N>, [ObservationToken<Self>; N]) {
(
Observed { peripheral: self },
core::array::from_fn(|_| ObservationToken { _p: PhantomData }),
Copy link
Contributor

Choose a reason for hiding this comment

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

I did some testing and determined this does not introduce any overhead. I recall that being a concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks!

)
}
}

impl<P: Observable + Sealed> IntoObservationToken for P {
type Peripheral = P;
fn into_ot(self) -> ObservationToken<Self::Peripheral> {
let (_peripheral, [ot]) = self.observe();
ot
}
}

impl<P: Observable + Sealed> Sealed for ObservationToken<P> {}
impl<P: Observable + Sealed> IntoObservationToken for ObservationToken<P> {
type Peripheral = P;
fn into_ot(self) -> Self {
self
}
}

impl<P, const N: usize> AsRef<P> for Observed<P, N> {
fn as_ref(&self) -> &P {
&self.peripheral
}
}

impl<P, const N: usize> AsMut<P> for Observed<P, N> {
fn as_mut(&mut self) -> &mut P {
&mut self.peripheral
}
}
Loading