Embedded-hal: Ship a 1.0 release

[thejpster]

Part of https://github.com/rust-embedded/wg/issues/383

Blockers:

• [x] ~WIP: Re-organising repo~ (closed for later consideration)
• [x] [[RFC] Changing the way we handle addition/changes to the traits](https://github.com/rust-embedded/embedded-hal/issues/163)
• [ ] ~[RFC] Use core language async primitives instead external crates~
• [x] [[RFC] digital::v3 interface](https://github.com/rust-embedded/wg/pull/393) + moving to a single interface
• [x] Use nb 1.0.0
• [ ] [[RFC] Revamp Error types](https://github.com/rust-embedded/embedded-hal/issues/229)
• [ ] [Add methods to enable/disable entire PWM](https://github.com/rust-embedded/embedded-hal/pull/182) PR: #183
• [x] [Improve watchdog API through move semantics](https://github.com/rust-embedded/embedded-hal/issues/98) PR: #222
• [x] [Allow PwmPin to return old value during transition time](https://github.com/rust-embedded/embedded-hal/pull/140)
• [x] [Support multiple addressing modes for I2C](https://github.com/rust-embedded/embedded-hal/issues/147) PR: #230
• [x] [Change reference implementation to something more maintained](https://github.com/rust-embedded/embedded-hal/issues/156)
• [x] [ADC API does not allow non-unit channels](https://github.com/rust-embedded/embedded-hal/issues/110)
• [ ] [Abstracting over PWM traits not ideal](https://github.com/rust-embedded/embedded-hal/issues/244) PR: #246
• [ ] [Capture trait requires moving/copying channel argument](https://github.com/rust-embedded/embedded-hal/issues/249)
• [ ] Your issue here!

Feel free to nominate issues also by commenting below

[jamesmunns]

Here is a [link to the video call] discussion had by myself and most of the members of @rust-embedded/hal team. This includes initial decisions made, and justification/reasoning for each point. The next steps for this would be to codify this into an RFC, and get approval for it (though there is already general consensus within the embedded-hal team).

by the way, this was the "issue with the list of big topics" we were looking for during the meeting :)

[eldruin]

I believe the only unresolved issue as of the list above is https://github.com/rust-embedded/embedded-hal/issues/172
I would also add the dependency on https://github.com/japaric/nb/issues/20
Then we need to think about the dev-dependencies which are at the moment stm32f3 and futures. I guess specially these will hold us back for a while.

[Disasm]

This async story began just recently and I do not expect it to end in the coming year. Why should we block on it? We can always add this later. In the worst case, these async traits will be released in embedded-hal v2.0.0.

[therealprof]

I also think we should keep async separate because it's a different paradigm and might need require a different approach anyway. I can imagine taking an approach similar to async-std vs std, i.e. have the async traits in a separate crate async-embedded-hal which would allow crates to move indepedently and express more clearly what is supported where.

[ryankurte]

i have mixed feelings about async being separate in the long term, but imo it very much depends whether we can find an approach (as is the case for one of the demonstrated ones) that allows _everything_ to be a default impl on top of our base nb or poll traits as we have now with the blocking default methods, so we have one source of truth for integration, simplify the job of hal implementers, and you'll always have access to both. Or if these introduce fundamentally incompatible requirements and must be managed separately. this, however, is not something we can conclusively tell afaik.

i posed here (https://github.com/rust-embedded/embedded-hal/issues/172#issuecomment-641719714) that _if_ we have a path towards a decision on the nb/futures swap I would be okay to hold, but so long as it's an open / unknown problem I would also prefer to keep moving and accept integrating async _may_ be another breaking change down the line.

(i would also _very much_ like to hear the thoughts of @japaric if ya have a moment to look at all of this)

[hannobraun]

I'd like to nominate #249 as a blocker for the 1.0 release.

[hannobraun]

@rust-embedded/hal Would you consider releasing 1.0-alpha.2? I'm working on a driver that I'd like to release soon, and it would be great if I had access to PinState.

[ryankurte]

Thanks for the reminder, shipped in #255

[hannobraun]

I've recently learned something that I'd like to share, in case it isn't common knowledge yet: A cargo update will pick up alpha.2, if alpha.1 is specified in Cargo.toml. I think this is problematic, as new alpha releases might introduce breaking changes. HAL crates that would like to test-drive the alpha can't do so while guaranteeing semver stability.

Maybe it would be best to yank the existing alpha releases and test the new changes in a 0.3 series instead? As far as I can see, the alternative would be breakage, inhibited adoption, or additional hoops to jump through (like hiding all alpha implementations behind a feature flag).

[eldruin]

I believe an exact version should be needed for alphas. i.e. embedded-hal = "=1.0.0-alpha.2"

[hannobraun]

Oh yeah, for a moment I completely forgot that you could do that. Seems to be a good way to work around the issue! (Although in general, the situation is still not ideal, as not everyone will remember to do that. Still, probably not worth yanking and re-releasing.)

[dbrgn]

Yeah, plus if you use alpha versions for production code, you should be aware that things might break at any time :slightly_smiling_face:

[hannobraun]

Yeah, plus if you use alpha versions for production code, you should be aware that things might break at any time

But that issue affects not only those running the alpha, but everyone using a HAL that supports it in addition to the 0.2.x version. Of course, you might say that HALs intended for production use shouldn't support the alpha version, but then what is the point of an alpha, if it can't be used? :-)

In any case, @eldruin's workaround is good enough, so I'm fine.

[therealprof]

But that issue affects not only those running the alpha, but everyone using a HAL that supports it in addition to the 0.2.x version. Of course, you might say that HALs intended for production use shouldn't support the alpha version, but then what is the point of an alpha, if it can't be used? :-)

I don't think you can support multiple versions at once. We do have a few implementations with a non-released separate branch for the alpha versions but I'm not worried about breakage there.

[david-sawatzke]

I don't think you can support multiple versions at once.

No issue there, see (and various impls of both in master):

https://github.com/lpc-rs/lpc8xx-hal/blob/6e787f8e26d908b3cea13c3c47f04695f4112837/Cargo.toml#L40

[rumatoest]

Hello. I think that you must address such issue as https://github.com/rust-embedded/embedded-hal/issues/29
There are many MCUs that do not have open-drain like pins (like Arduinos), but it is still possible to "emulate" similar behavior to communicate with devices via only single data line.
Basically it is mostly about combining two traits together OutputPin + InputPin considering that each HAL implementation will perform under the hood magic while calling function from different traits.

[David-OConnor]

Thoughts on ditching try_ before this goes live? It clutters syntax, isn't consistent with other libs, and doesn't provide much value.

[Dirbaio]

I think the try_ naming is a mistake.

1. It is noisy
2. It is inconsistent with the rest of the Rust ecosystem, therefore unidiomatic. For example, all IO in std is fallible, yet doesn't have try_.
3. AFAICT, the only reason for try_ is avoiding naming collisions if infallible traits are ever introduced, but:
   
   
   
   • Infallible traits can be achieved by impls having Error = ! (or core::convert::Infallible)
   
   
   • Infallible traits are a bad idea anyway, IMO:
     
     
     
     
     
     • If drivers use infallible traits, it fragments the ecosystem.
     
     
     
     • If drivers stick to fallible traits, no usability improvement is achieved.
     
     
     
     • HAL crates will have to implement both the infallible and fallible traits, unless a blanket impl is done. Such blanket impl can be done in an external crate, there's no advantage in having these in embedded-hal.
     
     
     
     • It makes the API more complicated because now there's two ways to achieve the same thing with non-obvious (or no) tradeoffs. It makes the docs harder to read because everything is duplicated.
     
     
     
   
   
   • If infallible traits were still introduced, naming collisions wouldn't be that bad honestly. You either import the fallible or infallible trait, and go use the methods. No code will use both at the same time, realistically.
   
   
   • It is very unlikely that infallible traits will be introduced for most traits. The only ones I kindof see happening is GPIO.
   
   

I think the way to go is keep the decision of having only fallible traits, but name the methods foo instead of try_foo.

• Less visual noise
• Consistent within the embedded-hal crate
• ... but also consistent with the rest of Rust.

[reneherrero]

I have to agree. Unnecessary as Rust won't let you mistake the return type.

[therealprof]

@Dirbaio Thanks for your writeup.

I almost fully agree with your assessment. However there is precedence for methods which are using try_ to differentiate from the previous infallible methods like try_fold vs fold. Fallible methods in e-h were always a mistake IMHO, but some people have (maybe had?) strong opinions about keeping the door open to go infallible without cognitive and computational overhead and having the fallible methods to include try_ is the idiomatic way to achieve that.

The reasoning from the critical meeting where this decision was made can be read here: https://hackmd.io/ck-xRXtMTmKYXdK5bEh82A?view#Final-Summary

I think the best way to achieve that would be for someone who deeply cares to create an RFC so people can properly weigh in and we can have a vote in the end.

[Rahix]

I want to make a point for keeping the try_ prefix. I came to like it quite a bit while building a HAL against the alpha-version because it allows a HAL-API design that clearly discerns the generic interfaces from the hardware-specific ones.

Generally an inherent problem with an effort like e-h is that the provided API must be a one-fits-all design. This of course leads to decisions like having every method fallible. Such things are absolutely necessary to ensure drivers written against e-h will work in every situation, but it limits the ergonomics for code that is written against a specific hardware. For example you have fallible methods for setting pins which is super strange on a microcontroller where it obviously cannot fail. No matter whether the method is called try_set_high() -> Result<> or set_high() -> Result<>, the problem is the same.

I think the solution to this is to not view the e-h API as the general HAL API users are meant to use. Instead, a HAL crate should provide its own _hardware-specific_ API for direct users and implement the e-h traits _alongside_ for generic drivers to interface with.

I think this is a useful Rust pattern in general which especially applies here: A type should implement its full featureset as an API consisting of inherent methods (impl Type {}) and then implement appropriate traits (impl Trait for Type {}) ontop of this. Using a trait for the primary API has its uses (e.g. io::Read) but comes with the huge drawback of locking everyone into a set API that might not fit them perfectly (see fallible vs. infallible). I think especially in the heterogeneous world of MCU peripherals, it is important that every HAL can make its own API decisions here.

As such I think it is wrong to use the e-h traits for anything else than enabling generic drivers that work across HALs. In particular using them as the main way to interact with the hardware from application code is not a design which scales in my opinion.

To put it into more concrete terms, here is a GPIO pin implementation for a HAL which leverages such a design. I have a HAL-crate locally where I explored this in more detail, if there is interest, I can publish it.

pub struct Pin<...> {}

impl<...> Pin<Output> {
    pub fn set_high(&mut self) {
        // non-fallible method for direct users
    }

    pub fn set_low(&mut self) {
        // non-fallible method for direct users
    }
}

impl<...> embedded_hal::OutputPin for Pin<Output> {
    pub fn try_set_high(&mut self) {
        // generic method for generic drivers
        self.set_high();
        Ok(())
    }

    pub fn try_set_low(&mut self) {
        // generic method for generic drivers
        self.set_low();
        Ok(())
    }
}

This allows direct code to look clean; no cluttering by try_ prefixes nor any handling of impossible errors (the never ending let _ = pin.set_high();, pin.set_high().void_unwrap();, or pin.set_high().ok();). Additionally, no magic traits need to be imported into scope first, because the API is made from inherent methods.

#[entry]
fn main() -> ! {
    // ...

    loop {
        pin.set_high();
        d.delay_ms(100);
        pin.set_low();
        d.delay_ms(100);
    }
}

But it also allows writing generic drivers which can work with any HAL, fallible or not, and such driver code will clearly show that it is able to handle errors by having the try_ prefix visible:

impl<PIN: OutputPin, DELAY: DelayMs> BlinkyDriver<PIN, DELAY> {
    pub fn blinky(&mut self) -> Result<!, SomeError> {
        loop {
            self.pin.try_set_high()?;
            self.d.try_delay_ms(100)?;
            self.pin.try_set_low()?;
            self.d.try_delay_ms(100)?;
        }
    }
}

In more complex situations, the benefit of the HAL being able to define its own API is even greater. Suppose peripherals like a SPI controller which often have custom features the HAL might want to expose. A design like outlined above places no limitations on doing this while still keeping the generic layer supported alongside (with reduced featureset) so generic drivers can be "connected" with zero effort (one of the big benefits of embedded rust in my eyes).

[David-OConnor]

I agree with all Rahix said re how to use EH, and how that mitigates try_ by reducing the number of places it occurs, and why fallibility doesn't seem like a good fit for hardware-infallible pins but is fine etc. I don't see how that justifies the try_ syntax over the alternative of its omission.

[therealprof]

This allows direct code to look clean; no cluttering by try_ prefixes nor any handling of impossible errors (the never ending let _ = pin.set_high();, pin.set_high().void_unwrap();, or pin.set_high().ok();). Additionally, no magic traits need to be imported into scope first, because the API is made from inherent methods.

BTW: If everything is fallible you can also chain calls rather than handling (or ignoring) the possibility of failure on each of them individually...

self.pin.try_set_high()
    .and_then (|_| self.d.try_delay_ms(100))
    .and_then (|_| self.pin.try_set_low())
    .and_then (|_| self.d.try_delay_ms(100))

I do think the ability to combine peripheral operations with the incredible Result API is a big feature, not a bug. And compared to the times of thetry! macro of the early days this is constantly becoming more and more convenient to use every other Rust release.

[Dirbaio]

@therealprof: However there is precedence for methods which are using try_ to differentiate from the previous infallible methods like try_fold vs fold.

As outlined in my previous post, try_ would be idiomatic if we had fallible and infallible traits. However, we don't have them right now, it's unlikely that we'll have them in the future, and IMO it's a bad idea to have them.

---

@Rahix: I came to like it quite a bit while building a HAL against the alpha-version because it allows a HAL-API design that clearly discerns the generic interfaces from the hardware-specific ones.

You're saying the try_ prefix is good because it allows distinguishing embedded-hal methods from HAL-speficic methods. I disagree this is a good thing, in fact I think it's yet another symptom of the prefix being the "wrong" choice:

In idiomatic Rust APIs, try_ is supposed to distinguish fallible from infallible. As a user, if I see a struct has try_foo and foo methods, I expect try_foo to return Result<X, Error> and foo to return X (and panic/block/etc on failure).

However, HAL methods are likely to be fallible too! For example, it's likely that a HAL Uart will end up with a HAL-specific read() method returning Result and the e-h try_read() method returning Result too. This is inconsistent and unidiomatic. As a user, if I see read and try_read I'd expect read to be infallible.

Maybe the HAL-specific method should be named try_read() too for consistency, but then the "advantage" of being able to distinguish HAL vs e-h methods by the try_ prefix is gone.

---

As an aside, if we were to keep the try_ prefix, the traits themselves should be named TryFoo too, right? See From + from vs TryFrom + try_from. This is yet another reason the current naming is unidiomatic.

[Dirbaio]

This has been discussed in today's Rust Embedded meeting.

           Dirbaio:  maybe the real question here is "do we want infallible versions of the traits or not"
           Dirbaio:  if yes, then try_ makes sense (but traits should be renamed to TryFoo too)
           Dirbaio:  if not, then try_ should be removed
             rahix:  I didn't get around to reply yet, but I do see the point Dirbaio made. I would be okay with removing the prefix as well, my point was related but not at its core a reason to keep try_.
           Dirbaio:  so maybe to unblock the issue, the discussion of "do we want infallible versions of the traits or not" should be had now, not posponed? 
            Lumpio:  IMHO the traits are most useful for building generic drivers, and I don't see infallible traits being a thing there if we want fallibleness ever. Which I think is a good thing. HALs can add infallible methods in their concrete implementations if they want, which can even have the same names because of how traits work.
         adamgreig:  having the HAL provide infallible methods for end-users and implement e-h to provide infallible seems like a really nice way to keep the HALs nice for direct end users to use while keeping e-h suitably generic for drivers
           Dirbaio:  IMO postponing the discussion of infallible traits, but adding the try_ naming "just in case" is not the way to go
           Dirbaio:  if the answer is "we don't want infallible traits" then we're stuck with bad naming forever for no reason
         adamgreig:  what's the argument for infallible types in e-h?
            Lumpio:  And hopefully in the Future Rust will be expanded so that you can just ignore Result<T, !>, and use the fallible methods in app code without any extra boilerplate or warnings.
             rahix:  From a technical standpoint I think infallible traits don't get us much at this point, the performance will be the same.
       therealprof:  re adamgreig "having the HAL provide infallible...": Not sure how you would do that. That'd bring you back right to the "no panic" issue.
            Lumpio:  I think adamgreig might've means "implement e-h to provide fallible"
            Lumpio:  meant*
         adamgreig:  sorry yes, second one should be fallible
         adamgreig:  same point Lumpio is making
           Dirbaio:  I haven't seen any convincing argument for having infallible traits in e-h to be honest
           Dirbaio:  usually it's "ease of use"
         adamgreig:  ease of use" seemed appealing until rahix pointed out we don't need it in e-h
             rahix:  yeah, that was exactly my point
           Dirbaio:  but it can be achieved by having HALs add infallible / friendlier methods, it can be achieved by wrapper traits in external crates: impl InfallibleOutputPin for OutputPin<Error=!>
           Dirbaio:  and the disadvantages are many (see my comment)
       therealprof:  I think the main proponent of the infallibe traits should be lurking here on the channel. \U0001f609 
           Dirbaio:  who is it?
       therealprof:  Not going to out anybody here if they don't want to speak up.
           Dirbaio:  great! no infallible traits then \U0001f61c
       therealprof:  Yeah, I guess that's what it's going to be.
             rahix:  would it make sense to have a vote for this or is it uncritical enough to just make a decision right now and here?
            Lumpio:  (IRC seems to have died to switching to Matrix for a while)
       therealprof:  Well, one of the team members is not present due to timezone issues.
 firefrommoonlight:  I took rahix's advice and use infallible gpio methods in HAL. I bring this up, since that's an option when using HALs directly, ie you aren't dependent on the EH trait. For generic drivers, you are, and I don't have an opinion.
         adamgreig:  perhaps the best option is PR making the name change and the hal team can vote on it? it's their call in the end, and at least the pr is hopefully fairly quick to put together
         adamgreig:  sed s/try_//
 firefrommoonlight:  I'm treating EH traits as a bolt-on to HAL - not the HAL's primary API
       therealprof:  Let's do that.
         adamgreig:  re firefrommoonlight "I'm treating EH traits as a bolt-on to HAL - not the HAL's primary API": yea, this shift in perspective has definitely brought me around to not needing infallible traits in e-h, personally