ADC API does not allow non-unit channels

[chrysn]

I'm just implementing the unproven ADC traits for RIOT and found an issue with Channel::channel:

As the channel() function does not even take a &self reference, there can't be implementations of it that are not fully contained in the type (ie. only really works for zero-sized types). Now the zero-sized types are a major plus of embedded Rust, but this signature precludes the use of eg. numeric channels as would be convenient when wrapping an operating system's ADCs.

I suggest adding a &self or &mut self argument to the .channel() function to make it usable as a method.

I don't understand the rationale behind there not being a self argument in or the existence of the channel method at all there in the first place, so I can't tell what it breaks. The example in the OneShot trait would need to say _pin.channel() instead of PIN::channel() -- but also there I don't see the rationale for not doing that in the first place.

(For the RIOT wrappers, until I can publish the code I'd like to point to, things look like this:

pub struct ADCLine(i32); // You'd get that from the OS on the long run; right now the int is pub
impl Channel<ADC> for ADCLine {
    type ID = i32;
    fn channel() -> i32 { unimplemented!() }
}

impl OneShot<ADC, i32, ADCLine> for ADC {
    fn read(&mut self, pin: &mut ADCLine) {
        unsafe { riot_sys::adc_init(pin.0) };
        ...
    }
}

)

[thenewwazoo]

I made the choice but it wasn't based on any kind of a deep analysis, it was just how it shook out (and, for me, it worked). I'm 100% in favor of this change, and afaik nobody else has impl'd these traits so imo the sooner the better to minimize breakage. Perhaps this change could get wrapped into #100?

[eldruin]

Hi, I am also trying out these traits in a driver I started for ADS1x1x ADCs.
I found the channels API also unexpected but what I did was to add marker types and "translate" them into an internal enum. See here.

It is a bit cumbersome but the benefit for this driver is that the read() for OneShot is not implemented for channel selections that are not supported. e.g. ADS1x13 and ADS1x14 only support DifferentialA0/A1 measurements.
This allows avoiding a "UnsupportedChannelSelection" runtime error in case the user made a mistake.

What is ugly is that the internal enum needs to be public (although I hid it in the docs and the user could not do much with it) because it is part of the public interface through the Self::ID type.

EDIT: The channel reference being mutable is also something that looks weird in my use case:

let measurement = block!(adc.read(&mut channel::DifferentialA0A1)).unwrap();

[thenewwazoo]

For a bit of context about why the channel method even exists, it’s due to this bug

[ryankurte]

Adding a self parameter sounds good to me, I'm not sure whether it should be mutable or not?

It is both v new and unproven (#10, #101 ), I think this is exactly the situation unproven exists for / imo it'd be reasonable to just replace it and release without expending undue effort on breakage mitigation.

@rust-embedded/hal maybe a quick vote before going down either path?

• 👍 for PR fix and release
• 👎 for stop and work out an approach to mitigate
• Comment for anything else (maybe on [RFC] Managing breaking changes to existing traits #100 with a reference to here?)

[couchand]

It looks like #192 made this situation less likely to be addressed, since it removed the associated function entirely in favor of an associated constant. @eldruin or @ryankurte, is it fair to assume that this issue will not be resolved any time soon?

[eldruin]

Uh, thank you for bringing this up.
I am afraid this issue was not followed up and was then just forgotten. I for one did forget about it and just replaced the method with a constant because the underlying issue was fixed in Rust 1.35.0.
So far only the 1.0.0-alpha.1 version contains this change and we have further breaking changes in the pipeline so we could still go back to the method.

Would it be necessary for the reference to be mutable?

[couchand]

@eldruin Oh good to hear.

For my part I don't think the reference needs to be mutable, it's simply to support a generic pin enum that would dispatch to the appropriate concrete pin type. The assumption that a concrete pin has a static channel is still valid. My reading of the OP's report is that a shared reference would be sufficient there, too, but @chrysn perhaps if you're still following this issue you'd like to weigh in?

One concern I have is that by forcing all usage through a dynamic code path, there would be a runtime cost even in the common, static case. There are a few too many moving pieces for me here for me to convince myself one way or the other.

[Rahix]

Would it be necessary for the reference to be mutable?

If the returned value is just meant to be an ID I don't see why it would need mutability. In fact I'd say mutability would give the implementation power it should not have. The ID should be an inert value and the channel() method should return the same ID if called multiple times without doing any internal magic or potentially changing the state of the pin ...

One concern I have is that by forcing all usage through a dynamic code path, there would be a runtime cost even in the common, static case.

I'm pretty sure the static case will always get optimized out.

[eldruin]

I made a PR doing this at #242.

[thenewwazoo]

I got fired from my last job in the embedded space and have been taking a hiatus from embedded Rust, but this PR has gotten me to pick it back up. I'm glad to see it getting some love. :)