Skip to content
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

Add TimerInfo to timer callback. #1292

Merged

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented Jun 2, 2024

address #1126 and #1265

counterparf of rclcpp implementation ros2/rclcpp#2343

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 2, 2024

@mjcarroll @ahcorde @clalancette can you take a look?

CC: @jmachowinski @tonynajjar

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/support-timerinfo-to-timer-callback branch from 375742d to baad477 Compare June 7, 2024 06:08
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 19, 2024

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

according to https://ci.ros2.org/job/ci_linux/21290/console, test_launch_ros gets stuck because of this fix. i am expecting that something is missing to support partial(...) case. anyway, i will stop all CIs for now.

@Barry-Xu-2018
Copy link
Contributor

according to https://ci.ros2.org/job/ci_linux/21290/console, test_launch_ros gets stuck because of this fix. i am expecting that something is missing to support partial(...) case. anyway, i will stop all CIs for now.

About the stuck, it is related to inspect.signature(tmr.callback).bind(object())
I still can't explain the root cause of being stuck.

Here it's just checking if the callback has one parameter.
Could it be changed to

    def _take_timer(self, tmr):
        try:
            with tmr.handle:
                info = tmr.handle.call_timer_with_info()
                if len(inspect.signature(tmr.callback).parameters) == 1:
                    timer_info = TimerInfo(
                        expected_call_time=info['expected_call_time'],
                        actual_call_time=info['actual_call_time'],
                        clock_type=tmr.clock.clock_type)                        
                    async def _execute():
                        await await_or_execute(tmr.callback, timer_info)
                else:
                    async def _execute():
                        await await_or_execute(tmr.callback)
                return _execute
        except InvalidHandle:
            # Timer is a Destroyable, which means that on __enter__ it can throw an
            # InvalidHandle exception if the entity has already been destroyed.  Handle that here
            # by just returning an empty argument, which means we will skip doing any real work
            # in _execute_timer below
            pass

        return None

@fujitatomoya
Copy link
Collaborator Author

according to https://ci.ros2.org/job/ci_linux/21290/console, test_launch_ros gets stuck because of this fix. i am expecting that something is missing to support partial(...) case. anyway, i will stop all CIs for now.

About the stuck, it is related to inspect.signature(tmr.callback).bind(object()) I still can't explain the root cause of being stuck.

Here it's just checking if the callback has one parameter. Could it be changed to

    def _take_timer(self, tmr):
        try:
            with tmr.handle:
                info = tmr.handle.call_timer_with_info()
                if len(inspect.signature(tmr.callback).parameters) == 1:
                    timer_info = TimerInfo(
                        expected_call_time=info['expected_call_time'],
                        actual_call_time=info['actual_call_time'],
                        clock_type=tmr.clock.clock_type)                        
                    async def _execute():
                        await await_or_execute(tmr.callback, timer_info)
                else:
                    async def _execute():
                        await await_or_execute(tmr.callback)
                return _execute
        except InvalidHandle:
            # Timer is a Destroyable, which means that on __enter__ it can throw an
            # InvalidHandle exception if the entity has already been destroyed.  Handle that here
            # by just returning an empty argument, which means we will skip doing any real work
            # in _execute_timer below
            pass

        return None

@Barry-Xu-2018 thanks for the suggestion.

this does not work either, because callback would have multiple parameters.

e.g. https://github.com/ros2/launch_ros/blob/rolling/test_launch_ros/test/test_launch_ros/actions/test_ros_timer.py#L173

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 21, 2024

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@Barry-Xu-2018
Copy link
Contributor

this does not work either, because callback would have multiple parameters.
e.g. https://github.com/ros2/launch_ros/blob/rolling/test_launch_ros/test/test_launch_ros/actions/test_ros_timer.py#L173

Oh, yeah. Finding parameter names through type is the correct way.

@fujitatomoya
Copy link
Collaborator Author

https://ci.ros2.org/job/ci_linux/21306/#showFailuresLink does not look related to the fix, but just in case i want to retry.

  • Linux Build Status

@fujitatomoya
Copy link
Collaborator Author

@ahcorde @sloretz @mjcarroll @clalancette i could use review for this.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

fujitatomoya and others added 2 commits July 29, 2024 09:12
Signed-off-by: Tomoya.Fujita <tomoya.fujita825@gmail.com>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/support-timerinfo-to-timer-callback branch from f5f7c42 to 7c8e1c2 Compare July 29, 2024 16:12
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
fujitatomoya added a commit to fujitatomoya/ros2_test_prover that referenced this pull request Jul 29, 2024
  ros2/rclpy#1292

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

There is a test failure https://build.ros2.org/job/Rpr__rclpy__ubuntu_noble_amd64/161/testReport/junit/rclpy.rclpy.test/test_timer/test_cancel_reset_0_001_/ is this related with this PR ?

i do not think this is related, all tests are pass in my local environment.

i did add a couple of cosmetic fixes including new test case, and rebasing the current rolling.

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya requested a review from ahcorde July 29, 2024 17:16
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jul 30, 2024

https://ci.ros2.org/job/ci_linux/21574/ is not related rclpy.Timer.

@fujitatomoya fujitatomoya merged commit 85ecfcb into rolling Jul 30, 2024
3 checks passed
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.

5 participants