Implement embedded-io Read + Write for UartPeripheral

[Sympatron]

No description provided.

[jannic]

Thanks for this pull request. I think this is a very useful addition.

Does enabling the embedded-io feature have any disadvantages? (Besides slightly increasing the compile time, of course. Which should be tolerable, embedded-io isn't a large library.) I wonder if it would be even better to add it unconditionally.

[jannic]

What's the benefit of making it mandatory ?

• simplicity (adding too many feature flags without good reason just makes the crate difficult to use)
• test coverage
  • thanks to cargo hack --optional-deps --each-feature we actually do test that the feature itself compiles
  • but we don't test every combination (that would be --feature-powerset and can quickly become costly)

Good reasons to have feature flags include:

• things that are causing runtime overhead people might want to avoid (eg. rom-func-cache)
• things that are very costly to build
• things that just don't work for everyone (eg. rom-v2-intrinsics)
• things that are experimental (eg. eh1_0_alpha)

I don't think embedded-io falls within any of these categories. But the list is not exhaustive - am I missing something that would apply to embedded-io?

[Sympatron]

GH seems to think there still is a change request...

[ithinuel]

IMHO, we should be consistent with our embedded-hal's support.

So, our default hal impl is for 0.2.7 (for now).
If we are to implement embedded-io support, then it should follow the same model as the eh1.0-rc.* (using a feature), or have it unconditionally & remove the eh1.0 feature.

Also, for consistency with eh1.0 implementation, the embedded_io::Error should be in uart::reader next to the error type's definition.

However, as far as I am concerns, those are nits so it's only an opinion that I don't hold strongly.

re-reviewed already.

[jannic]

Regarding eh1.0, the planned release date for that is in two weeks, an I expect that we'll remove the feature flag after that.
So even for consistency, I wouldn't add a feature flag hiding the embedded-io impls, just to remove it again in two weeks.

[thejpster]

I would bundle it under the eh-1.0 feature flag, to be honest, because it's effectively where the eh-0.2 serial traits went to after they were taken out of eh-1.0.

[jannic]

I would bundle it under the eh-1.0 feature flag, to be honest, because it's effectively where the eh-0.2 serial traits went to after they were taken out of eh-1.0.

But will we still have an eh-1.0 feature flag once eh-1.0 is released? The reason we have one now is because it's an unstable feature, not covered by stability guarantees. But once it's released, I'd prefer removing the flag entirely.

As the patch uses a released version of embedded-io, I see no reason to hide it behind a feature flag.

[thejpster]

No, I expect the eh-1.0 flag to go away. I was just thinking of (eh-1.0, e-io-0.6) as a pair, which have the equivalent functionality of (eh-0.2). You could just add e-io-0.6 support right now.