Skip to content

Impl Read<u8> / Write<u8> for Serial #167

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

Closed
dbrgn opened this issue Jan 17, 2020 · 3 comments · Fixed by #171
Closed

Impl Read<u8> / Write<u8> for Serial #167

dbrgn opened this issue Jan 17, 2020 · 3 comments · Fixed by #171

Comments

@dbrgn
Copy link
Contributor

dbrgn commented Jan 17, 2020

Is there a reason why Read / Write is only implemented for Rx and Tx (obtainable via split()) but not on the Serial type directly?

Other HALs like the stm32f4xx-hal have such impls.

I'm working with a driver that wants a Read<u8> + Write<u8> type and I don't want to create a wrapper type with my own impl if not necessary 🙂

@TheZoq2
Copy link
Member

TheZoq2 commented Jan 17, 2020

I don't believe there is an underlying reason, it's just that nobody has gotten around do doing it yet :)

@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 17, 2020

Hm, I tried to implement it, but I realized that for this struct:

pub struct Serial<USART, PINS> {
    usart: USART,
    pins: PINS,
}

...the Read / Write traits cannot be implemented, as it is not known how to get to the pin types from the generic PINS type argument.

One workaround would be to implement Read / Write for a tuple (Tx, Rx), but that's a bit of a hack in my opinion.

The better solution in my eyes would be to replace the Pins trait with separate TxPin / RxPin traits, as I did in the stm32l0xx-hal. Any opinions on that? It would be a breaking change, but we're still on version 0.x so I guess that's fine 🙂

@TheZoq2
Copy link
Member

TheZoq2 commented Jan 17, 2020

Any opinions on that? It would be a breaking change, but we're still on version 0.x so I guess that's fine slightly_smiling_face

Sounds like a good idea. We have breaking changes all the time 😉

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 a pull request may close this issue.

2 participants