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

Fulfillment of the I2c trait contract #85

Merged
merged 5 commits into from
May 8, 2024

Conversation

ohunter
Copy link
Contributor

@ohunter ohunter commented May 3, 2024

This seeks to address the issue with the LinuxI2CDevice not correctly implementing the I2c trait as mentioned in rust-embedded/linux-embedded-hal#82. I am not sure whether the fix really belongs in this repo but since i2c_msg.flags isn't pub then it doesn't seem like there is another way. Setting the flag when the device doesn't implement the feature seems to have no effect which might lead to some confusion.

@ohunter ohunter requested a review from a team as a code owner May 3, 2024 08:25
Copy link
Member

@nastevens nastevens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! I'm worried that this adds a new panic case if the messages input is empty and would like to discuss fixing before this is merged.

src/linux.rs Outdated Show resolved Hide resolved
src/linux.rs Outdated Show resolved Hide resolved
src/linux.rs Outdated Show resolved Hide resolved
nastevens
nastevens previously approved these changes May 7, 2024
@nastevens nastevens enabled auto-merge May 7, 2024 13:32
@nastevens
Copy link
Member

Looks good now! Thanks for your efforts!

@nastevens nastevens disabled auto-merge May 7, 2024 13:50
src/linux.rs Outdated Show resolved Hide resolved
nastevens
nastevens previously approved these changes May 7, 2024
@nastevens
Copy link
Member

Sorry, now rustfmt is unhappy 😮‍💨

@nastevens nastevens enabled auto-merge May 8, 2024 13:50
@nastevens nastevens added this pull request to the merge queue May 8, 2024
Merged via the queue into rust-embedded:master with commit a7a4dce May 8, 2024
8 checks passed
@nastevens
Copy link
Member

Thanks again @ohunter, sorry it took a few tries to get things merged!

@ohunter
Copy link
Contributor Author

ohunter commented May 8, 2024

No worries. I'm just happy to contribute 😀

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