Update Registers trait to match radio-sx1231

[henrikssn]

@ryankurte let me know what you think!

[ryankurte]

i added a commit to swap to words here and have implemented it in radio-at86rf23x, what do you think?

[henrikssn]

i added a commit to swap to words here and have implemented it in radio-at86rf23x, what do you think?

I have been thinking more about this, and I think I agree with you, except that I would use associated types instead. The motivation is that it allows us to implement TryFrom/TryInto for Specifier (behind a feature flag), which makes this work automagically for all modular bitfield types. Also it makes the Registers trait easier to implement, since you can assume that a given register has a unique size (with generics you can implement both Register<u8> and Register<u16> for the same type).

Associated consts is the "correct" way to implement this, but it currently sadly comes with too many limitations for it to be convenient. In particular, not supporting that type constraints can depend on associated consts is a deal breaker.

[ryankurte]

I have been thinking more about this, and I think I agree with you, except that I would use associated types instead.

oh yeah, limiting this to one impl makes sense to me. would this allow a driver to have read_register::<R: Register<u8>>() as well as read_register::<R: Register<u16>>() for your combined IRQ register case?

Also I intended TryFrom because register->value conversion can fail if returned data isn't as expected and Into because value->register is defined by the type system and cannot fail.

it allows us to implement TryFrom/TryInto for Specifier (behind a feature flag)

i'm not so convinced by this, given there's already a feature in modular_bitfield to automagically generate TryFrom and Into and baring in mind the orphan trait rules i don't think this can be made to work in a simple / unconflicting way.

if Specifier is part of modular_bitfield we would have to implement Register: Specifier in radio to avoid collisions, and if you guard things like this behind a feature then have two radios with different expectations it can cause problems. (imagine one radio has used the #[repr(u8)] feature and one has not, you'll get trait conflicts/errors if you enable/disable this feature)

based on that i'd prefer to TryFrom and Into as the requirements from the radio side so it's a simpler API / set of constraints (and we're not prescribing how people define registers).

[ryankurte]

ahh, there's a reason i didn't do this originally... Register needs to be generic to specify the TryFrom and Into bounds...

pub trait Register<W=u8>: Copy + TryFrom<W> + Into<W> {
    const ADDRESS: u8;
}

and there's no point in doing this to the Registers trait because, you might want to read/write both u8 and u16 registers.

you can hack around it with an intermediate trait:

// Register trait for radio implementations
pub trait Register: Copy {
    const ADDRESS: u8;
    type Word;
}

pub trait R2<W>: Register<Word=W> + TryFrom<W> + Into<W> {}

impl <W, T: Register<Word=W> + TryFrom<W> + Into<W>> R2<W> for T {}

pub trait Registers<W=u8> {
    type Error: Debug;

    /// Read a register value
    fn read_register<R: R2<W>>(&mut self) -> Result<R, Self::Error>;
    ...

which we could choose to seal

impl <W, T: Register<Word=W> + TryFrom<W> + Into<W>> private::Sealed for T {}

mod private {
    pub trait Sealed {}
}

i'm not sure if this has any other limitations, but it's implemented here and i've tried it out against radio-at86rf23x [1, 2]

[henrikssn]

You can actually get the best of both worlds by keeping the generic constraints while still using associated types:

pub trait Register: Copy + TryFrom<Self::Word, Error=<Self as Register>::Error> + Into<Self::Word> {
    type Word;
    type Error;
    const ADDRESS: u8;
}

pub trait Registers<Word>  {
    type Error: Debug;
    fn read_register<R: Register<Word=Word>>(&mut self) -> Result<R, Self::Error>;
    fn write_register<R: Register<Word=Word>>(&mut self, value: R) -> Result<(), Self::Error>;
    fn update_register<R: Register<Word=Word>, F: Fn(R) -> R>(&mut self, f: F) -> Result<R, Self::Error>;
}

I pushed a new commit which implements this.

[ryankurte]

You can actually get the best of both worlds by keeping the generic constraints while still using associated types:

oh wow, TIL! lgtm, let's do it

[ryankurte]

unless you have any reason not to i'll tag a minor release so we can publish the new drivers?

[henrikssn]

SG!

Ryan @.***> schrieb am Mi. 10. Nov. 2021 um 21:52:

unless you have any reason not to i'll tag a minor release so we can publish the new drivers?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rust-iot/radio-hal/pull/19#issuecomment-965736626, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGR5A7DQUSZRJLON6ZH5WTULLLRVANCNFSM5HOCQKLA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.