Added own timer. by shilingwangggg · Pull Request #468 · google/OpenSK

shilingwangggg · 2022-04-26T19:26:32Z

Create Timer trait for future use. Implement Timer using libtock core.

src/timer.rs

ia0 · 2022-05-03T19:03:25Z

src/timer.rs

@@ -0,0 +1,70 @@
+trait Timer: Sized {
+    fn start(milliseconds: u32) -> Self;
+    fn has_elapsed(self) -> Option<Self>;


Let's add some documentation for those 2 functions.

Documentation in Rust use three slashes:

/// Creates and starts a timer. fn start(milliseconds: u32) -> Self; /// Returns the remaining timer of a timer. /// /// In particular, if the timer has elapsed, `None` is returned. fn has_elapsed(self) -> Option<Self>;

And actually I would rename has_elapsed to is_running, because Some usually means true.

src/timer.rs

ia0

Looks good. I guess we should try using it in this PR directly. Otherwise we don't know if it works.

ia0 · 2022-05-17T10:11:43Z

src/timer.rs

@@ -0,0 +1,91 @@
+// Copyright 2019 Google LLC


You can change the year to 2022. This is all new code.

ia0 · 2022-05-17T10:17:35Z

src/timer.rs

@@ -0,0 +1,70 @@
+trait Timer: Sized {
+    fn start(milliseconds: u32) -> Self;
+    fn has_elapsed(self) -> Option<Self>;


Documentation in Rust use three slashes:

/// Creates and starts a timer. fn start(milliseconds: u32) -> Self; /// Returns the remaining timer of a timer. /// /// In particular, if the timer has elapsed, `None` is returned. fn has_elapsed(self) -> Option<Self>;

And actually I would rename has_elapsed to is_running, because Some usually means true.

ia0 · 2022-06-21T16:11:48Z

src/ctap/main_hid.rs

-    pub fn should_wink(&self, now: CtapInstant) -> bool {
-        self.wink_permission.is_granted(now)
+    pub fn should_wink(&self) -> bool {
+        self.wink_permission.is_some() && self.wink_permission.unwrap().has_elapsed().is_some()


Interesting. First I'm surprised this compiles. Second, the signature of should_wink might be problematic with the has_elapsed API. I see at least 2 options:

should_wink only returns the status since last update, i.e. self.wink_permission.is_some() (my preference)

should_wink is &mut self and also updates the timer.

ia0 · 2022-06-21T16:13:05Z

src/ctap/main_hid.rs

-    pub fn update_wink_timeout(&mut self, now: CtapInstant) {
-        self.wink_permission = self.wink_permission.check_expiration(now);
+    pub fn update_wink_timeout(&mut self) {
+        self.wink_permission = Some(LibtockAlarmTimer::start(Self::WINK_TIMEOUT_DURATION));


This looks wrong to me. I would expect this to update the timer, not reset it.

self.wink_permission = self.wink_permission.and_then(LibtockAlarmTimer::has_expired);

ia0 · 2022-06-21T16:14:26Z

src/ctap/mod.rs

+    pub fn new_reset() -> StatefulPermission {
        StatefulPermission {
-            permission: TimedPermission::granted(now, RESET_TIMEOUT_DURATION),
+            timer: Timer::start(30000),


Why not use the constant?

ia0 · 2022-06-21T16:16:30Z

src/ctap/mod.rs

-    pub fn clear_timer(&mut self, now: CtapInstant) {
-        if !self.permission.is_granted(now) {
+    pub fn clear_timer(&mut self) {
+        if self.timer.is_some() && self.timer.unwrap().has_elapsed() {


We should first update the timer, then check if elapsed.

self.timer = self.timer.and_then(has_expired); if self.timer.is_none() { self.clear(); }

ia0 · 2022-06-21T16:17:23Z

src/ctap/token_state.rs

 /// NFC only allows 19.8 seconds.
 /// TODO(#15) multiplex over transports, add NFC
-const INITIAL_USAGE_TIME_LIMIT: Milliseconds<ClockInt> = Milliseconds(30000 as ClockInt);
+const INITIAL_USAGE_TIME_LIMIT:u32 = 30000_u32;


What's the unit?

ia0 · 2022-06-21T16:24:17Z

src/ctap/token_state.rs

        }
-        self.usage_timer = self.usage_timer.check_expiration(now);
-        if !self.usage_timer.is_granted(now) {
+        if self.usage_timer.is_none() || self.usage_timer.unwrap().has_elapsed().is_none() {


This should also update the timer. We could actually provide a function for convenience:

/// Updates the expiration of a timer. pub fn update_timer<T: Timer>(timer: &mut Option<T>) { *timer = core::mem::replace(timer, None).and_then(T::has_expired); }

kaczmarczyck · 2023-02-24T17:45:27Z

Picked up by #596

shilingwangggg requested a review from ia0 April 26, 2022 19:26

ia0 reviewed Apr 27, 2022

View reviewed changes

src/timer.rs Outdated Show resolved Hide resolved

src/timer.rs Outdated Show resolved Hide resolved

src/timer.rs Outdated Show resolved Hide resolved

src/timer.rs Outdated Show resolved Hide resolved

src/timer.rs Outdated Show resolved Hide resolved

ia0 reviewed May 3, 2022

View reviewed changes

ia0 reviewed May 17, 2022

View reviewed changes

ia0 reviewed Jun 21, 2022

View reviewed changes

shilingwangggg force-pushed the feature/own-timer branch from 8a718f9 to 22f829a Compare July 9, 2022 09:14

shilingwangggg added 6 commits July 17, 2022 19:42

Added own timer.

fea0a73

Only store end tick in LibtockAlarmTimer

dbc4453

Update the copyright as well as some nit comments

ad68ec8

Format the file

b75c780

Only store end tick in LibtockAlarmTimer

74048a4

Put timer into env

8922b34

shilingwangggg force-pushed the feature/own-timer branch from 22f829a to 8922b34 Compare July 17, 2022 17:46

kaczmarczyck mentioned this pull request Feb 24, 2023

Clock trait #596

Merged

kaczmarczyck closed this Feb 24, 2023

Conversation

shilingwangggg commented Apr 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaczmarczyck commented Feb 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants