Closed thejpster closed 8 months ago
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 :)
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.
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
.
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.
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)
I'd like to nominate #249 as a blocker for the 1.0 release.
@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
.
Thanks for the reminder, shipped in #255
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).
I believe an exact version should be needed for alphas. i.e. embedded-hal = "=1.0.0-alpha.2"
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.)
Yeah, plus if you use alpha versions for production code, you should be aware that things might break at any time :slightly_smiling_face:
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.
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.
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
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.
Thoughts on ditching try_
before this goes live? It clutters syntax, isn't consistent with other libs, and doesn't provide much value.
I think the try_
naming is a mistake.
std
is fallible, yet doesn't have try_.try_
is avoiding naming collisions if infallible traits are ever introduced, but:
Error = !
(or core::convert::Infallible
)embedded-hal
.I think the way to go is keep the decision of having only fallible traits, but name the methods foo
instead of try_foo
.
embedded-hal
crateI have to agree. Unnecessary as Rust won't let you mistake the return type.
@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.
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).
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.
This allows direct code to look clean; no cluttering by
try_
prefixes nor any handling of impossible errors (the never endinglet _ = pin.set_high();
,pin.set_high().void_unwrap();
, orpin.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.
@therealprof: However there is precedence for methods which are using
try_
to differentiate from the previous infallible methods liketry_fold
vsfold
.
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.
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
Can I nominate #201? E-H CountDown timers can't really be used generically right now, because the choice of Time type varies heavily:
Right now a generic driver that needs to use a timer in some way has to ask a user for both a timer and as many different time values as it may need. (Or if it only needs one value, maybe ask a user to preconfigure a Periodic timer)
@AlyoshaVasilieva #201 certainly highlights a problem but at the moment there are no concrete PR proposals to fix it. I think this will require more time to iron out and would not include it in 1.0. In any case, please do not feel discouraged. We would welcome proposals on how to fix it.
Perhaps CountDown should be removed before 1.0, and added later when fixed then? The current trait is useless in generic drivers, and fixing it is a breaking change.
There are other traits with completely unconstrained type Time
, which I think suffer from the same problem.
Are there any generic drivers using them?
We should not forget that embedded-hal
is useful not only for generic drivers but also as a common abstraction for MCU HALs. Thanks to embedded-hal
it should be easier to change between MCUs.
If you find no HALs using a particular trait, removing it initially and readding them later on would also be possible.
In that regard, it provides a standard interface.
I've chosen set_freq(freq: f32)
in my code bases, where freq
is in hertz. This is an arbitrary choice over period: f32
, where period
is in seconds. Note that this interface doesn't make sense for non-FPU MCUs, except perhaps in initial setup as a once-off.
I agree with Dirbao's proposal. Timers are too varied in features and use to map well to a most-common-features-only trait. I make liberal use of them, and have never found a use for a blocking, non-configurable API. I don't think the common abstraction use case is a justification on its own - the implied question is, as "Are there any generic drivers using them", "Are there any code bases that use these HAL-provided APIs?"
As an extension to Dirbao and eldruin's most recent posts: embedded-hal
I2C, SPI, UART etc APIs are general; they provide a good example for how to set up an API for those peripherals. Countdown doesn't.
Are there any generic drivers using them?
Stepper is using timer::CountDown
, but with a where
clause that requires conversion from embedded_time::duration::Nanoseconds
to CountDown::Time
to be possible. I've extended LPC8xx HAL to support this, but I'm not aware of any other HALs that do so.
By itself, timer::CountDown
is not very useful.
https://github.com/dbrgn/espresso is an incomplete driver for interfacing with ESP devices running the ESP-AT firmware. ESP-AT firmware is designed to let you add an ESP microcontroller to a project and give your already-in-use microcontroller ability to use WiFi (plus other things). espresso
expects timers to have Time: From<u32>
and interpret it as milliseconds, which will fail for most HALs, whether due to it not being supported or because it interpreted 1000u32 as 1000Hz (1ms) or 1000ns (0.001ms).
It's built on an old version of atat; the current version uses CountDown timers in the same way.
I also have a never-finished driver for interfacing with devices running ESP-AT firmware; the idea was to be able to add WiFi/maybe Bluetooth to any E-H UART-capable MCU by adding an ESP32. While working on it I found a bug in ESP-AT firmware where it would crash and stop responding forever (until RESET pin was pulled low). I added a 10s timeout using a Timer with core::time::Duration
and then found out that:
So I stopped working on it and moved on to other things. (I planned to come back and keep working once ESP-AT bug was fixed, but didn't; pretty sure it would still need a timer)
I didn't need any advanced features of a timer, just to be able to say 'start timing ten seconds' and 'has that time passed?'
u32 milliseconds isn't much better than STM32fxx's Hertz: In this case, it's limited to no higher than 1Khz, which is a non-starter for many uses. On FPU-MCUs, thoughts on f32 seconds or Hz? One caveat is some people prefer a struct or enum wrapper, ie to avoid errors such as timer.set_freq(temperature)
.
I think time / duration / frequency related interfaces should be postponed and not be included in the 1.0 release as it is very obvious from the discussion above that the current abstractions are not useful enough on its own and the community has to further explore what the best solution is.
There are some solutions, like using f32
/ f64
based timing values or generic approaches, like embedded-time
or other proposals I've missed.
But my feeling is, that as of now, we do not have enough experience with those approaches to consider including them into embedded-hal
.
I prefer no floats, because:
I prefer "time units" (seconds/milliseconds/microseconds/ticks/whatever) to "frequency units" (hertz), because;
Either way, I guess our options are the following:
Traits that use time that would ahve to be renamed
I think option 3 is best.
Seems like we could keep DelayMs
and DelayUs
given they specify precisely their integer units?
True, they don't suffer from the problem. If we leave them in then they'll probably end up being inconsistent with how time handling works in the rest of the traits, which would be a shame....
I agree that delay_ms(ms: u32)
and delay_us(us: u32)
are nice APIs for short blocking delays, eg cortex-m systick. delay_μs(μs: u32)
I'm less sure about.
I prefer "time units" (seconds/milliseconds/microseconds/ticks/whatever) to "frequency units" (hertz), because;
* With Hz you can't delay for 2s, or for 0.75s (unless you use floats or fractions) * (almost?) all MCUs timers specify the "time to delay for" in time units (number of ticks) instead of frequency units. Converting from frequency to time requires expensive reciprocals (1/x), vs converting between differnt time units just requires multiplying by constants.
I buy that, but still find it arbitrary if using floats. I've now added set_period(time: f32)
to my timer APIs, so you can do either. (time is in seconds.)
Should we rather have the units in the trait names everywhere?
DelayMs
, DelayUs
, DelayNs
CountDownMs
, CountDownUs
, CountDownNs
I buy that, but still find it arbitrary if using floats. I've now added
set_period(time: f32)
to my timer APIs, so you can do either. (time is in seconds.)
16777216.0 + 1.0
Yet one pre-release maybe? cc @eldruin
@burrbull Seems appropriate, yes. -> https://github.com/rust-embedded/embedded-hal/pull/327
I would like to nominate #264 as blocking for 1.0.
We had a brief chat about 1.0 in the meeting today and two interesting points were raised:
nb
traits entirely for 1.0? It's not clear that they're especially useful or widely used, many users report confusion with them, and ultimately the async traits should fully replace nb
. Plus, we could always add them back, but we can't ever delete them later if they're included in 1.0.::blocking
sub-modules which simplifies the crate, but we couldn't easily merge async traits back in the future.Both worth resolving before we release 1.0.
- we could always add them back
Could we? If we plan to replace them with asyncs.
Move nb
traits in other crate like async
traits now?
I guess there is no need to depend on nb
because of async
. For compatiblity nb
should be in separate trait that probably will be deprecated one day or even right after it will be released :)
How would you do non-blocking calls without the traits where the methods return nb? It's entirely valid to spin in a reactor loop without using async/await syntax, and without using Futures.
Hi! What is the missing work to drive this to completion? How can folks help?
@JanBerktold There is a list of open blocker issues/PRs in the top post. Once those are settled, we can release 1.0. Feel free to participate in any of them.
Part of https://github.com/rust-embedded/wg/issues/383
Settled blockers:
163
234
98 PR: #222
140
147 PR: #230
156
110
244 PR: #246
298
229 PR: #296
287
321
351
365
398
394
392
432
440
443
445
367
349
480
283 PR: #284
482
483
embedded-hal-bus
later.541
552
455
Open blockers:
547
553 ?
555
Feel free to nominate issues also by commenting below