Add support for timers on reset callback #1979

mauropasse · 2022-08-02T14:18:41Z

Based on ros2/rcl#995, this PR adds support for the "on reset" callback for timers.

The issue which originated this need, is to wake up an events based executor when a timer is reset (which could also be caused by a time jump).

@alsora

alsora · 2022-08-02T14:48:45Z

CI including rcl PR

Linux
Linux-aarch64
Windows

fujitatomoya

overall lgtm, a few comments.

rclcpp/src/rclcpp/timer.cpp

rclcpp/include/rclcpp/timer.hpp

rclcpp/src/rclcpp/timer.cpp

Flova · 2022-10-17T22:46:02Z

Any updates here?

alsora · 2022-10-27T11:46:27Z

Linux
Linux-aarch64
Windows

Flova · 2022-11-24T18:42:28Z

The CI fails because ros2/rcl#995 is needed.

alsora · 2022-11-24T19:45:35Z

The main issue here is the Windows CI that is failing.
However, I see that the same failure is also present in a completely unrelated PR #1982

Flova · 2022-11-24T19:56:49Z

When I looked at the Win CI output it seemed that something related to the parameters interface in tf2_ros was failing. This seems to be kind of unrelated. Can we maybe try to tun the CI against some documentation PR to see if this is an issue with the current master of tf2?

ivanpauno · 2022-11-28T17:08:46Z

The main issue here is the Windows CI that is failing. However, I see that the same failure is also present in a completely unrelated PR #1982

I think that was fixed in #2018, the error is not happening in nightlies anymore.
It would be nice to rebase and rerun to confirm.

fujitatomoya

test would be ideal, lgtm.

fujitatomoya · 2022-11-28T18:18:12Z

CI:

Linux
Linux-aarch64
Windows

Flova · 2022-11-28T21:27:03Z

There seems to be another Windows build issue:

19:58:26 Running handlers complete
19:58:26 [2022-11-28T18:58:25+00:00] ERROR: Exception handlers complete
19:58:26 Cinc Client failed. 65 resources updated in 12 minutes 01 seconds
19:58:26 [2022-11-28T18:58:26+00:00] FATAL: LoadError: cannot load such file -- inspec/utils/deprecation/global_method
19:58:26 The command 'cmd /S /C c:\cinc-project\cinc\bin\cinc-solo.bat -c C:\TEMP\solo.rb -Eros2ci -j C:\TEMP\install_ros2_%ROS_DISTRO%.json' returned a non-zero code: 1

Flova · 2022-11-28T21:27:50Z

Also seems unrelated imo...

ivanpauno · 2022-11-29T13:21:35Z

Seems like a random CI network issue, running it again:

Windows

Flova · 2022-11-29T16:41:52Z

Seems like a random CI network issue, running it again:

The Windows CI still seems to have Network issues:

Relevant Jenkins Traceback

17:31:39 FATAL: command execution failed
17:33:44 java.nio.channels.ClosedChannelException
17:33:44 	at org.jenkinsci.remoting.protocol.NetworkLayer.onRecvClosed(NetworkLayer.java:154)
17:33:44 	at org.jenkinsci.remoting.protocol.impl.NIONetworkLayer.ready(NIONetworkLayer.java:179)
17:33:44 	at org.jenkinsci.remoting.protocol.IOHub$OnReady.run(IOHub.java:790)
17:33:44 	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
17:33:44 	at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
17:33:44 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
17:33:44 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
17:33:44 	at java.lang.Thread.run(Thread.java:748)
17:33:44 Caused: java.io.IOException: Backing channel 'JNLP4-connect connection from ip-10-0-1-137.ec2.internal/10.0.1.137:49694' is disconnected.
17:33:44 	at hudson.remoting.RemoteInvocationHandler.channelOrFail(RemoteInvocationHandler.java:216)
17:33:44 	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:286)
17:33:44 	at com.sun.proxy.$Proxy76.isAlive(Unknown Source)
17:33:44 	at hudson.Launcher$RemoteLauncher$ProcImpl.isAlive(Launcher.java:1213)
17:33:44 	at hudson.Launcher$RemoteLauncher$ProcImpl.join(Launcher.java:1205)
17:33:44 	at hudson.tasks.CommandInterpreter.join(CommandInterpreter.java:194)
17:33:44 	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:144)
17:33:44 	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:91)
17:33:44 	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
17:33:44 	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:806)
17:33:44 	at hudson.model.Build$BuildExecution.build(Build.java:198)
17:33:44 	at hudson.model.Build$BuildExecution.doRun(Build.java:163)
17:33:44 	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:514)
17:33:44 	at hudson.model.Run.execute(Run.java:1888)
17:33:44 	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
17:33:44 	at hudson.model.ResourceController.execute(ResourceController.java:99)
17:33:44 	at hudson.model.Executor.run(Executor.java:432)
17:33:44 FATAL: Unable to delete script file C:\Windows\TEMP\jenkins469429339840863473.bat
17:33:44 java.nio.channels.ClosedChannelException
17:33:44 	at org.jenkinsci.remoting.protocol.NetworkLayer.onRecvClosed(NetworkLayer.java:154)
17:33:44 	at org.jenkinsci.remoting.protocol.impl.NIONetworkLayer.ready(NIONetworkLayer.java:179)
17:33:44 	at org.jenkinsci.remoting.protocol.IOHub$OnReady.run(IOHub.java:790)
17:33:44 	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
17:33:44 	at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
17:33:44 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
17:33:44 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
17:33:44 	at java.lang.Thread.run(Thread.java:748)
17:33:44 Caused: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@30159803:JNLP4-connect connection from ip-10-0-1-137.ec2.internal/10.0.1.137:49694": Remote call on JNLP4-connect connection from ip-10-0-1-137.ec2.internal/10.0.1.137:49694 failed. The channel is closing down or has closed down
17:33:44 	at hudson.remoting.Channel.call(Channel.java:994)
17:33:44 	at hudson.FilePath.act(FilePath.java:1165)
17:33:44 	at hudson.FilePath.act(FilePath.java:1154)
17:33:44 	at hudson.FilePath.delete(FilePath.java:1681)
17:33:44 	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:162)
17:33:44 	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:91)
17:33:44 	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
17:33:44 	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:806)
17:33:44 	at hudson.model.Build$BuildExecution.build(Build.java:198)
17:33:44 	at hudson.model.Build$BuildExecution.doRun(Build.java:163)
17:33:44 	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:514)
17:33:44 	at hudson.model.Run.execute(Run.java:1888)
17:33:44 	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
17:33:44 	at hudson.model.ResourceController.execute(ResourceController.java:99)
17:33:44 	at hudson.model.Executor.run(Executor.java:432)
17:33:44 Build step 'Execute Windows batch command' marked build as failure
17:33:44 ERROR: Step ‘Record compiler warnings and static analysis results’ aborted due to exception: 
17:33:44 java.io.IOException: No workspace found for ci_windows #18344
17:33:44 	at io.jenkins.plugins.analysis.core.steps.IssuesRecorder.perform(IssuesRecorder.java:577)
17:33:44 	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
17:33:44 	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:806)
17:33:44 	at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:755)
17:33:44 	at hudson.model.Build$BuildExecution.post2(Build.java:178)
17:33:44 	at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:699)
17:33:44 	at hudson.model.Run.execute(Run.java:1913)
17:33:44 	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
17:33:44 	at hudson.model.ResourceController.execute(ResourceController.java:99)
17:33:44 	at hudson.model.Executor.run(Executor.java:432)
17:33:44 ERROR: Step ‘Publish Cobertura Coverage Report’ failed: no workspace for ci_windows #18344
17:33:44 ERROR: Step ‘Publish xUnit test result report’ failed: no workspace for ci_windows #18344
17:33:44 Finished: FAILURE

ivanpauno · 2022-11-29T16:46:13Z

The Windows CI still seems to have Network issues:

Yes, I will run it again in some hours, after checking with the infra team if there are any windows CI issues.

ivanpauno · 2022-11-30T13:17:56Z

Windows

ivanpauno · 2022-11-30T14:27:16Z

it seems we are still having problems with the windows agents ...

nuclearsandwich · 2022-11-30T15:36:05Z

Thanks for everyone's patience on these infrastructure issues. We use low-bid spot instances in order to keep costs down but these cause us trouble every year around the shopping season. The ROS infra team is currently working to add some on-demand capacity to allow jobs to continue.

In order to run that transition I am unfortunately going to abort but I will requeue it again and link it here once the transition is complete.

nuclearsandwich · 2022-11-30T15:41:27Z

Alright. Because is running on a relatively fresh agent, there may still be issues related to other reliability issues caused by the holiday activity but this agent should at least stick around.

Flova · 2022-11-30T17:25:27Z

The run still fails :/, but it at least failed relatively early with an infrastructure related error.

clalancette · 2022-11-30T18:00:54Z

is the same job and running now.

alsora · 2022-12-06T16:52:32Z

CI

Linux
Linux-aarch64
Windows

alsora · 2022-12-07T11:44:06Z

CI after fixing rcl PR

Linux
Linux-aarch64
Windows

alsora · 2022-12-07T19:12:50Z

CI

Linux
Linux-aarch64
Windows

Flova · 2022-12-08T09:20:51Z

Finally 🎉

alsora · 2022-12-08T17:12:45Z

@fujitatomoya @ivanpauno @hidmic it looks like all comments have been addressed; can you have a final look before we merge this PR and its related rcl PR?

fujitatomoya · 2022-12-08T18:27:55Z

I am good to go with this, but requesting another review from maintainers. CC: @ivanpauno @hidmic @wjwwood

be advised that this must be merged with ros2/rcl#995

Signed-off-by: Mauro Passerino <[email protected]>

mauropasse · 2022-12-16T14:55:36Z

Rebased after #2059 got merged

alsora · 2023-01-22T16:29:11Z

Ping; it looks like the rcl PR (ros2/rcl#995) has been approved, can we get an additional review here?

this protects against the callback being modified while we are resetting the timer Signed-off-by: Alberto Soragna <[email protected]>

alsora · 2023-02-10T11:29:02Z

Hi, any update on this?

Signed-off-by: Mauro Passerino <[email protected]> Co-authored-by: Alberto Soragna <[email protected]>

fujitatomoya reviewed Aug 17, 2022

View reviewed changes

rclcpp/src/rclcpp/timer.cpp Show resolved Hide resolved

rclcpp/include/rclcpp/timer.hpp Show resolved Hide resolved

rclcpp/src/rclcpp/timer.cpp Show resolved Hide resolved

mauropasse force-pushed the mauro/add-timer-on-reset-callback branch from 052aa82 to 0673586 Compare October 28, 2022 10:56

clalancette added the more-information-needed Further information is required label Nov 17, 2022

Flova mentioned this pull request Nov 24, 2022

Add timer on reset callback ros2/rcl#995

Merged

alsora mentioned this pull request Nov 24, 2022

Add clock type to node_options #1982

Merged

mauropasse force-pushed the mauro/add-timer-on-reset-callback branch from 0673586 to d5f1949 Compare November 28, 2022 17:51

mauropasse requested review from ivanpauno, hidmic and wjwwood as code owners November 28, 2022 17:51

fujitatomoya approved these changes Nov 28, 2022

View reviewed changes

Mauro Passerino added 2 commits December 16, 2022 11:16

Add support for timers on reset callback

42349f7

Signed-off-by: Mauro Passerino <[email protected]>

Update cpp_callback_trampoline on timer.cpp

43dee48

Signed-off-by: Mauro Passerino <[email protected]>

mauropasse force-pushed the mauro/add-timer-on-reset-callback branch from d5f1949 to 43dee48 Compare December 16, 2022 14:52

add mutex in timer reset

a759f38

this protects against the callback being modified while we are resetting the timer Signed-off-by: Alberto Soragna <[email protected]>

fujitatomoya merged commit ce5a261 into ros2:rolling Feb 10, 2023

Flova mentioned this pull request Feb 10, 2023

Upstream changes related to the humble-future branch irobot-ros/events-executor#15

Open

mauropasse deleted the mauro/add-timer-on-reset-callback branch February 13, 2023 11:05

tonynajjar pushed a commit to pixel-robotics/rclcpp that referenced this pull request Apr 3, 2023

Add support for timers on reset callback (ros2#1979)

85b2dbb

Signed-off-by: Mauro Passerino <[email protected]> Co-authored-by: Alberto Soragna <[email protected]>

tonynajjar pushed a commit to pixel-robotics/rclcpp that referenced this pull request Apr 3, 2023

Add support for timers on reset callback (ros2#1979)

ad4a063

Signed-off-by: Mauro Passerino <[email protected]> Co-authored-by: Alberto Soragna <[email protected]>

tonynajjar mentioned this pull request Apr 3, 2023

create_waitable_callback causing segmentation fault irobot-ros/events-executor#18

Open

alsora added a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023

Add support for timers on reset callback (ros2#1979)

735a784

Signed-off-by: Mauro Passerino <[email protected]> Co-authored-by: Alberto Soragna <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for timers on reset callback #1979

Add support for timers on reset callback #1979

mauropasse commented Aug 2, 2022

alsora commented Aug 2, 2022

fujitatomoya left a comment

Flova commented Oct 17, 2022

alsora commented Oct 27, 2022 •

edited

Loading

Flova commented Nov 24, 2022

alsora commented Nov 24, 2022

Flova commented Nov 24, 2022

ivanpauno commented Nov 28, 2022

fujitatomoya left a comment

fujitatomoya commented Nov 28, 2022

Flova commented Nov 28, 2022

Flova commented Nov 28, 2022

ivanpauno commented Nov 29, 2022

Flova commented Nov 29, 2022 •

edited

Loading

ivanpauno commented Nov 29, 2022

ivanpauno commented Nov 30, 2022

ivanpauno commented Nov 30, 2022

nuclearsandwich commented Nov 30, 2022

nuclearsandwich commented Nov 30, 2022

Flova commented Nov 30, 2022

clalancette commented Nov 30, 2022

alsora commented Dec 6, 2022

alsora commented Dec 7, 2022

alsora commented Dec 7, 2022

Flova commented Dec 8, 2022

alsora commented Dec 8, 2022

fujitatomoya commented Dec 8, 2022

mauropasse commented Dec 16, 2022

alsora commented Jan 22, 2023

alsora commented Feb 10, 2023

Add support for timers on reset callback #1979

Add support for timers on reset callback #1979

Conversation

mauropasse commented Aug 2, 2022

alsora commented Aug 2, 2022

fujitatomoya left a comment

Choose a reason for hiding this comment

Flova commented Oct 17, 2022

alsora commented Oct 27, 2022 • edited Loading

Flova commented Nov 24, 2022

alsora commented Nov 24, 2022

Flova commented Nov 24, 2022

ivanpauno commented Nov 28, 2022

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Nov 28, 2022

Flova commented Nov 28, 2022

Flova commented Nov 28, 2022

ivanpauno commented Nov 29, 2022

Flova commented Nov 29, 2022 • edited Loading

ivanpauno commented Nov 29, 2022

ivanpauno commented Nov 30, 2022

ivanpauno commented Nov 30, 2022

nuclearsandwich commented Nov 30, 2022

nuclearsandwich commented Nov 30, 2022

Flova commented Nov 30, 2022

clalancette commented Nov 30, 2022

alsora commented Dec 6, 2022

alsora commented Dec 7, 2022

alsora commented Dec 7, 2022

Flova commented Dec 8, 2022

alsora commented Dec 8, 2022

fujitatomoya commented Dec 8, 2022

mauropasse commented Dec 16, 2022

alsora commented Jan 22, 2023

alsora commented Feb 10, 2023

alsora commented Oct 27, 2022 •

edited

Loading

Flova commented Nov 29, 2022 •

edited

Loading