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.

Sign up for GitHub

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

Jump to bottom

RST-6618 fixes for action bridge #2

Merged
merged 4 commits into from
Feb 7, 2024
Merged

RST-6618 fixes for action bridge #2

merged 4 commits into from
Feb 7, 2024

Conversation

abencz
Copy link

@abencz abencz commented Jul 7, 2023

No description provided.

abencz
abencz commented Jul 7, 2023
View reviewed changes
README.md Show resolved Hide resolved
abencz
abencz commented Jul 7, 2023
View reviewed changes
include/ros1_bridge/action_factory.hpp
@@ -146,7 +146,7 @@ class ActionFactory_1_2 : public ActionFactoryInterface
std::shared_future<ROS2ClientGoalHandle> gh2_future;
auto send_goal_ops = ROS2SendGoalOptions();
send_goal_ops.goal_response_callback =
[this, &gh2_future](std::shared_future<ROS2GoalHandle> gh2) mutable {
[this, &gh2_future](ROS2GoalHandle gh2) mutable {
Copy link
Author

Choose a reason for hiding this comment

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

API change from Foxy to Rolling.

Copy link

Choose a reason for hiding this comment

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

I assume this also works for iron?

Copy link

Choose a reason for hiding this comment

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

Why make these changes on our fork of ros1_bridge instead of either

  1. Making a change upstream and back porting to iron?
  2. or switching over to rolling if it's fixed there
  3. or just cherry-picking commits from rolling onto our fork?

Copy link
Author

@abencz abencz Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The action bridge stuff is not merged into upstream ros1_bridge. The PR for it is probably abandoned. Notice that this merge is into an action_bridge branch and not locus-devel. I'll see what Paul wants to do with the action_bridge branch once he's back.

@abencz abencz force-pushed the RST-6618-action-bridge-fixes branch from 7265e74 to 83b744d Compare July 13, 2023 14:49
@abencz
Copy link
Author

abencz commented Jul 13, 2023

I suspect there will be more changes, but might as well get these reviewed for now.

@abencz abencz marked this pull request as ready for review July 13, 2023 14:53
AadityaRavindran
AadityaRavindran reviewed Jul 13, 2023
View reviewed changes
include/ros1_bridge/action_factory.hpp
@@ -146,7 +146,7 @@ class ActionFactory_1_2 : public ActionFactoryInterface
std::shared_future<ROS2ClientGoalHandle> gh2_future;
auto send_goal_ops = ROS2SendGoalOptions();
send_goal_ops.goal_response_callback =
[this, &gh2_future](std::shared_future<ROS2GoalHandle> gh2) mutable {
[this, &gh2_future](ROS2GoalHandle gh2) mutable {
Copy link

Choose a reason for hiding this comment

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

Why make these changes on our fork of ros1_bridge instead of either

  1. Making a change upstream and back porting to iron?
  2. or switching over to rolling if it's fixed there
  3. or just cherry-picking commits from rolling onto our fork?

resource/interface_factories.cpp.em Show resolved Hide resolved
ros1_bridge/__init__.py Show resolved Hide resolved
@abencz abencz force-pushed the RST-6618-action-bridge-fixes branch from 83b744d to f7b36e4 Compare January 16, 2024 15:29
AadityaRavindran
AadityaRavindran reviewed Jan 16, 2024
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
jwallace42
jwallace42 reviewed Jan 16, 2024
View reviewed changes
Copy link

@jwallace42 jwallace42 left a comment

Choose a reason for hiding this comment

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

I think there are still some open questions here. Some of the questions I see are from 6 months ago.

@abencz abencz changed the base branch from action_bridge to action-bridge-rebased January 19, 2024 14:08
@abencz
Copy link
Author

abencz commented Jan 19, 2024

I think there are still some open questions here. Some of the questions I see are from 6 months ago.

I think most of that is about upstreaming these changes?

There's also a big element missing from the action_bridge feature fork: support for running from parameter_bridge. Currently there's no code in parameter bridge for creating action bridge - but for getting a nav2 + executive prototype running I don't think it's necessary.

jwallace42
jwallace42 approved these changes Jan 19, 2024
View reviewed changes
Copy link

@jwallace42 jwallace42 left a comment

Choose a reason for hiding this comment

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

LGTM.

Do you have any demo's of exec running with nav2?

@abencz
Create generated folder explicitly
8a90a8e 
Required because output files are now copied in using
`update_output_file` instead of being created by `expand_template`.
@abencz abencz force-pushed the RST-6618-action-bridge-fixes branch from a7823f3 to 8a90a8e Compare January 19, 2024 18:42
@abencz abencz merged commit 27b0dc6 into action-bridge-rebased Feb 7, 2024
0 of 3 checks passed
@abencz abencz deleted the RST-6618-action-bridge-fixes branch February 7, 2024 15:41
abencz added a commit that referenced this pull request Feb 7, 2024
@abencz
RST-6618 fixes for action bridge (#2)
3a35ce4 
Fixed action_bridge changes to work with newer ros2 releases and added support for skipping previously built generated factories to speed up recompiling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AadityaRavindran AadityaRavindran AadityaRavindran approved these changes

@jwallace42 jwallace42 jwallace42 approved these changes

@aurone aurone Awaiting requested review from aurone

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abencz @AadityaRavindran @jwallace42