[RFC] Managing breaking changes to existing traits

[ryankurte]

As highlighted in #95, #97 and #99, there are likely to be situations in which we need to fix or upgrade traits in breaking ways, and these are likely to have significant ramifications on the embedded ecosystem. This RFC proposes an approach to manage these breaking changes.

Goals:

• Mitigate the downstream breakage caused by embedded-hal breaking changes
• Communicate breaking changes to give library uses an opportunity to update

Approach:

We propose a staged approach for managing breaking changes, with some time between each stage to allow API consumers to update.

In the first release, replacement traits are introduced and feature-gated allowing users to opt in to their use. Feature names start with with use- and a description of the change, for example use-fallible-digital-traits for the new fallible digital trait update in #97. Existing traits are marked #[deprecated] to alert users to the need to update, and this opt-in flag should be added to the CI build matrix. Documentation should also be updated to recommend the use of the opt-in traits and the related github issue for the change. This stage alerts HAL users of deprecation and allows immediate migration should they desire, without requiring any explicit action.

In the second release, the opt-in feature flag is replaced with an opt-out flag to allow regression to the previous trait if required, this should start with regress-. Continuing the fallible digital trait example this would be regress-infallible-digital-traits. This opt-out flag should be added to the build matrix, and the previous flag removed. Documentation should be updated to mention opt-out trait availability if required. This stage will break HAL consumers using the previous traits, requiring explicit action to update or defer the change.

In the third release, the old traits are removed, feature flags are removed from the package and the CI build matrix, and the documentation should be updated to remove mention of the opt-out trait. This stage will break any hal consumers that have not updated to the new traits.

In each release, these changes will be documented in the changelog (as is standard) and should be published through other useful means. The linking of the related issue in the documentation should allow for feedback from HAL consumers at each stage.
Each stage will correspond to a semver version increase (though it is worth noting that more than one breaking change may be included in a single release). At 0.x this will be patch, minor, minor, and >=1.x: will be minor, major, major.

I also wrote a small tool to analyse the dependency requirements, resolutions, and features of published crates (https://github.com/ryankurte/rust-dep-check), which would let us track migration through versions and feature flags. An example output showing the current use of embedded-hal:

Found 100 crates using 'embedded-hal'
Version requirements:
	^0.1	(0.1.3):	   1 / 100 (1.00 %)
	^0.1.0	(0.1.3):	   6 / 100 (6.00 %)
	^0.1.1	(0.1.3):	   1 / 100 (1.00 %)
	^0.1.2	(0.1.3):	   7 / 100 (7.00 %)
	^0.2	(0.2.1):	  19 / 100 (19.00 %)
	^0.2.0	(0.2.1):	  12 / 100 (12.00 %)
	^0.2.1	(0.2.1):	  45 / 100 (45.00 %)
	~0.2	(0.2.1):	   9 / 100 (9.00 %)
Features:
	unproven:	  35 / 100 (35.00 %)
Resolved versions and features:
	0.1.3:	  15 / 100 (15.00 %)
		unproven:	   3 / 15 (20.00 %)
	0.2.1:	  85 / 100 (85.00 %)
		unproven:	  32 / 85 (37.65 %)

Questions

• Should this apply to traits marked unproven? My view is that given the propensity of unproven traits we may as well try to mitigate all breakages or no worry about it at all (see the alternative).

Alternative

Do nothing, semver already mitigates breaking changes and we’re in the 0.x series with no API stability guarantees. This is much simpler, though not particularly supportive of our API consumers or the possible impact of breaking changes on the ecosystem, and it doesn't give our consumers any notice of the change outside of the github discussions.

cc. @hannobraun @therealprof

[jamwaffles]

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.

[ryankurte]

[I] feel like it's more suited to post-1.0 quality software

I agree in principle, but I think if we want to get it right by 1.x we should start practicing it now and work out what works and doesn't. In addition, I suspect embedded-hal will not reach 1.0 for some time yet, and as the ecosystem grows this is going to become ever more important.

What would the version numbers look like in the above process

Roughly following the semver spec for breaking changes, so for 0.x: patch, patch, minor, and for >1.x: minor, minor, major. The first stage is not breaking and should be updated automatically to notify consumers, the second only requires a feature flag change to opt out, the third explicitly breaks anything not updated.

I can see an argument that the second two stages should both be considered breaking, the flags will have to be updated to work, and I'm not completely convinced either way. It's also important to note that there is likely to be than one change applied per release.

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.

Agreed that it should be quick. I'd like to see us iterating and releasing more regularly, and I think having some process like this would decrease the risk / scariness of releasing so we can do it faster. I think if we had a minimum limit of a couple of weeks between each stage we could have a similar release cadence.

This approach also lets us collect feedback on and learn from our releases / changes and the impact they have on our consumers, and to stop part way if there are issues, where breaking changes are otherwise not communicated except for the breakage.

[hannobraun]

I'm generally in favor. The one concern I have is what consequences this has on CI. If I understand correctly, all combinations of features will have to be tested. Could it cause problems to have that many CI builds (i.e. running up against usage limits)?

Roughly following the semver spec for breaking changes, so for 0.x: patch, patch, minor, and for >1.x: minor, minor, major.

I disagree with this part. Upgrading to a new patch (0.x) or minor version (>= 1.x) should never break the build (except to fix security issues). Even if the fix to the breaking change is as simple as adding a feature.

[therealprof]

@ryankurte Your primary plan sounds good to me.

[ryankurte]

Could it cause problems to have that many CI builds (i.e. running up against usage limits)?

I hope breaking migrations are not so frequent as to require many more builds.

Upgrading to a new patch (0.x) or minor version (>= 1.x) should never break the build (except to fix security issues)

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.

[hannobraun]

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.

[eldruin]

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 Make digital pin traits fallible #97. I only did this locally.

[hannobraun]

I just noticed @therealprof and @ryankurte having a discussion on IRC that they really should be having here, in my opinion :)

@therealprof said this:

That still doesn't allow mixing and matching which is a huge problem IMHO.
It requires the HAL implementation and all drivers to use the same version of the trait.

This brings up an issue that is hugely important in my opinion: You can't have multiple drivers among your dependencies that depend on different versions of the trait. If one driver enables the features, than all drivers that don't enable the feature are suddenly broken and won't build. Unless I miss something, there's no other way to fix the broken build than fork one group of drivers and change them to do what the other group of drivers is doing.

I think this makes all other advantages that this proposal has moot. What's the point of having a smooth transition, if everything breaks down the moment I use two drivers that are at different stages of the smooth transition?

In light of this, I now prefer the following approach:

• Add new trait variations under a new name (e.g. MyTrait2) deprecate the old trait (e.g. MyTrait) and publish a non-breaking release.
• On the next breaking release, replace MyTrait with MyTrait2, deprecate MyTrait2.
• On the next breaking release after that, remove MyTrait2.

Yes, that's one additional breakage, but at least it won't break my build if I mix two drivers.

That whole thought about mixing and matching drivers, has led me to another insight: It's possible to import different versions of the same package under different names: rust-lang/cargo#4953

That means, a HAL implementation can release a transition version that supports both and old and a new version, allowing application authors to use any driver during the transition period.

[therealprof]

After thinking about it some more and some discussion on IRC I have to revise my previous statement.

I see two fundamental problems with a part of this approach (the replacement traits and the feature gating of them):

1. In one application you can only have exactly one version of a trait and all components must adhere to that, i.e. the application, the HAL impl and all used drivers need to implement the exact same embedded-hal version with the same "version" of the trait. This can be annoying already when only the application and the HAL impl are involved but add a few drivers to the mix and this can easily turn into a blocker
2. Feature gating itself can be very annoying and should be used sparsely rather than generously. This begins with the question where to define the features (because removing a feature is a breaking change) and how to select them (if you have a hierarchy of crates there're multiple places where one could do this and this can easily lead to conflicting ideas of which features to use) leading up to the infamous global feature problem where all crates used in an application have to use the exact same features (cf. Disable default-features on rand dependency to avoid std version cortex-m-rt#107) even in different versions so each addition of new a new feature is potentially also a breaking change.

I'd really want to avoid a cfg gate hell of our own. The only alternatives I see here are:

• naming the new trait differently and slowly phasing out the old one
• simply replace it and release a new breaking version

[ryankurte]

Good points ^_^ I kindof figured breaking changes should be few and far between, and our goal would be to keep most of our environment in sync with the hal, but the rand crate experience seems not great.

In light of this, I now prefer the following approach:

• Add new trait variations under a new name (e.g. MyTrait2) deprecate the old trait (e.g. MyTrait) and publish a non-breaking release.
• On the next breaking release, replace MyTrait with MyTrait2, deprecate MyTrait2.
• On the next breaking release after that, remove MyTrait2.

I like that we'd be giving notice to consumers of depreciation, I don't like that we'd effectively be causing more breakage and thus fragmentation of dependencies.

That means, a HAL implementation can release a transition version that supports both and old and a new version, allowing application authors to use any driver during the transition period.

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?

I just had a go at supporting both by including a previous hal version and writing adaptors (as suggested by @therealprof I think, but I may have misunderstood the suggestion), but, it seems like it would become a bit of a nightmare to manage if we ever had overlapping breakages / dependency requirements.

naming the new trait differently and slowly phasing out the old one

How would you approach this? I don't love the idea of version numbers in the api.

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.

[hannobraun]

@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.

[ryankurte]

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 ^_^

[hannobraun]

Relevant discussion in #92:

• New version? #92 (comment)
• New version? #92 (comment)

[ryankurte]

Hey, thanks for linking that back here (I'll try do that for any future discussions too), and I appreciate everyone's efforts in working to get this right ^_^ All my responses here are in the context of the alternative proposal here: #100 (comment)

@therealprof from #92 (comment)

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.

I appreciate the sentiment, I'm worried about fragmenting the ecosystem too, though I agree with @hannobraun here that having separate traits for fallible and infallible pins and leaving users to handle the interop issues long term would be more of a problem.

We're also at 0.x, I don't think the possibility of breakage is outside of anyones expectations, I think the impact of adding an error type is pretty minimal, and I'd like to hope that this is a very rare event. Whatever approach we take, we can monitor and collect feedback to improve for (the hopefully not required) next time.

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

Afaik the same trait in different crate version is incompatible, so it seems to me that having a larger set of versions and traits over which we apply the changes is going to cause the same headache as my proposal with combinations of versions across dependencies instead of features.

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 [...]

It sucks to have that impact, though should we go with the non-breaking merge and rename approach it would then require you to fix everything twice, and break all your downstreams twice? It seems to me that is also likely to leave people behind. Unfortunately I can't see the unpublished crates to collect data on them, but I know I'll have a bunch to fix too, and maybe there's an opportunity for us to rally the community around updating the published crates (while it's still october anyway).

@hannobraun your post about multiple crate versions got me thinking, I think (in this instance at least) it should be possible to backport adaptors for breaking traits to the previous series? If we were to release 0.3 as a breaking change, then 0.2.x could include 0.3 with From and Into implementations for the altered traits and I think you should just be able to .into() across hal versions? I mocked part of it out, but got a bit stuck due to the changes in groupings of traits.

I like the idea of this in that it doesn't block us from moving forward, shouldn't put any future requirements on new releases or require continued support in master, and is hopefully reasonably easy to explain and use. I dislike the precedent it sets for maintaining historic branches and the resulting opportunity for 0.2.x to hang around (which is part of the reason for my openness to a hard breaking change).

[therealprof]

We're also at 0.x, I don't think the possibility of breakage is outside of anyones expectations, I think the impact of adding an error type is pretty minimal, and I'd like to hope that this is a very rare event.

I strongly disagree. Not only should we avoid unnecessary breakage at any cost (we had far too much necessary breaking changes in the last month and there's only so much breakage users will tolerate without moving on), also the severity of the previously planned breakage (replacing the infallible traits with fallible ones under the same name, that is) is far from minimal, rather the opposite: It will instantly break all existing applications and drivers as soon as one component pulls in the new version.

Afaik the same trait in different crate version is incompatible

Yes, that's exactly the reason why we should not reuse the same traits.

It sucks to have that impact, though should we go with the non-breaking merge and rename approach it would then require you to fix everything twice, and break all your downstreams twice?

Yeah, not to happy about that either. I'd prefer to simply give the new traits fresh names and let the old names fade away into irrelevance and then remove them. We have a nice #[deprecated] flag to tell the users how to use the new traits and it's also rather simple for existing HAL implementations to provide both old and new trait impls.

I dislike the precedent it sets for maintaining historic branches and the resulting opportunity for 0.2.x to hang around (which is part of the reason for my openness to a hard breaking change).

Unless we decide to yank the old version (and instabreak all current uses of it immediately; probably the easiest way to create a giant shitstorm in the Rust universe!) it is going to hang around anyway. Before moving to a new minor (or major!) version reasonable folks will always consider:

• How much pain is it going to be for me?
• How much pain is a new release of my crate going to be for my users?

Using new traits under the same name would make the dependent crate immediately incompatible with all other crates using the old version so crate authors are likely to hold off with the changes until a considerable part of the whole ecosystem decided to make a move, which could potentially take a long time or even not happen at all. That's a huge risk and quite frankly -- given the rare cases of beneficial use of the fallible trait version -- not one I'd want to take lightly.

[hannobraun]

@ryankurte

Afaik the same trait in different crate version is incompatible, so it seems to me that having a larger set of versions and traits over which we apply the changes is going to cause the same headache as my proposal with combinations of versions across dependencies instead of features.

Wouldn't this problem be solved by employing the semver trick?

It sucks to have that impact, though should we go with the non-breaking merge and rename approach it would then require you to fix everything twice, and break all your downstreams twice? It seems to me that is also likely to leave people behind.

I really don't see this danger of leaving people behind.

• We presumably wouldn't release a breaking version with that one change, and instead wait until we have enough breaking changes to justify a breaking release.
• The changes will be minimal: Changing InputPin2 to InputPin is no big deal, especially if you're already upgrading to a new version anyway.
• No single release will ever break anything. InputPin2 will still exist, it will just be deprecated.
• We're not in a hurry. We can keep deprecated stuff around for as long as we want, until we're absolutely convinced the ecosystem won't be harmed by removing them. There's barely any harm in keeping InputPin2 around forever, if that's what we decide is best.

Any crate that's "left behind" under these circumstances is essentially abandoned, and unless you're going to maintain all of those crates, that's nothing we can prevent :)

@therealprof

Yeah, not to happy about that either. I'd prefer to simply give the new traits fresh names and let the old names fade away into irrelevance and then remove them. We have a nice #[deprecated] flag to tell the users how to use the new traits and it's also rather simple for existing HAL implementations to provide both old and new trait impls.

I understand where you're coming from but I'd really hate if in 5 years, I have to type FallibleInputPin instead of InputPin, due to some old version of the trait that no one even remembers any more.

Yes, let's be super careful and super conservative about making these changes. Yes, let's keep those deprecated traits around until they have faded into irrelevance. But please, let's not lock us into cumbersome trait names forever, due to some stupid mistakes we've made in what's basically the stone age of the embedded Rust ecosystem.

Whether we call it InputPin2 or FallibleInputPin during the transition phase is not my primary concern[1]. But let's at least intend to design a system that ends up at a near-optimal state.

[1] Although I prefer InputPin2, despite it's ugliness, because it communicates better that it's a new and improved version of InputPin, while FallibleInputPin doesn't, and is thus more likely to confuse casual observers.

[therealprof]

I understand where you're coming from but I'd really hate if in 5 years, I have to type FallibleInputPin instead of InputPin, due to some old version of the trait that no one even remembers any more.

Well, my 2 cents here:

• It's up to us to come with a more clever naming
• How often have you actually typed any of the trait names? You'll need that maybe a few times in an HAL impl, twice per driver impl using it and that's pretty much it...

I for one couldn't any less whether we call it SuperDuperInputPin, InputPin2 or put put them into a new module with the same name or create a sub-module under digital with the same name or any combination of those.

[ryankurte]

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/[email protected] is a clean break 0.3 release
• ryankurte/[email protected] 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?

[therealprof]

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`

[therealprof]

I also breaks I2C in other crates with other drivers, e.g. SSD1306.

[ryankurte]

Huh, how strange!

I based my 0.2.x branch on master, which does have plenty of changes, but I can't see any that should break I2C. I just released v0.2.4 with only my changes applied over v0.2.1, does that still break I2C? (And is there a project I can use to repro?)

[therealprof]

Yes, same problem. I'm using my stm32f042-hal for this:

# git status
On branch master
Your branch is up to date with 'origin/master'.
# cargo build --release --examples
    Finished release [optimized + debuginfo] target(s) in 0.10s
# git stash pop
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   Cargo.toml
# git diff
diff --git a/Cargo.toml b/Cargo.toml
index c12c7f5..0a6ce86 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -30,7 +30,9 @@ version = "0.2.2"

 [dependencies.embedded-hal]
 features = ["unproven"]
-version = "0.2.1"
+#version = "0.2.1"
+git = "https://github.com/ryankurte/embedded-hal"
+tag = "v0.2.4"

 [dependencies.stm32f0]
 features = [
# cargo build --release --examples
   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`
...

[ryankurte]

Right, it seems the fault occurs as soon as you add embedded-hal v0.3.0 as a dependency to v0.2.1, so it isn't related to the traits, and renaming the dependency also doesn't help. I have no idea why this might happen :-/

[ryankurte]

This is not a strict definition. version = "0.2.1" is equivalent to version = "^0.2.1". A strict definition would be version = "=0.2.1".

Oh gosh, you are totally right, my bad. I am not used to caret being inferred / not explicit.

but, well, that's my working theory right now 😁

Seems like a good working theory tbh.

it makes me think you didn't actually test the code that's on GitHub right now (not saying you willfully didn't test it, I'm saying you must have been accidentally testing something else)

Fair concern, seems fine from my side though and a fresh clone works for me (on cargo 1.31.0-nightly (5dbac9888 2018-10-08)) :-/

➜  example git:(master) git status                                               [1:24]
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
➜  example git:(master) git remote -v                                          [1:24]
origin	[email protected]:ryankurte/hal-interop-example.git (fetch)
origin	[email protected]:ryankurte/hal-interop-example.git (push)
➜  example git:(master) ✗ git  push                                             [1:26]
Everything up-to-date
➜  example git:(master) ✗ cargo +nightly test                            [1:26]
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
     Running target/debug/deps/hal_backport_example-0168dd1c99b77478

running 4 tests
test tests::new_new ... ok
test tests::old_new ... ok
test tests::new_old ... ok
test tests::old_old ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests hal-backport-example

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

[therealprof]

So far I've only assessed this based on the code on GitHub and what the both of you said here. Until I've had a chance to test it out myself I won't outright say that I'm right and the both of you are wrong but, well, that's my working theory right now 😁

As I've said before: If one is using semantic versioning correctly then just incrementing the patch version shouldn't have any impact at all; those are compatible versions and will be treated as such by cargo. Fiddling with the traits makes the version incompatible, as can be observed by running cargo semver on the v0.2.4 tag in @ryankurte version:

error: breaking changes in `OutputPin`
  --> /Users/egger/.cargo/git/checkouts/embedded-hal-68ea237a42de07e4/f22caf2/src/digital.rs:4:1
   |
4  | / pub trait OutputPin {
5  | |     /// Error type
6  | |     type Error;
7  | |
...  |
18 | |     fn set_high(&mut self) -> Result<(), Self::Error>;
19 | | }
   | |_^
   |
warning: added item to trait (breaking)
  --> /Users/egger/.cargo/git/checkouts/embedded-hal-68ea237a42de07e4/f22caf2/src/digital.rs:6:5
   |
6  |     type Error;
   |     ^^^^^^^^^^^

error: breaking changes in `set_low`
  --> /Users/egger/.cargo/git/checkouts/embedded-hal-68ea237a42de07e4/f22caf2/src/digital.rs:12:5
   |
12 |     fn set_low(&mut self) -> Result<(), Self::Error>;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = warning: type error: expected (), found enum `std::result::Result` (breaking)

error: breaking changes in `set_high`
  --> /Users/egger/.cargo/git/checkouts/embedded-hal-68ea237a42de07e4/f22caf2/src/digital.rs:18:5
   |
18 |     fn set_high(&mut self) -> Result<(), Self::Error>;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = warning: type error: expected (), found enum `std::result::Result` (breaking)

You might be totally right about the I2C problem vanishing into thin air by using a released crate instead of pointing to a git (or in my case local) repository, so I'm not worried about that at all. If it still breaks after a release we'll have to address this properly...

I am still wary and worried about the trait reuse for GPIO. The 0.2.0 breakage was a somewhat different case and did not have such a big impact as this will have.

[hannobraun]

@ryankurte

it makes me think you didn't actually test the code that's on GitHub right now (not saying you willfully didn't test it, I'm saying you must have been accidentally testing something else)

Fair concern, seems fine from my side though and a fresh clone works for me (on cargo 1.31.0-nightly (5dbac9888 2018-10-08)) :-/

Sorry, that was my bad. Whatever the problem was, it's gone after rustup update. Now I'm confused why it works at all, but that's my problem, not yours ;)

@therealprof

As I've said before: If one is using semantic versioning correctly then just incrementing the patch version shouldn't have any impact at all; those are compatible versions and will be treated as such by cargo. Fiddling with the traits makes the version incompatible, as can be observed by running cargo semver on the v0.2.4 tag in @ryankurte version:

But that's just it, what @ryankurte did really shouldn't have any impact! I can't see any reason why it wouldn't be a correct use of semver.

The cargo semver output you posted doesn't make sense to me. @ryankurte v0.2.4 tag doesn't include any changes to those traits (diff).

It looks rather like cargo semver was run against v0.3.0 (diff).

I am still wary and worried about the trait reuse for GPIO. The 0.2.0 breakage was a somewhat different case and did not have such a big impact as this will have.

I'm not ruling out the possibility that I'm still missing something big, but at this point, it seems pretty clear to me that your objections are based on misunderstandings.

It seems to me that:

• Ryan's approach follows semver strictly.
• Ryan's approach allows for a smooth transition without forced breakage at any point.
• Ryan's approach seems to allow mixing and matching between old and new implementations, old and new drivers.
• Ryan's approach might even allow implementations to stay on old versions of embedded-hal indefinitely, and still implement traits from new versions, if we add a pub before `extern crate embedded_hal as hal_v03.

I still have some reservations about the nature of the blanket impl's, and would like to see more extensive testing. But I don't see any big conceptual problems with the approach at this point.

[ryankurte]

@therealprof

As I've said before: If one is using semantic versioning correctly then just incrementing the patch version shouldn't have any impact at all

Yeah this was totally a misunderstanding of cargo semver on my part sorry, there have definitely been some on this side too 😅

@hannobraun

Ryan's approach might even allow implementations to stay on old versions of embedded-hal indefinitely, and still implement traits from new versions, if we add a pub before `extern crate embedded_hal as hal_v03.

If we base things on the previous breaking change we should replace any identical traits with the v03 ones, then maybe we don't need to re-export the whole v03 hal, just the new bits? idk. Also thanks for clearing up all my misunderstandings here ^_^

[therealprof]

@hannobraun

The cargo semver output you posted doesn't make sense to me. @ryankurte v0.2.4 tag doesn't include any changes to those traits (diff).

No it wasn't. Maybe cargo semver is easily confused by the cross impls.

it seems pretty clear to me that your objections are based on misunderstandings.

I would boldly claim that my picture is very clear. This proposal (reusing the trait names) is in the black magic realm and that kind of magic tends to fail in mysterious and sometimes spectacular ways which is exactly why I don't like it.

[hannobraun]

@ryankurte

If we base things on the previous breaking change we should replace any identical traits with the v03 ones, then maybe we don't need to re-export the whole v03 hal, just the new bits? idk.

Yes, we absolutely need to replace identical traits with re-exports, otherwise the whole semver trick won't work. How we re-export additions to the API is an implementation detail, I think, and not really important in the grand scheme of things. I don't have a preference right now (although a single pub use would be less work).

Also thanks for clearing up all my misunderstandings here ^_^

No problem, always glad if I can help :)

@therealprof

I would boldly claim that my picture is very clear. This proposal (reusing the trait names) is in the black magic realm and that kind of magic tends to fail in mysterious and sometimes spectacular ways which is exactly why I don't like it.

I am baffled by this comment.

First, making breaking changes to APIs without changing the names of those APIs is a practice that is absolutely pervasive and likely as old as the concept of APIs. The only novel thing here is the proposed approach to alleviating the breakage (which I wouldn't call reusing the name; bridging between the new and old versions seems more precise).

Second, you saying your picture is clear, then calling the proposal "black magic" seems to be a contradiction. Isn't "black magic" an expression used for things that are not understood or understandable? The only coherent interpretation of this comment that I can come up with is that you don't fully understand the proposal, don't think it can be fully understood, and therefore it's very clear to you we shouldn't use it.

I don't think that this is a very useful stance and I don't think this proposal is black magic. I think it is an interesting approach that can and will be understood. That understanding might unearth reasons why it can't work in practice, therefore my call for further testing.

[therealprof]

@hannobraun

First, making breaking changes to APIs without changing the names of those APIs is a practice that is absolutely pervasive and likely as old as the concept of APIs.

No, reasonable projects don't do that unless there's an important reason to do it (which there isn't IMHO). The way this is typically done is to introduce a new API and deprecate the old API, then phase it out.

Second, you saying your picture is clear, then calling the proposal "black magic" seems to be a contradiction. Isn't "black magic" an expression used for things that are not understood or understandable?

It is clear to me, that doesn't mean it is clear to everyone. That's exactly the problem with black magic, the wizards-in-charge think it's a great idea at the time until someone or something changes their mind... If you don't like the term black magic, we can also call it a footgun. Also (as expressed several times) I'm concerned about the users, not the few participants of this discussion.

I think it is an interesting approach that can and will be understood.

And I disagree and don't appreciate your misguided interpretations of my comments.

[jamesmunns]

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.

[hannobraun]

@therealprof

@hannobraun

First, making breaking changes to APIs without changing the names of those APIs is a practice that is absolutely pervasive and likely as old as the concept of APIs.

No, reasonable projects don't do that unless there's an important reason to do it (which there isn't IMHO). The way this is typically done is to introduce a new API and deprecate the old API, then phase it out.

I'm not disagreeing with that. I was observing that the practice is pervasive, not that it is good or should be emulated. This was brought on by my misunderstanding of your comment.

Second, you saying your picture is clear, then calling the proposal "black magic" seems to be a contradiction. Isn't "black magic" an expression used for things that are not understood or understandable?

It is clear to me, that doesn't mean it is clear to everyone. That's exactly the problem with black magic, the wizards-in-charge think it's a great idea at the time until someone or something changes their mind... If you don't like the term black magic, we can also call it a footgun. Also (as expressed several times) I'm concerned about the users, not the few participants of this discussion.

I think it is an interesting approach that can and will be understood.

And I disagree and don't appreciate your misguided interpretations of my comments.

I'm very sorry for misunderstanding your comment. I didn't mean to offend you.

That still leaves me very confused about what your misgivings are specifically.

In any case, I won't attempt to continue this part of the discussion for now. I think it's obvious that one of us is operating based on misunderstandings, and I'd like do my homework to make sure that it isn't me. (Please, don't take this previous sentence as any kind of attack. I'm very aware of the fact this it could be me who's wrong, and all I'm saying is I want to experiment more before I'm ready to continue here).

@jamesmunns

• Although cargo will update items to their newest 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.

Thank you, I'll keep that in mind.

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?

I agree that we should try our best to not let this discussion get (more) heated, and I'd be happy to have a call with anyone who's willing to have one. I think that real-time communication is very valuable for the social function it has (in so far as it can remind us that we're all in the same boat here). But I doubt that any form of low-latency communication will be useful for resolving the technical argument.

[therealprof]

@hannobraun

I'm very sorry for misunderstanding your comment. I didn't mean to offend you.

No offence taken.

That still leaves me very confused about what your misgivings are specifically.

I'm opposing the reuse of the same trait names for the fallible versions because I think there's nothing inherently wrong with the infallible versions that would warrant such a radical API change because there's a good chance that this will cause confusion and frustration down the road.

My preferred approach is to name the fallible traits differently, mark the old ones as deprecated, keep the fallible->infallible impl from @ryankurte and release the whole thing as quickly as possible a new patch or minor version (to signify the change even without breakage as recommended for >1.0.0 versions here: https://doc.rust-lang.org/cargo/reference/manifest.html#the-version-field).

[hannobraun]

@therealprof Thank you for that summary. It's actually very helpful in understanding where you're coming from, as I got lost in the details of the discussion, and this brings us back to the big picture. Let me try to summarize my own big-picture position in a similar way.

While I agree that it's important to minimize breakage, I'm concerned that in doing so, we end up with baggage that we'll be saddled with forever (for example in the form of names that are less understandable and straight-forward than they could be). I believe that it's likely that the embedded Rust ecosystem will grow significantly in the future. However minor the effect of these less-than-optimal names may be in isolation, they will add up over a large number of users over many years, leading to a significant amount of confusion.

Therefore I think it's worth putting effort into finding an approach that can potentially allow us to end up with a simpler end result, without causing pain right now. I'm not certain what my preferred approach is, but I think @ryankurte's proposal looks promising.

I think our disagreement comes mainly from the fact that we're weighing things differently. I put a lot of importance on the simplicity of the end result, and think some complexity during the transition is warranted to achieve that. I think you put more importance on keeping the process simple, and avoiding approaches that could potentially backfire. Do you think this is a fair assessment?

---

I've realized two things that I think should inform the discussion from here on:

1. Before we can agree on an approach, we need to agree on a basic value framework first. For example, is it a priority for us that our latest version presents a simple, clear, and well though out picture to new users (i.e. there's only InputPin and nothing else), or is it more important to make any single update for existing users as simple and risk-free as we can, at the cost of ending up with FallibleInputPin or InputPin2?
2. We can just start with your approach, and decide at any later point whether we want to clean up those InputPin2 names, or not.

Given that, I'd like to propose the following. Short-term (i.e. right now), we do this:

1. We add the fallible versions as new traits, to the module digital::v2 or similar (i.e. digital::v2::InputPin). That may not be pretty, but at least it's clear what it is, while a name like FallibleInputPin will forever pose the question to new users, why the pin is fallible and where the normal pin is.
2. We deprecate the current versions.
3. We look into providing blanket impls to convert between the two. I recommend doing more testing before committing to this, as a blanket impl might prevent any embedded-hal implementation from implementing both traits (not sure if this is a problem, just raising the concern).
4. We release all that as a patch version. We could release it as a minor version, but then we would have to employ the semver trick to prevent splitting the ecosystem, and I see little reason to do all that work, if we're not introducing breaking changes anyway.

Longer-term, we talk about what the values and priorities should be for the development of this project, as mentioned above. We probably won't come to a consensus here, but I think it's fine to come up with a few drafts and decide on one using majority vote.

Thoughts?

[therealprof]

Sounds good.

[ryankurte]

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?

Therefore I think it's worth putting effort into finding an approach that can potentially allow us to end up with a simpler end result, without causing pain right now.

I agree also. My perspective is that the hal exists to provide simple compatibility, and anything that can result in fragmentation of implementations is worth being wary of. There are a lot of people using the project now, hopefully in the future it will be orders of magnitude more, but it does depend on us getting this right.

Given that, I'd like to propose the following. Short-term (i.e. right now), we do this ...

Sounds good to me.

[hannobraun]

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.

[ryankurte]

Hey all, I raised in #108 that I think we need to provide adaptors between the new/old traits so we don't end up with fragmentation / incompatible implementations (though under a separate PR would be fine), any disagreement with that?

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? Do you have an alternative approach we could explore, and what testing would you like to see before we go ahead with one?

[hannobraun]

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.

[ryankurte]

more discussion of how cursed breaking updates are in #135 :-(

[ryankurte]

Following the hal team meeting and updated process I think we can close this.

Thank you all for your attention and effort!