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

Finish my current round of clock source clean-up #239

Merged
merged 4 commits into from
May 26, 2020
Merged

Finish my current round of clock source clean-up #239

merged 4 commits into from
May 26, 2020

Conversation

hannobraun
Copy link
Member

The clock source code is not perfect by any means, but I think this PR leaves it in a good enough state to focus on other stuff for a while.

The `&()` I had to add isn't pretty, but I think it's worth it. We can
define more targeted types in the future, to make this more
understandable. And to fix (insert words to prevent GitHub from closing
that issue) #205, we might have to anyway.

In any case, this is how far I'm willing to go right now, and I think
it's good enough.
Maybe the duplication could be removed, but I think this would require a
closer reading of the user manual than I'm willing to do right now.

I don't feel the need to make this perfect right now, since there's very
little duplication left anyway. And since we likely want to support
other main clock frequencies at some point, we'll have to revisit this
code then anyway.
I believe keeping the target-specific code small and inline is more
readable than having the sub-module, and this is the style I've already
been implementing in other code.
@hannobraun hannobraun requested a review from david-sawatzke May 25, 2020 10:29
@hannobraun
Copy link
Member Author

The Travis build fails on nightly only, because rustfmt isn't available. Stable and beta build fine.

@david-sawatzke david-sawatzke merged commit 71e4aac into lpc-rs:master May 26, 2020
@hannobraun hannobraun deleted the clock-source branch May 27, 2020 09:56
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