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

Use statically typed time consistently #231

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

TheZoq2
Copy link
Member

@TheZoq2 TheZoq2 commented Jun 20, 2020

Closes #230

Replaces the uses of _ms and _us postfix variables with Microsecond and Millisecond types. I also renamed the types to use a lowercase S which I believe is more correct. It's the way wikipedia spells it at least :) https://en.wikipedia.org/wiki/Microsecond

This is built on top of #229 as that was where a lot of the us occurrences were.

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jun 20, 2020

Oh, and I want to do another patch release with some doc improvements and some non-breaking PRs, so we should probably hold off on merging this until that is done.

@@ -1,16 +1,17 @@
//! Inter-Integrated Circuit (I2C) bus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for the comment style change? C++ comments are not so typical in the Rust world.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I think I started adding a larger doc chunk, but then changed my mind about what I wanted to do. I'll revert it until I get around to adding i2c docs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even then it should be possible to use Rust style comments. ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I kind of dislike that for large blocks, especially inserting new lines

Copy link
Member

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the usual lack of a CHANGELOG.md entry, looks good to me. 😅

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jun 20, 2020

Of course, I can't just break the tradition of forgetting to include changelogs with changes :)

@burrbull
Copy link
Member

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jun 21, 2020

Yes, that's probably a better idea than using our own structs. I'll port our code over to that

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jun 25, 2020

Looks like using unsigned base types with embedded_time is a bit tricky FluenTech/embedded-time#19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Statically typed time
3 participants