ros2 / rclpy

rclpy (ROS Client Library for Python)
Apache License 2.0
289 stars 224 forks source link

Add TimerInfo to timer callback. #1292

Closed fujitatomoya closed 2 months ago

fujitatomoya commented 3 months ago

address https://github.com/ros2/rclpy/issues/1126 and https://github.com/ros2/rclpy/issues/1265

counterparf of rclcpp implementation https://github.com/ros2/rclcpp/pull/2343

fujitatomoya commented 3 months ago

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

CC: @jmachowinski @tonynajjar

fujitatomoya commented 3 months ago

CI:

fujitatomoya commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

CI:

Barry-Xu-2018 commented 3 months ago

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 commented 3 months ago

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

fujitatomoya commented 2 months ago

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

fujitatomoya commented 2 months ago

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:

fujitatomoya commented 2 months ago

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