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 Foxy support #60

Open
wants to merge 28 commits into
base: foxy
Choose a base branch
from
Open

Add Foxy support #60

wants to merge 28 commits into from

Conversation

aprotyas
Copy link
Member

@aprotyas aprotyas commented Oct 28, 2021

PR description

This PR attempts to make domain_bridge a Foxy-compatible package - based off of the galactic branch.

The following changes have been introduced:

  • Re-add the GenericPublisher/GenericSubscription classes to this package's source tree. 0c05dab
  • Cherry-pick some of the compress/decompress mode logic before Add compressing and decompressing modes #24 was rebased on Refactor to use generic pub/sub from rclcpp #30. 0c05dab
  • Re-add rosidl_target_interfaces in the CMakeLists.txt file. 0c05dab
  • Update the rmw_connextdds test dependency to read rmw_connext_cpp; this package is named differently in Foxy. 0c05dab
  • Update CI to target Foxy. cce4273
  • Use explicit rclcpp::Duration(int64_t ns) constructor over rclcpp::Duration::from_nanoseconds. 9f3b66a
  • Obtain domain ID from rcl_node_options_t. 925a5e1
  • Use rmw types for QoS options class. 699ab3a
  • Define a function to convert from rmw_time_t to rclcpp::Duration. 1562d57
  • Remove unused "unreachable logic" code. cfb36cf
  • Update "Galactic" references to read "Foxy" as appropriate. e614643
  • Replace set_domain_id functionality by doing the same through the rcl layer. 3272a48, 549654a, eb58092

TODOs

I'll just list the things that are missing in Foxy for now:

Closes #59.

Signed-off-by: Abrar Rahman Protyasha [email protected]

@aprotyas
Copy link
Member Author

aprotyas commented Oct 28, 2021

I've targeted this PR to the galactic branch, but will change that ASAP if the maintainers could push a foxy branch off of the current repository head.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Going from `SerializedPublisher` to generic

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Migrate from rclcpp generic pub/sub

With this patch, the package builds successfully in rolling, but there
are failing tests to address, and all of this is before having built in
foxy at all (and before adding back some of the `create_subscription`
code from PR 24).

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Add missing compress/decompress logic

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Update generic pub sub source

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Publish the message rather than its shared ptr

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Rename connext foxy package

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Address uncrustify error

Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Migrate back to rosidl_target_interfaces

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas aprotyas changed the base branch from main to galactic October 28, 2021 09:54
@jacobperron jacobperron changed the base branch from galactic to foxy October 28, 2021 18:04
@jacobperron
Copy link
Member

jacobperron commented Oct 28, 2021

@aprotyas Thanks for working on this :) I've retargeted the PR to foxy (which is based on galactic).

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member

jacobperron commented Oct 28, 2021

I've also pushed a commit to your branch to make CI target Foxy (cce4273).

This constructor has since been deprecated in favor of
`rclcpp::Duration::from_nanoseconds`, but that also means it won't be
backported to Foxy.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Copy link
Member Author

aprotyas commented Oct 28, 2021

@jacobperron thanks for retargeting to foxy.

Question: from the TODOs in the PR description, should the preference be to backport the PRs which introduced said rclcpp functionality, or to opt for the alternative methods I listed (usually just falling back to the rmw/rcl headers)? The latter option is nontrivial for the rclcpp::*Policy enum classes and almost impossible for rclcpp::InitOptions::set_domain_id, but I've already done that for rclcpp::Context::get_domain_id in 925a5e1.


EDIT (10/30)

Alright, so domain_bridge_lib and its tests successfully compile now, but there are a bunch of test failues. It looks like they're almost all related to the fact that the domain ID is not set correctly.
There really does seem to be no way to set the domain ID. Thoughts?

This is because the `rclcpp::Context::get_domain_id` functionality is
missing in Foxy.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
This commit migrates away from the `rclcpp::*Policy` getters and setters
used throughout this package, instead just using the equivalent types
from `rmw/types.h`.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
This commit introduces a utility header that defines a conversion
function from `rmw_time_t` to `rclcpp::Duration`.

This conversion function is necessary because `rmw_time_t` holds time
values (both sec/nsec) as `uint64_t` fields, but `rclcpp::Duration`
has a constructor that accepts sec as a `int32_t` value, raising the
concern of a narrowing conversion.

Similar in spirit to a function with a similar signature introduced in
ros2/rclcpp#1467.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Correct "Galactic" references to read "Foxy" where appropriate.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
This will allow the test code to also access said utility functions,
with the primary purpose being to alleviate the lack of a
`set_domain_id` functionality in `rclcpp` in Foxy.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Copy link
Member Author

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

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

Just pointing out the fact that I added some code for debugging purposes. Will remove in a later commit.

test/domain_bridge/test_qos_matching.cpp Outdated Show resolved Hide resolved
test/domain_bridge/test_qos_matching.cpp Outdated Show resolved Hide resolved
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@antonyramsy
Copy link

antonyramsy commented May 28, 2022

Hi. I just tried to test this branch with with ROS foxy, but i encountered an error when a node begins to publish some data. I used the example_bridge_config.yaml file as mentioned in the readme file and published data in the chatter topic. the error is:

terminate called after throwing an instance of 'std::runtime_error'
  what():  unexpected callback being called

this is my example_bridge_config.yaml file:

name: my_bridge
from_domain: 0
to_domain: 3
topics:
  chatter:
    type: example_interfaces/msg/String

am i missing something alongway doing it or it's a known issue?

@aprotyas
Copy link
Member Author

aprotyas commented May 28, 2022

@antonyramsy this branch was still WIP and I didn't have enough bandwidth to get back to it for a while now.

I remember seeing these errors when I was testing this branch too, and the problem was that in Foxy, rcl offers no such API to set domain IDs -- check #60 (comment) for what I'd already (unsuccessfully) tried. I just never had time to fully verify that my approach in efa97ae works.

I might have some time to revive this PR soon-ish, but if you need this branch to work soon I think the way to go would be to run the tests in this package and debug the errors from there. As it is, I think your configuration is correct but the branch is not ready to land.

@antonyramsy
Copy link

@aprotyas Thanks a lot for the response. I will consider using this package along with other approaches i can take, and will inform if any progress in the development has taken place.

Reference: ros2/rclcpp#910 (comment)

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Copy link
Member Author

aprotyas commented Jun 1, 2022

@jacobperron as of ff41c31, the end-to-end tests pass in this branch. I'm still struggling against the default lifespan and deadline values in QOS profiles.

From ros2/rmw#301 (review), it looks like in Foxy, the default zero values for the lifespan and deadline means each RMW uses its own default value, which is difficult to test against compared to the "max" duration enforced post-Foxy (i.e. rclcpp::Duration(std::numeric_limits<int64_t>::max())). Is there a way around this?


@antonyramsy, this branch may be good to go now. If possible, can you test this branch out?

@aprotyas aprotyas marked this pull request as ready for review June 1, 2022 06:35
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Improves readability of code for rclcpp types, in my opinion.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas

This comment was marked as outdated.

@antonyramsy
Copy link

antonyramsy commented Jun 6, 2022

@aprotyas Hi, sorry for delay. I've tested the updated code both on foxy and humble. in normal condition(sending some data in one domain and receiving it in another one) both of them are ok. but i realized that bidirectional bridging in ROS foxy is not working as i expect (maybe i'm doing it in the wrong way) but in humble it works fine(by using the same YAML config file in different versions of ROS). here is the config i'm using to test bidirectional bridge:

name: my_bridge
from_domain: 2
to_domain: 3
topics:
  chatter:
    type: example_interfaces/msg/String
    remap: chitter
    bidirectional: True

with this config i was able to publish some date on topic chatter from domain 2 and receive it in domain 3 under chitter topic. and vice versa(publishing in domain 3 in topic chatter and receiving it in domain 2 in topic chitter ) in ROS humble.

but doing the same in ROS foxy, although you are able to send data from domain 2 to 3, but you wont be able to see published data from domain 3 in topic chatter in domain 2 in topic chitter (the topic chitter does not show up in ros2 topic list command result.)

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.

Foxy Support
3 participants