Skip to content

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

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

Merged
merged 4 commits into from
Jan 18, 2020

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Jan 17, 2020

Fixes #167.

I managed to implement this without having split tx/rx pins in the Serial struct thanks to the help of @Disasm. Instead, it uses a private trait (UsartReadWrite) to avoid having duplicated peripheral access logic. There should be no breaking change in this PR.

@dbrgn dbrgn requested review from TheZoq2 and Disasm January 17, 2020 15:21
@TheZoq2
Copy link
Member

TheZoq2 commented Jan 17, 2020

Seems like a nice and simple change, good job! The last matrix message seems to suggest that you may want to make additional improvements. Is that the case or is it ready for merging?

@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 17, 2020

The last matrix message seems to suggest that you may want to make additional improvements. Is that the case or is it ready for merging?

I couldn't figure out how to implement the change suggested by @Disasm, but maybe I misunderstood something...

To me, it seems better to keep the trait methods as associated functions without a &self reference, since they actually are static and don't require a reference to an instance.

@Disasm
Copy link
Member

Disasm commented Jan 17, 2020

Passing &self ensures ownership, but it is something we do not have in all of the cases anyway. I'm trying to improve this code locally it's dfficult to implement this way. So LGTM. Maybe I will improve this in the future.

@Disasm
Copy link
Member

Disasm commented Jan 17, 2020

Well, I have an update. Should I open a PR to the dbrgn's PR or should I PR my changes here after this PR gets merged?

@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 17, 2020

Well, I have an update. Should I open a PR to the dbrgn's PR or should I PR my changes here after this PR gets merged?

If you want, you can also send me the patch (git show > patch.txt) and I'll add it to this PR 🙂

@Disasm
Copy link
Member

Disasm commented Jan 17, 2020

Here we are: dbrgn#1

@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 17, 2020

Thanks! I merged your changes, will add a comment.

@Disasm
Copy link
Member

Disasm commented Jan 17, 2020

With this implementation only one copy of UsartReadWrite::{read/write/flush} is created in debug, as opposed to the original implementation with macros.

@dbrgn dbrgn force-pushed the read-write-on-serial branch from 917d63c to f13e244 Compare January 17, 2020 17:34
@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 17, 2020

I rebased the branch and squashed my fixup commits. I also integrated the "Strip confusing comment" commit into the commit "Deduplicate USART driver code".

From my side, this PR is now ready.

@Disasm
Copy link
Member

Disasm commented Jan 17, 2020

@TheZoq2 could you review this one more time?

@TheZoq2
Copy link
Member

TheZoq2 commented Jan 18, 2020

Looks great, thank you both for your work!

@TheZoq2
Copy link
Member

TheZoq2 commented Jan 18, 2020

Oh yea, just one minor thing. Would you mind adding a changelog entry before I merge?

@dbrgn dbrgn force-pushed the read-write-on-serial branch from f13e244 to 6dfdef3 Compare January 18, 2020 14:09
@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 18, 2020

Oh yea, just one minor thing. Would you mind adding a changelog entry before I merge?

Sure! Forgot about that. Done!

@TheZoq2
Copy link
Member

TheZoq2 commented Jan 18, 2020

Lovely!

@TheZoq2 TheZoq2 merged commit 358889f into stm32-rs:master Jan 18, 2020
@dbrgn dbrgn deleted the read-write-on-serial branch January 18, 2020 16:01
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.

Impl Read<u8> / Write<u8> for Serial
3 participants