Add AcknowledgeCheckFailedReason

[bjoernQ]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• I have added necessary changes to user code to the Migration Guide.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

This prepares for a maybe-future breaking change when we want to give the user an indication of an AckCheckFailed reason. For now it's just using Unknown but changing that shouldn't be a breaking change in future

Testing

CI

[bjoernQ]

Prepares #2814

[Dominaezzz]

Why is this non_exhaustive, isn't this enum complete? What else is there for the device to not acknowledge?

(Using both Unknown and non_exhaustive seems wrong to me at a glance)

[bjoernQ]

In future we might want to provide more information with the error (e.g. at which byte count the nack was detected) - adding such a variant would be a breaking change then. I imagine user-code will do the same when matching Unknown or _ so I don't think it's a big deal

I agree for now we wouldn't even need Address and Data for now, then

[bugadani]

Wouldn't such a change be breaking (as we'd add the info to Data)? Or how do you imagine such change to manifest?

[bjoernQ]

if we change the variant that would be breaking - but we could add another variant - but I agree we should just remove the currently unused variants for now

[Dominaezzz]

if we change the variant that would be breaking - but we could add another variant - but I agree we should just remove the currently unused variants for now

Adding a variant can still be a breaking change, not at compile time but at runtime. Errors are tricky.

If you add a new variant that is a subset of an existing variant, this is a breaking change.

Imagine a user doing something like this. (Based on the documentation this is acceptable to do)

if err == AcknowledgeCheckFailedReason::Unknown {
    // do something because address might not have been acknowledged.
}

If you split Unknown into Unknown and Address, once the user pulls in the new version of esp-hal, this if statement no longer executes where they expected it to.

[bjoernQ]

yes but that is true for pretty much every error-enum we have

checking an error like that is also bad for exhaustive enums

[Dominaezzz]

My code example isn't fantastic but it's easy for code to semantically look like that.

Perhaps I'm being paranoid, I can't think of a nice solution besides completing the enum.

[bugadani]

There is precedent in rust std to introduce such breaking changes. We can't prevent users from explicitly matching against Unknown but we can document that we don't guarantee that we don't split error variants out of it, and it should be handled as the "nothing else applies" case.

That being said, we should be extra careful with these cases. We should probably add and handle Address and Data right now, and accept that they won't carry additional data, or include an empty struct in them if we know we might have something useful in them later.

[JurajSadel]

Thanks!