embedded-hal: [RFC] Managing breaking changes to existing traits

Misc notes as an idle watcher of this thread:

• Although cargo will update items massage versions to their newest semver compatible version, if you still had a stale Cargo.lock around, the versions will be pinned. This could cause a dependency to keep the old version, while the main crate takes the new version. This may explain why “broken” stuff worked with either a clean checkout (no lockfile), or with an upgraded/changed version of Rust (which I think at least partially invalidates cached items, but I am not 100% on this).
  • The fix for this is either cargo update, or deleting the target dir and lockfile.

Also, again as an outsider here (and as someone who has admittedly not read all of the context), this discussion seems to be getting a little heated. We’re all working (hard, and in free time), to get this right. Perhaps it might be better to take this to a lower latency format to discuss iteratively? e.g. IRC, Hangouts, call, etc?

edit: clarified cargo’s version resolution behavior.

I definitely agree that we need to be more cautious for 1.0, and as mentioned above I think we should try to have whatever approach we decide on in place and tested before we get there.

+1 for a clean break for #97 so we can land it and move forward ^_^

@ryankurte

It seems to me that if there’s a way we can do the work on our side it’d be better than expecting it of the whole ecosystem?

If that’s feasible, then we should do it. As an author/co-maintainer of several HAL implementations, I’m glad to have that in my toolbox though.

simply replace it and release a new breaking version

Pretty onboard with this, we have a common understanding with semver of what a version number change means, it’s simple for us and easy to understand, and it only breaks once.

So we’re back to the simple solution? I’m not opposed to this, but I think for a foundational crate like embedded-hal, it’s worth exploring alternatives.

I see your points regarding version numbers in traits and the additional breakage, but I think both problems are not that important:

• The version numbers in the traits (e.g. InputPin2) are only a temporary measure. Temporary ugliness is okay in my book, if it serves a purpose (the purpose being a smooth transition period).
• The additional breakage causes minimal effort (e.g. change InputPin2 to InputPin), and the breakage as a whole is spaced out over multiple releases. If we follow this transition strategy, every single one of those breaking releases will be such a non-issue, that the additional breakage is of no consequence, in my view.

Another point to consider: Right now we can release breaking changes whenever it suits us. Once 1.0 is out, we should be much more conservative about breaking changes. That means, we’ll either have a transition strategy in place by then, or we’ll potentially be shipping broken traits with no alternative for what could be years. I think even the ugliest transition strategy (InputPin2, InputPin3, InputPin4, … 😃 ) is preferable to that scenario.

Still, I’d be fine with clean, breaking releases, if that’s the majority consensus. But I do think it’s not the best solution, and not even a good solution in a post-1.0 world.

Suggestion: Regardless of what we decide here for the general case, let’s go with a clean break for #97. That way, we can take the time to come to a good decision without blocking that pull request.

Hey, sorry for the delay. I’m away from the office right now, and this got buried under loads of unhandled notifications.

@ryankurte

Assuming for now we’re in agreement, @hannobraun you raised concerns about impl hal_v03::digital::OutputPin for OutputPin vs. impl<T> hal_v03::digital::OutputPin for T where T: OutputPin (#100 (comment)), as well as with limiting the ability to implement both traits (#100 (comment)). As far as I can tell (given we provide adaptors) we shouldn’t need anyone to implement both trait versions?

Assuming the adapters work, I can’t think of a reason why anyone would need to implement both trait versions. I just wanted to bring it up, in case someone else can think of a reason why this would be a problem.

Do you have an alternative approach we could explore, and what testing would you like to see before we go ahead with one?

I don’t have other ideas. If I were to go ahead with this personally (which I don’t have time for right now), I’d want to experiment with impl ... for OutputPin vs impl<T> ... for T ..., to fully understand what the difference is, and why for OutputPin works (which surprises me). I don’t have any specific test cases in mind.

Since I’m not going to do the work, and don’t have concrete concerns, I won’t object to going ahead with this.

Almost the same as above, a new v0.x.0 minor release (v0.3.0) would include the breaking IO trait changes, and a new v0.2.x patch release (v0.2.2) would introduce the compatibility backport. This leaves us with a new release with fallible traits and backwards compatibility with the ^0.2.2 line, letting people choose to either bump minor versions with no change or major versions and update their use of the traits, and either option interoperate.

This approach probably won’t be applicable to all breaking changes, but it seems like a pretty reasonable mitigation for this one imo. It’ll also be interesting to track what people do, given the options.

I’m still of the opinion that unnecessary breakage should be avoided at any cost and the mentioned changes are not significant enough to warrant such a sledge hammer.

Specifically what breakage are you talking about here? There are a bunch of sub-optimal outcomes from making and not making changes, and afaik any action we take causes breakage of some kind. No change leaves us with flawed traits, releasing any new package version causes breakage so long as packages refer to different crate versions, having both trait types creates breakage with incompatibility between drivers and hals using one or the other, changing the api creates breakage because people have to update their code to match.

As far as I can see, this approach mitigates about as much of this as we can, while still letting us move forward. All this is built on the premise that we should have fallible pins, and that the should replace the infallible ones rather than maintaining two alternatives, which I thought was addressed in #97, if that is not the case perhaps we need a wider wg vote?

Before we can agree on an approach, we need to agree on a basic value framework first

I agree, I wonder whether it’d be useful to talk about goals/objectives/expectations for the embedded-hal somewhere else, though that discussion should hopefully not block us making changes, maybe a big issue that people can drop their perspectives into could be a way to start?

I agree that a such an issue sounds like a good way to start.

I think it would also be a good idea if someone wrote up the consensus that seems to have developed here (release breaking changes under new names, research other approaches, revisit later) and opened a pull request that adds this as guidelines to this repository. Then we can have a proper team vote on it.

I gave v0.2.3 a quick spin and for some reason it breaks I2C for me:

cargo build --release --examples
    Updating git repository `https://github.com/ryankurte/embedded-hal`
    Updating git repository `https://github.com/ryankurte/embedded-hal`
   Compiling embedded-hal v0.3.0 (https://github.com/ryankurte/embedded-hal?tag=v0.3.0#f22caf24)
   Compiling embedded-hal v0.2.2 (https://github.com/ryankurte/embedded-hal?tag=v0.2.3#2479c003)
   Compiling stm32f042-hal v0.6.0 (/Users/egger/OSS/stm32f042-hal)
error[E0277]: the trait bound `hal::i2c::I2c<hal::stm32f0x2::I2C1, (hal::gpio::gpiof::PF1<hal::gpio::Alternate<hal::gpio::AF1>>, hal::gpio::gpiof::PF0<hal::gpio::Alternate<hal::gpio::AF1>>)>: embedded_hal::blocking::i2c::WriteRead` is not satisfied
  --> examples/i2c_hal_ina260serial.rs:55:26
   |
55 |         let mut ina260 = INA260::new(i2c, 0x40).unwrap();
   |                          ^^^^^^^^^^^ the trait `embedded_hal::blocking::i2c::WriteRead` is not implemented for `hal::i2c::I2c<hal::stm32f0x2::I2C1, (hal::gpio::gpiof::PF1<hal::gpio::Alternate<hal::gpio::AF1>>, hal::gpio::gpiof::PF0<hal::gpio::Alternate<hal::gpio::AF1>>)>`
   |
   = note: required by `<ina260::INA260<I2C>>::new`

So I wrote a long reply without a good conclusion and decided to try implementing the semver trick inspired no-breakage update I mentioned earlier, I ended up applying the breaking change and backporting an implementation of the new traits into an 0.2.x branch to enable compatibility between them, rather than using Into as I had expected, and have uploaded it to my fork to demonstrate:

• ryankurte/embedded-hal@v0.3.0 is a clean break 0.3 release
• ryankurte/embedded-hal@v0.2.4 backports the new traits to the v0.2.1 hal (see digital.rs and cargo.toml)
• ryankurte/hal-interop-example demonstrates no-overhead interop between hal traits (see lib.rs).

[updated with new links]

Which I think gives us:

• Compatibility between version
• No duplicated traits / forever numbering / multiple fixes required
• No changes required to resolve breakage (0.2.x minor update adds 0.3 compatibility)
• Nothing particular for us to maintain going forwards (all compatibility is in 0.2.x branch)
• We can still mark the 0.2 traits deprecated to move people forwards

I couldn’t work out how to adapt the StatefulOutputPin trait in one direction, I think it should be possible but am not enough of a wizard with rust syntax to work out how. Apart from that, maybe I’m missing something, but it seems pretty good?

This sounds very good @ryankurte.

1. It seems to me like there will be an increase of feature flags and it can become difficult to track the flags across releases. In this regard, I would add a list somewhere where the flags are registered, including in which release they will appear/disappear. A more condensed form of the Changelog, if you like, to ease the release process. Checking on this should be added to the release checklist.

2. We can have a first try at testing combinations of flags in the CI build in #97. I only did this locally.

Fair enough, do you have a preferred arrangement, or would you be happy with 0.x: patch, minor, minor, and >=1.x: minor, major, major? While it deviates from strict semver for 0.x, it benefits us if the first deprecation marking is an automatic upgrade.

I’d be very happy with that. I don’t see how it deviates from semver. Adding a deprecation won’t break the build, unless the crate denies warnings, which is effectively opting out of semver stability.

I like the safety and caution of this approach, but it feel like it’s more suited to post-1.0 quality software. What would the version numbers look like in the above process, semver-minor or semver-patch? I noticed you didn’t define any timeframes for these releases. To keep things flowing, I think the deprecation process should be pretty quick at the stage the ecosystem is at now.

Personally, I’d prefer to have this stuff break catastrphically (as per the Alternative section) and fix it once, but I can see that causing a lot of pain for people with more embedded Rust written than myself.