-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: foxy
Are you sure you want to change the base?
Add Foxy support #60
Conversation
I've targeted this PR to the |
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]>
6925c80
to
0c05dab
Compare
@aprotyas Thanks for working on this :) I've retargeted the PR to |
Signed-off-by: Jacob Perron <[email protected]>
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]>
@jacobperron thanks for retargeting to Question: from the TODOs in the PR description, should the preference be to backport the PRs which introduced said EDIT (10/30) Alright, so |
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]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
There was a problem hiding this 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.
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
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
this is my
am i missing something alongway doing it or it's a known issue? |
@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, 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. |
@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]>
@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. @antonyramsy, this branch may be good to go now. If possible, can you test this branch out? |
Signed-off-by: Abrar Rahman Protyasha <[email protected]>
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]>
This comment was marked as outdated.
This comment was marked as outdated.
@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:
with this config i was able to publish some date on topic 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 |
PR description
This PR attempts to make
domain_bridge
a Foxy-compatible package - based off of thegalactic
branch.The following changes have been introduced:
GenericPublisher
/GenericSubscription
classes to this package's source tree. 0c05dabrosidl_target_interfaces
in the CMakeLists.txt file. 0c05dabrmw_connextdds
test dependency to readrmw_connext_cpp
; this package is named differently in Foxy. 0c05dabexplicit rclcpp::Duration(int64_t ns)
constructor overrclcpp::Duration::from_nanoseconds
. 9f3b66arcl_node_options_t
. 925a5e1rmw
types for QoS options class. 699ab3armw_time_t
torclcpp::Duration
. 1562d57set_domain_id
functionality by doing the same through thercl
layer. 3272a48, 549654a, eb58092TODOs
I'll just list the things that are missing in Foxy for now:
Either backport the node option generating hooks added toMy bad, I started from therclcpp_components::ComponentManager
in Add hook to generate node options in ComponentManager rclcpp#1702 to Foxy - which is a difficult ask because this is most likely an ABI breaking change, or "revert" Domain bridge container #32, but that may have performance implications because (quoting @rebecca-butler)"This would allow the bridge and all other nodes to be run in the same process, ideally reducing latency."
main
branch instead ofgalactic
.rclcpp::*Policy
enum classes and the respective getters.These can be accessed through
rmw/types.h
but I suspect the PR these were introduced in (Add getters to rclcpp::qos and rclcpp::*Policy enum classes rclcpp#1467) can be backported.Technically this functionality is exposed in
rcl_node_options_t
, so it's just a deficiency in therclcpp
API (in Foxy). In caserclcpp::InitOptions::set_domain_id
cannot be backported to Foxy, thedomain_id
can be obtained throughget_rcl_node_options
(minus some mutex concerns..). Not sure how to set thedomain_id
though. Refer to Domain ID not changable on rclcpp::NodeOptions rclcpp#910 and the PRs that fell out of it.rclcpp::Context::get_domain_id
.rclcpp::InitOptions::set_domain_id
.get_rcl_node_options
since that is aconst
function. Not sure how to compensate for this API without backporting at least Add setter and getter for domain_id in rcl_init_options_t rcl#678, and ideally also Ability to configure domain_id via InitOptions. rclcpp#1165 - although this one is most likely not an ABI-compatible patch.const
object throughconst_cast
is undefined behavior.rclcpp::Duration::from_nanoseconds
.Use the
Duration(rcl_duration_value_t nanoseconds)
constructor instead.Closes #59.
Signed-off-by: Abrar Rahman Protyasha [email protected]