-
Notifications
You must be signed in to change notification settings - Fork 219
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
Monotonic rewrite #874
Monotonic rewrite #874
Conversation
Hi @Finomnis , sorry for the delay on this! Overall it looks like a very nice improvement of the monotonics. Would you like to cleanup the PR to get tests to start passing? |
@korken89 Hey! Sorry for the delay, I was kinda busy with family and stuff. I'll start working on this now again, looking forward to your feedback. |
@korken89 Bump :) |
Is this planned for merging or is there still discussion or more to do? |
Planned for merging, waiting for a review. |
Awesome, I like what you've done and looking forward to it
|
We just merged a large RISC-V support PR that has minor clashes with this PR, could you rebase? |
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.
Some minor comments
Busy at work RN, might take a look later :) |
@korken89 Fixed |
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.
This is really impressive :)
There's a lot to like here, but in particular how clean the actual usage of monotonics became, awesome!
I know @korken89 got much more experience with monotonics/timer queues so he'll get the last word, but I'm happy to see this PR landing :)
@Finomnis Could you please rebase on latest master branch and while doing that remove all merge commits? Might also be a good opportunity to tidy a bit in the commit log 😇 If unsure how, I'm happy to help 👍 |
I planned to do a squash merge, if that's allright. I'm too lazy to clean them up :D |
Why have you downgrade esp32c3 version? |
My bad. Merging gets out of hand with these kind of huge merge requests. I hope it is merged soon. |
@AfoHT Is it alright with you if we squash merge, instead of me cleaning up the commit history? |
@korken89 Did some minor fixes after merging the main branch, should be stable again |
@Finomnis don't worry about the esp32c3 QEMU run :) I'm in the process of squashing/tidying the PR which is about to be merged But if you got more commits coming I can hold off a little, or we just create another PR. What do you think? I'll bring in the 986f959 onto the squashed/rebased WIP I got :) |
Goals: * make Monotonic purely internal * make Monotonic purely tick passed, no fugit involved * create a wrapper struct in the user's code via a macro that then converts the "now" from the tick based monotonic to a fugit based timestamp We need to proxy the delay functions of the timer queue anyway, so we could simply perform the conversion in those proxy functions.
986f959
to
8edd4c3
Compare
I took the liberty of tacking on a tiny CI fix, to get CI happy enough for a merge :) |
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.
Awesome work! 🎉 Thank you!
I hope I didn't miss anything while squashing, but if that is the case that's on me and we can follow up 👍
Very nice |
See #873.