-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat(crux_time): cancellable timer #279
feat(crux_time): cancellable timer #279
Conversation
c43b7bd
to
86d1ecf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, we've had something like that in mind as well, just didn't get around to it. Thank you!
Can I ask what the use for this was that you found?
I was persisting the timer fire date and resuming it upon app reload and of course I didn't want the timer to live forever, hence the need to clear it. |
Makes a lot of sense! We've got an issue in the backlog to think about how we do effect cancellation universally for all effects (#247), but this is good for timer 👍🏻 |
@charypar I don't think the ci failure is related with the changes I made, can you take a look or re-run? 🙏 |
Yea, that looks like a |
@PatrykBuniX my apologies. I really should have published I'll merge it anyway! Thanks for your help with this! |
Hi! I was playing around with the
time
capability and realised there's no way to clear the pending timer. This PR adds this possibility.This PR adds
TimeId
field for relevant requests/responses ofcrux_time
capability. Since we know the id of the timer we are able to clear it using the new time request:TimeRequest::Clear {id: TimeId}
. Once core notifies the shell (notify_shell
since we don't expect any response to this particular request) with this effect, the shell should fire the initially set timer sooner with aTimeResponse::Cleared
instead ofTimeResponse::InstantArrived
/TimeResponse::DurationElapsed
.I'm open to any suggestions, I'm also aware that this is a breaking change, maybe you guys see a non-breaking solution to this? Otherwise I will just create a custom capability 😉