Correct/clarify TimerEvent::insert documentation #14005
Conversation
ciarmcom
added
the
release-type: patch
|
@kjbracey-arm, thank you for your changes. |
There was much confusion over the functionality of the original `TimerEvent::insert` call which was described as "Set relative timestamp of the internal event". This then extended to my Chrono conversion, meaning the new `insert` call is not equivalent. Clarify the original documentation, correct the deprecation messages, and add more notes on conversion. No functional change, as the new Chrono API makes more sense - it's just different from the old API. Problem actually spotted when I saw the strange code `convert_timestamp` was producing for the 32-bit->64-bit timestamp conversion. The caller of it was actually making the mistake of issuing "TimerEvent::insert(rel_timeout)`, meaning they'd also misunderstood the documentation, and were not getting the timeout they expected. (Chrono would have prevented that mistake as durations and time points are incompatible types).
There was a problem hiding this comment.
Thanks for the documentation update. I confirm that insert_absolute() was added as an equivalent to insert() to support 64-bit timestamp, see #4094, so both are absolute. I guess "relative" (in that PR) initially meant relative to the current 32-bit cycle, instead of relative to the current time.
It feels to me the non-chrono insert() would've required a proper fix to align with the chrono one, if it weren't deprecated. But as we don't want behaviour changes on something deprecated, this change looks good.
|
Thanks for finding the history! Even if the old |
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.6.0
Release-pending
and removed
release-type: patch
Summary of changes
There was much confusion over the functionality of the original
TimerEvent::insertcall which was described as "Set relative timestamp of the internal event".This then extended to my Chrono conversion, meaning the new
insertcall is not equivalent - it really is relative.Clarify the original documentation, correct the deprecation messages, and add more notes on conversion.
No functional change, as the new Chrono API makes more sense - it's just different from the old API.
Problem actually spotted when I saw the strange code
convert_timestampwas producing for the 32-bit->64-bit timestamp conversion. The caller of it was actually making the mistake of issuingTimerEvent::insert(rel_timeout), meaning they'd also misunderstood the documentation, and were not getting the timeout they expected.(Chrono would have prevented that mistake as durations and time points are incompatible types).
Impact of changes
Probably not worth putting anything specific in release notes - the
insertcall was already deprecated (probably should have been double deprecated, given the 64-bit API). Anyone actually using it must have already spotted the issue during conversion. I don't thinkTimerEventis used much by apps directly - it's a building block forTimeoutandTicker, which are more commonly used.Migration actions required
Documentation
None
Pull request type
Test results
Reviewers