search

ros2 / ros1_bridge

ROS 2 package that provides bidirectional communication between ROS 1 and ROS 2
Apache License 2.0
425 stars 275 forks source link

Parametrizing service execution timeout #340

Closed nirwester closed 2 years ago

nirwester commented 2 years ago

By convention, service calls in ROS should have a short execution time. However, since the bridge currently doesn't yet have full support for actions, services are sometimes used where a bit of computation time is needed.

Currently the service call timeout on service_bridge_1_to_2 is hard-coded to 5 seconds for all services. This constrain is not transparent to the user, that needs to take a look at the code to understand what's going on.

The PR adds a new parameter, "service_execution_timeout", that can be set by the user to extend (or reduce) the required timeout for service calls. This was added for both the dynamic_bridge and the parameter_bridge.

gbiggs commented 2 years ago

LGTM pending CI.

nirwester commented 2 years ago

Made it Ament-friendly to make CI happy

fujitatomoya commented 2 years ago

@gbiggs @sloretz can you run CI?

gbiggs commented 2 years ago

Gist: https://gist.github.com/gbiggs/d25e3d7ba7370e48c391d1dcf73b8815 BUILD args: --packages-up-to ros1_bridge TEST args: --packages-select ros1_bridge ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9593/

fujitatomoya commented 2 years ago

CI is failing cz of https://ci.ros2.org/job/ci_linux/15874/console

14:22:52 Package 'ros1_bridge' specified with --packages-up-to was not found

i am not sure what went wrong, and cannot reproduce this problem either in my local environment. any idea?

nirwester commented 2 years ago

I'm not familiar with this integration process, but having ros1_bridge in the packages to ignore 23:22:52 Create marker file: src/ros2/ros1_bridge/COLCON_IGNORE Looks fishy... how are these ignore packages chosen?

clalancette commented 2 years ago

Running CI for the ros1_bridge is slightly tricky. We don't support the bridge on anything but Linux, so you don't need to run CI for Windows or macOS.

For Linux, the easiest way to run CI with the ros1_bridge is to run the ci_packaging jobs:

If you put your .repos file into the parameters there, then it should do the correct thing while trying to build the bridge.

fujitatomoya commented 2 years ago

@clalancette thanks!

CI for linux and linux-aarch64:

gbiggs commented 2 years ago

Linux CI looks good, but aarch64 is failing with a strange error in a Python module. @clalancette Is it expected that aarch64 will fail for ros1_bridge?

clalancette commented 2 years ago

Linux CI looks good, but aarch64 is failing with a strange error in a Python module. @clalancette Is it expected that aarch64 will fail for ros1_bridge?

No, it should work. But I think we were having some unrelated issues. Here's another run of CI:

gbiggs commented 2 years ago

The instability looks to be in ros1_bridge. I think that's safe to ignore, so this can be merged?

clalancette commented 2 years ago

The linting failures are almost certainly because this PR would need to be rebased onto the latest. That we can ignore for now. This otherwise looks good, so I'm going to go ahead and merge it. Thanks for the contribution.