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

Parametrizing service execution timeout #340

Merged
merged 2 commits into from
Jan 24, 2022
Merged

Conversation

nirwester
Copy link
Contributor

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.

Signed-off-by: Marco Bassa <marco@robotcloud.eu>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

this sounds reasonable for me, just a couple of minor comments.

src/dynamic_bridge.cpp Outdated Show resolved Hide resolved
@gbiggs
Copy link
Member

gbiggs commented Dec 17, 2021

LGTM pending CI.

Signed-off-by: Marco Bassa <marco@robotcloud.eu>
@nirwester
Copy link
Contributor Author

Made it Ament-friendly to make CI happy

@fujitatomoya
Copy link
Collaborator

@gbiggs @sloretz can you run CI?

@gbiggs
Copy link
Member

gbiggs commented Jan 2, 2022

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/

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Collaborator

@gbiggs
Copy link
Member

gbiggs commented Jan 5, 2022

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
Copy link
Contributor

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:

  • Linux Build Status
  • Linux-aarch64 Build Status

@gbiggs
Copy link
Member

gbiggs commented Jan 24, 2022

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

@clalancette
Copy link
Contributor

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.

@clalancette clalancette merged commit fe94cf5 into ros2:master Jan 24, 2022
tompe17 pushed a commit to tompe17/ros1_bridge that referenced this pull request Mar 25, 2022
* Parametrizing service execution timeout

Signed-off-by: Marco Bassa <marco@robotcloud.eu>
Signed-off-by: Tommy Persson <tommy.persson@liu.se>
tompe17 pushed a commit to tompe17/ros1_bridge that referenced this pull request Mar 25, 2022
* Parametrizing service execution timeout

Signed-off-by: Marco Bassa <marco@robotcloud.eu>
Signed-off-by: Tommy Persson <tommy.persson@liu.se>
tompe17 pushed a commit to tompe17/ros1_bridge that referenced this pull request Mar 25, 2022
* Parametrizing service execution timeout

Signed-off-by: Marco Bassa <marco@robotcloud.eu>
Signed-off-by: Tommy Persson <tommy.persson@liu.se>
lmark1 pushed a commit to larics/ros1_bridge that referenced this pull request Jan 15, 2024
* Parametrizing service execution timeout

Signed-off-by: Marco Bassa <marco@robotcloud.eu>
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