Update for deprecation of non-Chrono timing APIs#112
Update for deprecation of non-Chrono timing APIs#112kjbracey wants to merge 3 commits intoARMmbed:masterfrom
Conversation
|
The failure is because of astyle checks, it is been fixed, on the master, rebase would resolve the issue |
|
Build failures now look like what I'd expect before Mbed OS gets the associated PR. |
|
@kjbracey-arm could you rebase, the CI build should be successful now. |
|
Rebased. |
|
@kjbracey-arm I just came across this having already fixed a couple of the examples. After I get my next fix in , would you be able to adjust this PR accordingly so we can actually get it in? Not sure why it didn't go in at the time :( |
|
Oh, distant memories. Sure, I can. Ping me again. Although if anyone's been doing significant example messing, it might not be a simple rebase. Might need more search-and-replace? Can't remember how I found the places to fix originally... |
|
@kjbracey-arm I've only fixed the event and eventQueue examples :) |
Timer functions now use chrono values, replace deprecated calls to library. Replace integer time values with chrono durations.
|
@kjbracey-arm I've been working on updating the values and have picked up all of your changes. Rather than create a new PR, can I push my changes to your branch? |
|
@adbridge, @kjbracey-arm I've fixed some minor issues that were causing build errors, I believe the PR is ready for review now. Could you please take a look when you have a chance? |
kjbracey
left a comment
kjbracey
left a comment
There was a problem hiding this comment.
Looks fine to me, except the "Alarm" example is a bit glitched. I think that might be my fault - I remember stumbling on it a bit.
| } else { | ||
| delay += MINUTE; | ||
| min_count++; | ||
| delay = +min_inc++; |
There was a problem hiding this comment.
Typo here. Not += like above. But then I think it's still wrong
Surely this and the above should look like.
delay += 1min;
min_inc++;
It's still a bit wonky, because there's no compelling reason to track both delay and min_inc. And why even bother making min_inc be Chrono if it's just a counter?
So, instead, you could forget the delay increment, just do min_inc++ and have
auto delay = hour_inc + min_inc;
in main - making the Chrono nature of those two variables useful.
There was a problem hiding this comment.
@kjbracey-arm thanks for the feedback! I've cleaned it up now and tested the changes to make sure everything works as expected.
Remove increments of delay, instead only increment chrono min_inc and hour_inc values. Allows improved utilisation of chrono values and removes redundant operations to track the delay variable.
kjbracey
left a comment
kjbracey
left a comment
There was a problem hiding this comment.
Looks good to me. Can't hit "approve" because it's my own pull request :)
|
@adbridge could you take a look at this since you're the only one who can approve in this scenario? |
|
I was going through the docs and noticed a couple of discrepancies between the example descriptions and the actual code (e.g. Kernel::Clock example here) - which I can see this PR fixes, so just wanted to ping you @adbridge for potentially picking back up reviewing/approving this. I hope that's not too annoying of me! |
6 participants
Updates corresponding to ARMmbed/mbed-os#12425.