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

Refactor serial code #8

Merged
merged 5 commits into from Jun 7, 2019

Conversation

@hannobraun
Copy link
Member

commented Jun 7, 2019

See commit messages for detailed explanation and justification.

This is based on #7, which needs to be reviewed and merged before this is ready for review.

hannobraun added some commits Jun 7, 2019

Merge `serial/macros.rs` into `serial/mod.rs`
This doesn't make a whole lot of sense by itself, but it prepares for
the next refactoring step I'm about to do. Please bear with me.
Move serial implementation code out of macro
Having all this code in a macro has numerous disadvantages:
- The code is less readable, as you often have to jump between the code
  itself and the arguments passed into the macro.
- The code is deeply indented, and the macro can sometime confuse the
  syntax highlighting, both of which exacerbate the readability problem.
- Any compiler error in the macro shows up multiple times, for each
  implementation that the macro generates.
- Multiple nearly identical methods are generated. I haven't checked
  whether the affects the size of the compiled binary, but even if the
  compiler is smart enough to optimize this, why not unify the common
  code into one method.

I'm sure there are more, but this is what I can think of off the top of
my head.

This commit replaces the big-macro approach with the following:
- The bulk of the code is moved out of the macro.
- The out-of-macro code is made generic over the USART instance.
- A trait is implemented for all USART instances, which abstracts over
  the differences between them.
- The implementations of this traits are the last thing left in the
  macro.

I realize this breaks with tradition, but for the reasons mentioned, I
think this approach is superior to the classic "everything in the macro"
approach. There's also prior art in the nrf-rs[1] and lpc-rs[2] worlds.

[1] https://github.com/nrf-rs/
[2] https://github.com/lpc-rs/

@hannobraun hannobraun force-pushed the braun-embedded:usart branch from 54d8123 to 85726ff Jun 7, 2019

@hannobraun hannobraun marked this pull request as ready for review Jun 7, 2019

@hannobraun

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Rebased, ready for review.

@mvertescher

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Awesome, looks much better to me! Thanks again @hannobraun!

@mvertescher mvertescher merged commit 600834d into stm32-rs:master Jun 7, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@hannobraun hannobraun deleted the braun-embedded:usart branch Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.