Skip to content

CAN: returning Result instead of Option (#741) - #749

Open
jorgeandrecastro wants to merge 7 commits into
rust-embedded:masterfrom
jorgeandrecastro:fix-can-result-741
Open

jorgeandrecastro wants to merge 7 commits into
rust-embedded:masterfrom
jorgeandrecastro:fix-can-result-741

Conversation

@jorgeandrecastro

Copy link
Copy Markdown

Hello embedded-hal team,

I’m submitting this pull request to address issue #741.

To do so, I modified the methods that previously returned an Option to signal logic errors (such as invalid IDs or payloads that are too long). They now return a Result instead, using ErrorKind::InvalidId and ErrorKind::DataTooLong to provide more meaningful error information.

Changes I made

Updated StandardId::new and ExtendedId::new in id.rs so that they return Result<Self, ErrorKind> instead of Option.

Updated the Frame trait definitions in lib.rs (new and new_remote) so that they return Result<Self, ErrorKind>.

Added corresponding unit tests and clear English comments to ensure the expected behavior and make the code easier to maintain.

I hope this pull request addresses the issue and can be useful to embedded developers.

I’d be happy to discuss the changes with you and receive any feedback or suggestions.

Thank you for your time and for reviewing my contribution!

@jorgeandrecastro
jorgeandrecastro requested a review from a team as a code owner September 28, 2026 13:00
@jorgeandrecastro

Copy link
Copy Markdown
Author

Hello!

My code changes are ready, but the MSRV check is currently failing on CI. After investigating locally, it turns out the failure is caused by an external dependency (cortex-m-macros) which now requires Rust 1.85 / Edition 2024, whereas the current MSRV for this crate is set to 1.83.

Since this is coming from a third-party dependency update outside of this PR, I wanted to point it out. Please let me know how you would prefer to handle this (e.g., bumping the MSRV or locking the dependency version). Thank you!

@adamgreig

Copy link
Copy Markdown
Member

Please could you bump the MSRV in this pull request?

@jorgeandrecastro

Copy link
Copy Markdown
Author

Please could you bump the MSRV in this pull request?

Hello !, Its done! I've bumped the MSRV to 1.85 across the affected workspace crates (embedded-can, embedded-io, embedded-hal, etc.) and updated the CI workflow (test.yml) accordingly.

All CI checks are now passing successfully!

This branch has not been deployed

No deployments
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

Successfully merging this pull request may close these issues.

2 participants