Embedded-hal: New version?

[little-arhat]

Hi,

Number of features have been added since 0.2.1 -- should there be another release? I can't use git dependency if I want to publish hal-impl to crates.io.

Are there any guildelines regarding versioning? Seems like any new feature merged to master should bump patch level, otherwise hal-impl can't really use them.

[therealprof]

I agree both with the idea of bumping the version with each change and to have a new release soon.

[little-arhat]

What about releasing new version with revised list of unproven features?

[ryankurte]

I'd like to push a new version once we've landed #101, #102 and #105 (hopefully shortly), which will be an 0.X release (specifically 0.3.0) due to breaking changes, any comments @rust-embedded/hal ?

[therealprof]

@ryankurte Which change is do you see as breaking?

[ryankurte]

The falible digital traits PR (#105) is breaking

[therealprof]

Right, but I was hoping we would release a non-breaking 0.2.2 before pulling that in.

[ryankurte]

Fine by me, we need more reviews on all the linked PRs so once 101 and 102 are reviewed and merged we can release an 0.2.3 then land 105 and 0.3.0.

[hannobraun]

• #101 is almost ready to go. Waiting for the author to squash commits.
• I still question the wisdom of removing the unproven flag on a trait right before making breaking changes to it, but if you want to merge #102, I won't stand in the way.
• #105 is blocked by this issue, but otherwise ready to go

[therealprof]

I can see people wanting to stay on 0.2.x until all crates are ready for 0.3 which will take a while and the request to prove traits has been an elephant in the room for quite a while even longer than this ticket which is also is a few months old now.

[hannobraun]

I understand that people want proven traits, but I question the point of declaring traits with known issues proven, then breaking them due to those known issues.

Yes, it sucks that those traits have known issues, and that we have to make breaking changes to fix those known issues. I don't see how declaring those traits proven right before making those breaking changes benefits anyone.

I understand that those users who want the traits proven might stay on 0.2.x for a while. I don't see how not having to enable the unproven feature during this time benefits them. They'll have to fix their implementations when upgrading to 0.3 in any case. Declaring those traits proven will remove a minimal obstacle to using them (enabling a feature) at the cost at confusing people about the future stability of those traits.

But as I said previously: I'm not making a stand here. It's not what I would do, but if you want to merge #102, go ahead.

[therealprof]

I understand that people want proven traits, but I question the point of declaring traits with known issues proven, then breaking them due to those known issues.

I'd like to see that we're sticking to the rules which were once decided (or change them).

Regarding the "issues" and breaking the traits in the next version: When the "digital" traits were conceived they were meant to cover regular GPIOs and operations on those usually can not fail so it didn't make sense to have them fallible. In hindsight it should have been clear that IO expanders and multiplexers exist (especially since people including me have used them in the past) and that someone will eventually come up with a driver.

I am no and (most likely never will be) a fan of replacing the previous digital traits with fallible versions of it under the same name because the old traits are widely used and the fallible ones are really only required for a few new and special use cases. This change will split the whole Rust "full-stack" embedded ecosystem in two and I have serious doubts that it'll quickly recover. This is a huge risk and could potentially cause a lot of harm. Hence I really prefer a separate set of traits which would be non-breaking and allow fallible and non-fallible versions to co-exist (and even better to be used at the same time) and phase out the old traits much later.

[hannobraun]

I understand that people want proven traits, but I question the point of declaring traits with known issues proven, then breaking them due to those known issues.

I'd like to see that we're sticking to the rules which were once decided (or change them).

Let me take a step back here, because I think it's important we're all on the same page:

1. #102 proposes to mark InputPin proven, with 2 participating team members being in favor, 1 (me) arguing against.
2. In #100, I proposed making the digital trait changes a clean break, which has been met with +1's and no opposition.

The combination of those two things is what I'm arguing against, because I think it makes no sense. But I don't think the combination of these two things is what you're arguing for. You want to go forward with #102, but you don't want to break the digital traits.

Let's revisit my proposal to make the digital changes a clean break. I proposed this because I started feeling bad about that pull request, and didn't want it turning into one of these months-long affairs, where the contributor is changing stuff again and again, due to changing reviews with ever new insights. Due to the response there (+1's, no opposition), I assumed that we'd be doing this.

I think I was wrong in at least one, possibly two ways. First, your comment has almost got me convinced that my proposal was wrong, and that we shouldn't do a clean break here. Second, even if I was right, it was wrong of me to assume we were going with this proposal after just a few days without opposition. I should have given you and others more time to respond.

So, if we won't break the digital traits, #102 makes a lot more sense. The digital traits can become proven (and possibly, at the same time, deprecated), while we go forward with a second set of traits. That's something I can get behind.

The question remains is how exactly we're going to transition (or whether we're going to transition at all). I think you're saying you might want to have two sets of traits in perpetuity, but I think that is not a workable solution, as I've previously explained.

I am no and (most likely never will be) a fan of replacing the previous digital traits with fallible versions of it under the same name because the old traits are widely used and the fallible ones are really only required for a few new and special use cases. This change will split the whole Rust "full-stack" embedded ecosystem in two and I have serious doubts that it'll quickly recover. This is a huge risk and could potentially cause a lot of harm. Hence I really prefer a separate set of traits which would be non-breaking and allow fallible and non-fallible versions to co-exist (and even better to be used at the same time) and phase out the old traits much later.

As I said, I don't think keeping two sets of traits in perpetuity is a workable solution (again, my explanation), but a slow transition phase is perfectly fine. I still think my proposal ([1], [2]), which is possibly the same as your proposal, is the best way to go here. @ryankurte is not convinced, though :)

[therealprof]

I'm definitely less thinking of "in perpetuity" than a couple of months or a year. Having both traits side by side would allow for a smooth transition (and as mentioned previously it's even possible to bridge between the fallible and non-fallible interfaces even without losing anything in the common case of MCU GPIOs) and then we can easily phase out the old traits. New drivers for multiplexers etc. could easily offer just the new traits thus softly nudging users to change the HAL impls and their software...

In cases like this (where I have two dozens of affected published crates and many more unpublished) I'm always considering how much pain it would cause me to have such breakage (and it would be immense!) and then I also have to consider when to make the change and how much pain it would cause to all users (or how many would then rather not take the new version and stay with the old one...). I'm not even sure how many GPIO examples we have in official documentation which would all require changing and big fat disclaimers and explanation of the two alternatives and how to choose...

[hannobraun]

I'm definitely less thinking of "in perpetuity" than a couple of months or a year.

Great, that's totally in line with what I'm thinking. I think our next step should be to reach consensus on a transition strategy in #100. I've gone ahead and blocked #102 and #105 until then.

[ryankurte]

In that case, what would y'all think of a 0.2.2 release from the current master?

[ryankurte]

(sorry, after landing 101)

[therealprof]

@ryankurte +1

[hannobraun]

@ryankurte Fine by me.

[thenewwazoo]

Now that 101 has landed, I'd love to get a new version published. :)

[ryankurte]

Looks to me like we need a PR with an updated changelog including all the changes introduced since v0.2.1 (looks like we need to be better at merging those as part of PRs) and a version bump in Cargo.toml. If you'd like to do that, it might speed up the process, otherwise one of us will hopefully have some time soon ^_^

@therealprof this action is exactly equivalent to my proposal in #100, see how it goes I guess?

[japaric]

Operational comment: dont forget to do the semver trick to allow interoperation between 0.2.2 hal-impls / drivers and 0.3.0 drivers / hal-impls, at least for the traits that didn't change between the two versions.

[hannobraun]

Operational comment: dont forget to do the semver trick to allow interoperation between 0.2.2 hal-impls / drivers and 0.3.0 drivers / hal-impls, at least for the traits that didn't change between the two versions.

Thank you, @japaric. The release we currently agreed on will be a non-breaking patch release. We're aware of the semver trick and have been writing whole books worth of discussion about how specifically to employ it over in #100 :)

[ryankurte]

hi folks,

we've landed all the changes for fallible pins, which as far as i can see is currently non-breaking and only adds to the API (v1 is re-exported by default but marked depricated, v2 must be explicitly selected), which i think means we can consider this a minor release?

as an aside, running cargo semver over master highlights a set of errors on the serial module that i'm not sure about / probably need to be resolved prior to shipping anything, any ideas what this means?

error: breaking changes in `read`
  --> /home/ryan/projects/embedded-hal/src/serial.rs:14:5
   |
14 |     fn read(&mut self) -> nb::Result<Word, Self::Error>;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = warning: type error: expected enum `nb::Error`, found a different enum `nb::Error` (breaking)

[therealprof]

@ryankurte Definitely a minor release since we're not at 1.0 yet. However I have a feeling you have a slightly different understanding of _minor_. This is going to be 0.3.0 and we should revisit anything that should be added for that.

as an aside, running cargo semver over master highlights a set of errors on the serial module that i'm not sure about / probably need to be resolved prior to shipping anything, any ideas what this means?

Who knows? 😅

[ryankurte]

@therealprof ahh good catch, s/minor/patch/g in my previous post, are there outstanding changes that require the planned minor bump over a patch release given the digital::* stuff is no longer _breaking_ afaik?

Seems to me we only have additions so far, though we could switch the default pin implementation to v2.

also are there any other things we particularly want to land before a release?

[therealprof]

Good question. But due to the intrusive nature of this chance I'd probably rather make it a minor release instead, even if not technically necessary.

[hannobraun]

@therealprof

Good question. But due to the intrusive nature of this chance I'd probably rather make it a minor release instead, even if not technically necessary.

That doesn't seem like a good reason enough to me. Incrementing the minor version will split the ecosystem, unless we do the manual work required to do the semver trick.

I mean, if you volunteer to do that work, then I don't mind, I guess. Still, there'd be a chance of you missing something and causing breakage. Seems like an unnecessary risk to me.

[therealprof]

@hannobraun If you say so. My worry is more on the side of accidentally breaking existing applications than requiring an explicit opt-in to a new version.

[hannobraun]

Well, I think both ways have a risk of accidentally breaking applications. My way could introduce an unintended breaking change, your way could accidentally split the ecosystem. In both cases, the solution would be to release a fix and yank the offending crate. I prefer my way because it's simpler and less work.

But if we're going to release a new minor version anyway, I think we should switch the default gpio implementation to v2, as @ryankurte suggested.

In any case, I don't feel very strongly about this right now, so feel free to go ahead in whichever manner you think is best :-)

[ryankurte]

-wonders whether we could convince crater to regression test hal changes-

seems like it should be possible / could be valuable for us longer term, but, documentation seems lacking / i can't see how

[edit] opened an issue: https://github.com/rust-lang-nursery/crater/issues/407

[ryankurte]

ooh, what about a patch pre-release as a reasonably low effort middle ground? no changes by us (and thus a decreased chance of Maintenance Induced Failures), opt-in testing, and if we haven't had any reports of failures after a while we could then push a normal release.

[therealprof]

@ryankurte Is there a mechanism for that?

[ryankurte]

i've definitely seen prereleases in use and on further research it's supported by semver but i can't parse from this issue or seem to find any docs about how cargo handles it ¯\_(ツ)_/¯

[therealprof]

I'm fine with playing guinea pig here to unblock this.

[ryankurte]

a quick test with this suggests that cargo won't auto-update from 0.1.0 to 0.1.1-alpha.1, which seems okay.

just to be clear, is that an ack on publishing a pre-release? (also any thoughts about calling it an -rc release so it's easy with cargo-release?)

[therealprof]

@rust-embedded/hal

Do we want to test the waters of the new version with a pre-release of the crate?

Ticky-boxes for the win:

• [x] @ryankurte
• [x] @hannobraun
• [ ] @thejpster
• [ ] @ithinuel
• [x] @therealprof

[hannobraun]

Please do whatever you think is best. As I already stated, I'd just do a patch release, but I didn't mean to block any decision here, and I apologize if I gave that impression.

[therealprof]

@ryankurte Are we happy with this now?

[ryankurte]

i've been using 2.3-rc.1 for a while, but, turns out the -rc.X releases are not considered compatible by cargo so i haven't managed to test interop in a meaningful way :-(

that said, i think i'd be happy to release it and yank/fix if anything unforeseen crops up (partly because i'm not really seeing another option).

[therealprof]

@ryankurte Let's release it.

[ryankurte]

aand 0.2.3 is published and released. thanks for all your efforts!