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

Update rmw_iceoryx for galactic #54

Merged
merged 9 commits into from
Aug 6, 2021
Merged

Update rmw_iceoryx for galactic #54

merged 9 commits into from
Aug 6, 2021

Conversation

mossmaurice
Copy link
Collaborator

@mossmaurice mossmaurice commented Jul 27, 2021 •
edited
Loading

Done

  • Remove iceoryx repos file as iceoryx is now available in upstream ROS 2
  • Adhere to new galactic APIs
  • Refactor type support calls and remove code duplication

What's working

  • Talker/Listener demo
  • Basic ros2 command line support

What's not working/ known limitations

  • RMW_IMPLEMENTATION=rmw_iceoryx_cpp ros2 topic echo /chatter

Closes #48

@mossmaurice
ros2#48 Remove additional repos file as iceoryx is now available in u…
35682ee 
…pstream ROS 2

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
ros2#48 Adhere rmw_iceoryx to galactic APIs
a4f9429 
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice mossmaurice self-assigned this Jul 27, 2021
@mossmaurice mossmaurice linked an issue Jul 27, 2021 that may be closed by this pull request
Update rmw_iceoryx to support ROS 2 Galactic #48
Closed
@mossmaurice mossmaurice changed the title Update rmw for galactic Update rmw_iceoryx for galactic Jul 27, 2021
@mossmaurice mossmaurice mentioned this pull request Jul 27, 2021
Open
@mossmaurice
Copy link
Collaborator Author

@ZhenshengLee Could you have a look at this pull request?

ZhenshengLee
ZhenshengLee approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@ZhenshengLee ZhenshengLee left a comment

Choose a reason for hiding this comment

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

Good to me.

@mossmaurice
ros2#48 Remove iceoryx packages from CI build and let colcon find the…
7048292 
… dependecy

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
dkroenke
dkroenke reviewed Jul 27, 2021
View reviewed changes
.github/workflows/main.yml Outdated
target-ros2-distro: foxy
vcs-repo-file-url: |
./.github/workflows/additional_repos.repos
package-name: iceoryx_utils iceoryx_posh rmw_iceoryx_cpp iceoryx_ros2_bridge
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
package-name: iceoryx_utils iceoryx_posh rmw_iceoryx_cpp iceoryx_ros2_bridge
package-name: rmw_iceoryx_cpp iceoryx_ros2_bridge

@christophebedard
Copy link
Member

christophebedard commented Jul 28, 2021 •
edited
Loading

What's not working/ known limitations

>>> [rcutils|error_handling.c:108] rcutils_set_error_state()
This error state is being overwritten:

  'Handle's typesupport identifier (rosidl_typesupport_c) is not supported by this library, at /home/simon.hoinkis/ros2_galactic_ade/src/ros2/rosidl_typesupport/rosidl_typesupport_c/src/type_support_dispatch.hpp:113'

with this new error message:

  'Handle's typesupport identifier (rosidl_typesupport_cpp) is not supported by this library, at /home/simon.hoinkis/ros2_galactic_ade/src/ros2/rosidl_typesupport/rosidl_typesupport_cpp/src/type_support_dispatch.hpp:111'

rcutils_reset_error() should be called after error handling to avoid this.

I just recently found the "solution" for this case of an error message being overwritten, so I thought I'd mention it. Currently, if get_message_typesupport_handle() returns a nullptr, it sets an error message, so you're expected to call rcutils_reset_error(). Example: https://github.com/ros2/rmw_connextdds/pull/22/files#diff-8310735a1b9a2eb25a4d85563c1a1862ca08f0243488ac9867c76a7ae7d9ed01

It looks like this behaviour may change at some point though: ros2/rosidl_typesupport#102

@mossmaurice
ros2#48 Wrap get_message_typesupport_handle(), remove duplicate error…
b73c4b8 
… handling and reset error

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
ros2#48 Add empty implementations for new rmw galactic API calls
a52c36b 
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
ros2#48 Update copyright info in headers
70a4db5 
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
ros2#48 Try to fix CI build
d71e511 
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
ros2#48 Remove empty line in license header
c885817 
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
ros2#48 Add missing include for std::pair
72c572a 
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
Copy link
Collaborator Author

@jeising Would you mind doing a review of the changes? Thanks in advance!

@mossmaurice mossmaurice mentioned this pull request Jul 29, 2021
Closed
ZhenshengLee
ZhenshengLee approved these changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@ZhenshengLee ZhenshengLee left a comment

Choose a reason for hiding this comment

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

much of work about checking typesupport, nice!

@mossmaurice mossmaurice mentioned this pull request Jul 30, 2021
Merged
@mossmaurice mossmaurice added the enhancement New feature or request label Jul 30, 2021
jeising
jeising approved these changes Aug 4, 2021
View reviewed changes
.github/workflows/main.yml Show resolved Hide resolved
iceoryx_ros2_bridge/src/generic_subscription.cpp Show resolved Hide resolved
@mossmaurice mossmaurice merged commit 4bfa27f into ros2:galactic Aug 6, 2021
@mossmaurice
Copy link
Collaborator Author

@Karsten1987 Could you please set the default branch for this repository to the branch galactic? Cheers and have a nice weekend!

@mossmaurice mossmaurice deleted the update-rmw-for-galactic branch August 6, 2021 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkroenke dkroenke dkroenke left review comments

@jeising jeising jeising approved these changes

@budrus budrus budrus approved these changes

@ZhenshengLee ZhenshengLee ZhenshengLee approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

Assignees

@mossmaurice mossmaurice

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update rmw_iceoryx to support ROS 2 Galactic
7 participants
@mossmaurice @christophebedard @Karsten1987 @ZhenshengLee @jeising @dkroenke @budrus