Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: Remove ErrorKind from digital and pwm modules #527

Open
madsmtm opened this issue Nov 21, 2023 · 1 comment
Open

Suggestion: Remove ErrorKind from digital and pwm modules #527

madsmtm opened this issue Nov 21, 2023 · 1 comment

Comments

@madsmtm
Copy link

madsmtm commented Nov 21, 2023

embedded-hal defines its own Error traits for each submodule, which makes sense since it wants the kind method.

I think this trait should've been a subtrait of core::error::Error, though I get why it isn't - Display isn't as useful, and the standard library's error trait is not yet stable in core - so I won't belabour this point further, it's more important to ship a v1.0 than to wait for that stabilization.

However, both embedded_hal::digital::ErrorKind and embedded_hal::pwm::ErrorKind only contain one variant Other, which means it is currently completely useless. So I'd suggest to remove these for now, until the need arises.

This would be unlikely to be SemVer breaking to change back in the future (you could re-add the method with a default implementation of ErrorKind::Other), if we do find that we want to re-add error kinds for the digital and pwm modules.

@Dirbaio
Copy link
Member

Dirbaio commented Nov 25, 2023

However, both embedded_hal::digital::ErrorKind and embedded_hal::pwm::ErrorKind only contain one variant Other, which means it is currently completely useless. So I'd suggest to remove these for now, until the need arises.

it's not useless, HALs can still return their own error enums with more variants than the ones defined in embedded-hal.

This would be unlikely to be SemVer breaking to change back in the future (you could re-add the method with a default implementation of ErrorKind::Other), if we do find that we want to re-add error kinds for the digital and pwm modules.

if you mean removing ErrorType entirely and adding it back: it's a breaking change to go from trait OutputPin to trait OutputPin: ErrorType.

if you mean removing ErrorKind and fn kind() only: I still think consistency in how errors work across all traits is important. Also doing this would make trait Error be empty, which would be odd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants