Skip to content

Conversation

reitermarkus
Copy link
Member

Add method for software reset via the “General Call” address.

@reitermarkus reitermarkus requested a review from a team as a code owner August 27, 2022 22:56
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ryankurte (or someone else) soon.

Please see the contribution instructions for more information.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

This is an interesting addition which seems fine to me as the traits stand.
However, I wonder how it would look after #392.
Specially there I think there is a non-negligible risk of people thinking it is only going to reset their device and not every device connected (which supports it) since they get access to the bus through an I2cDevice.

device.transaction(|bus| {
    bus.software_reset()
});

I see that software_reset_on_every_device_which_supports_it() seems wonky, though.
I think this would be an interesting point to add to the comments I added here.

PS: I added this to the 1.0.0 milestone only because we need to decide on whether to merge this or not before 1.0.0 due to it being a breaking change.

Comment on lines +12 to +15
### Added

- `I2c::software_reset` for resetting all (supported) devices connected to the bus.

Copy link
Member

Choose a reason for hiding this comment

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

This should be added to the unreleased section

@eldruin eldruin added this to the v1.0.0 milestone Aug 30, 2022
@eldruin
Copy link
Member

eldruin commented Aug 30, 2022

We discussed this in the meeting today and decided against merging it due to a number of reasons:

  • It does not seem anybody would need anything else than the default implementation, so it would not build an abstraction that implementations later define, like with the rest of traits.
  • It is trivial to do in user code if needed: bus.write(0x0, &[0x06]).
  • It has potential for unintended foreign device resets.
  • Support for I2C General Call Reset command is rare.

Nevertheless, thank you very much for the proposal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants