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

Impl Peripheral for I2s trait #336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bbutkovic
Copy link

This PR changes the I2s trait so that it is a superset of the Peripheral trait.

I recently got into a situation where I was trying to pass a PeripheralRef<'_, I2s> into I2sDriver::new_std_tx and found it problematic because the I2s trait does not necessarily guarantee it is a Peripheral, rather I2S0 and I2S1 structs implement Peripheral, making it hard to go about generic implementations.

This change will make it a little bit easier to work with the API when dealing with generics.

@bbutkovic bbutkovic marked this pull request as ready for review November 9, 2023 17:40
@ivmarkov
Copy link
Collaborator

ivmarkov commented Nov 11, 2023

This PR changes the I2s trait so that it is a superset of the Peripheral trait.

I recently got into a situation where I was trying to pass a PeripheralRef<'_, I2s> into I2sDriver::new_std_tx and found it problematic because the I2s trait does not necessarily guarantee it is a Peripheral, rather I2S0 and I2S1 structs implement Peripheral, making it hard to go about generic implementations.

This change will make it a little bit easier to work with the API when dealing with generics.

Why do you need to use PeripheralRef in your own code in the first place? The only reason why this type is public is so that it can be re-used in the esp-idf-svc crate. Yes, I know this is a bit of a code smell, but it is not supposed to be used by users directly.

So can you elaborate?

@bbutkovic
Copy link
Author

The only reason why this type is public is so that it can be re-used in the esp-idf-svc crate.

It looks like I misunderstood the documentation then, my bad!

As I mentioned in #334, I'm working on a wrapper around the I2S driver and without the reconfiguration support I found it suitable to store a PeripheralRef in my struct and then use that later to create the I2s driver.

@ivmarkov
Copy link
Collaborator

@bbutkovic Were you able to NOT use PeripheralRef in your code? It should be well possible. I really don't think we can proclaim I2c as extending Peripheral. Does not feel right...

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