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.

Sign up for GitHub

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

Jump to bottom

Feature/timeint #453

Closed
wants to merge 1 commit into from
Closed

Feature/timeint #453

wants to merge 1 commit into from

Conversation

shilingwangggg
Copy link
Collaborator

To discuss

@kaczmarczyck kaczmarczyck changed the base branch from stable to develop April 5, 2022 23:29
ia0
ia0 reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

To help development you can declare the module in src/api/mod.rs. This way the compiler will compile it (and that's also needed in the end).

src/api/clock.rs Outdated
use embedded_time::TimeInt;
use embedded_time::fraction::Fraction;

#[derive(Clone, Copy, PartialOrd, Ord, Eq, fmt::Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't derive PartialOrd and Ord. If we need those traits, we should implement it ourselves because the derived implementation is wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not understand why the derived implementation is wrong. Using our creation functions ( new(), new_wrap()) we are assured that only the 24 LSB of the u32 are non-zero. Therefore, it looks safe to me that PartialOrd and Ord hold. Do I miss anything?

src/api/clock.rs Outdated Show resolved Hide resolved
@shilingwangggg
Copy link
Collaborator Author

I implemented all required traits on U24 to make it a valid TimeInt.

The user code of TimeInt requires quite some update too because we are currently using TimeInt in many const context. However, with U24 we do not have a primitive type anymore, and therefore operators such as as are not available, std::ops cannot be declared consts without using an unstable feature, and from is also non-consts.

Do you have any idea how should I proceed?

@kaczmarczyck kaczmarczyck requested a review from ia0 April 11, 2022 22:39
ia0
ia0 reviewed Apr 12, 2022
View reviewed changes
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

I think this shows that we don't want to use embedded-time and use our own abstraction instead. I think this is a bug in embedded-time to ask TimeInt to implement Integer (etc) when it really should only use wrapping operations. I think they ask for that because they use the same representation for ticks and time AFAICT. And in our case we don't want that. We want to use non-wrapping usize for time (seconds, etc) and wrapping u24 for ticks. I say non-wrapping because it's probably a bug to have wrapping seconds, while doing it at the ticks level is expected.

src/api/mod.rs
@@ -5,3 +5,4 @@

pub mod firmware_protection;
pub mod upgrade_storage;
pub mod clock;
Copy link
Member

Choose a reason for hiding this comment

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

This should be sorted.

src/clock.rs
@@ -4,6 +4,8 @@ use embedded_time::duration::Milliseconds;
pub use embedded_time::Clock;
#[cfg(not(feature = "std"))]
use libtock_drivers::result::FlexUnwrap;
#[cfg(target_pointer_width = "32")]
Copy link
Member

Choose a reason for hiding this comment

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

I think we always want to use U24 for now, in both test and prod.

src/api/clock.rs

impl num::traits::WrappingSub for U24 {
fn wrapping_sub(&self, v: &Self) -> Self {
Self::new_wrap(self.get().wrapping_sub(v.get()))
Copy link
Member

Choose a reason for hiding this comment

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

We need to & 0xffffff here and in WrappingAdd too.

@kaczmarczyck kaczmarczyck mentioned this pull request Feb 24, 2023
Merged
@kaczmarczyck
Copy link
Collaborator

Picked up by #596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ia0 ia0 ia0 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shilingwangggg @kaczmarczyck @ia0