Skip to content

Skip any missed timer events updating the timer to now#104

Merged
mdavidsaver merged 2 commits into
epics-base:masterfrom
kiwichris:kiwichris/fix-time-spin-on-big-time-jump
Dec 6, 2025
Merged

Skip any missed timer events updating the timer to now#104
mdavidsaver merged 2 commits into
epics-base:masterfrom
kiwichris:kiwichris/fix-time-spin-on-big-time-jump

Conversation

@kiwichris

Copy link
Copy Markdown
Contributor

This stops a timer calling the callback for every missed event if the time jumps forward when corrected or moved.

The PR stops the EPICS shell locking up with NTP if no NTP server is available for RTEMS init to use or that request fails. The jump from the RTEMS epoch to the current time resulted the timer making all the missed timer calls which was a lot. The timer and callback looped until finished.

@mdavidsaver mdavidsaver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiwichris Could you describe the situation you are seeing leading to a lockup? Which timer and timer queue are involved? From context, I guess the effect is a live lock?

Comment thread src/misc/timer.cpp Outdated
Comment thread src/misc/timer.cpp Outdated
* the timer's run time to now and avoid the timer
* spinning issuing callbacks for missed timer events
*/
if ((now - work->timeToRun) > work->period) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the earlier

... (waitfor = queue.front()->timeToRun - now) ...

It would make more sense to me to express this condition as:

if (waitfor < -work->period) {

@JJL772

JJL772 commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

@mdavidsaver (per RTEMS Discord discussion) The callback being run due to this bug is epics::pvAccess::ChannelSearchManager::callback()

This stops a timer calling the callback for every missed event if the
time jumps forward when corrected or moved.
@mdavidsaver mdavidsaver force-pushed the kiwichris/fix-time-spin-on-big-time-jump branch from 88deb5a to b02545d Compare December 4, 2025 02:30
@mdavidsaver

Copy link
Copy Markdown
Member

@kiwichris I have rebased this PR, and pushed an additional commit. Please have a look and see if you agree with this addition.

For clarify, I reuse the previously calculated waitfor difference. I also opted to re-fetch the time after execution, which I think might help both in the time jump case, and also in a situation where eg. a periodic callback takes longer than one period to execute. Re-fetching the timer afterwards will effectively add a one period "penalty" to allow other threads some time.

@mdavidsaver mdavidsaver force-pushed the kiwichris/fix-time-spin-on-big-time-jump branch from b02545d to 143ad93 Compare December 4, 2025 03:01
@AppVeyorBot

Copy link
Copy Markdown

Build pvDataCPP 1.0.63 failed (commit f797fe82d8 by @kiwichris)

@mdavidsaver mdavidsaver force-pushed the kiwichris/fix-time-spin-on-big-time-jump branch from 143ad93 to ba02097 Compare December 4, 2025 23:21
@AppVeyorBot

Copy link
Copy Markdown

Build pvDataCPP 1.0.67 failed (commit 8e926f8386 by @mdavidsaver)

@AppVeyorBot

Copy link
Copy Markdown

Build pvDataCPP 1.0.68 failed (commit e3a9ded505 by @mdavidsaver)

@AppVeyorBot

Copy link
Copy Markdown

Build pvDataCPP 1.0.67 failed (commit e3a9ded505 by @mdavidsaver)

@mdavidsaver

Copy link
Copy Markdown
Member

I think this is enough of an improvement for now.

@mdavidsaver mdavidsaver merged commit 9ec9a52 into epics-base:master Dec 6, 2025
25 of 26 checks passed
@kiwichris

Copy link
Copy Markdown
Contributor Author

@kiwichris I have rebased this PR, and pushed an additional commit. Please have a look and see if you agree with this addition.

Sorry about the delay responding, I was away last week. Thanks for review and changes. They looks good and I have tested it on my setup and I do not see the start up lock up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants