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

Zenoh-pico add ROS2 Unix example using CycloneDDS CDR serializer #8

Merged
merged 7 commits into from
Feb 20, 2023

Conversation

PetervdPerk-NXP
Copy link
Contributor

Adds a UNIX example to communicate with ROS2 using the native ROS2 message definitions.

For serialization we use the CycloneDDS CDR serializer, for now we compile some components separately. But ideally we would have to have do decouple some modules from CycloneDDS i.e. dds_cdrstream.c and turn it into a standalone library that could run also on small RTOS'es (FreeRTOS, Zephyr and more)

To use the UNIX example you've to specify the ROS_DISTRO and it will automatically generated the definitions and serializer to communicate on a ROS2 topic. For more information see https://github.com/NXPHoverGames/zenoh-demos/blob/400df907eaf50416b36fc729e9bd5a0d0c44f428/ROS2/zenoh-pico-cyclonedds-cdr-message-log/README.md

Furthermore this PR has a soft dependency to this eclipse-cyclonedds/cyclonedds#1564 PR to avoiding that we compile the complete DDS stack.

@gabrik
Copy link
Contributor

gabrik commented Feb 13, 2023

Thank you for your contribution.
Before merging this PR we need to wait for eclipse-cyclonedds/cyclonedds#1564 to be merged.

@gabrik gabrik self-assigned this Feb 13, 2023
@cguimaraes
Copy link
Member

@PetervdPerk-NXP I did a PR on top of yours to improve out-of-the-box experience and to fix things here and there in the README.md(s)

The dependency to eclipse-cyclonedds/cyclonedds#1564 does not seem mandatory. Until merged, it will compile the entire CycloneDDS suite, but it does not seem a problem to me.

Thanks for your contribution.

@cguimaraes cguimaraes self-requested a review February 16, 2023 10:06
Copy link
Member

@cguimaraes cguimaraes left a comment

Choose a reason for hiding this comment

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

LGTM.
Just pending a merge on last tweaks:

  • Removed unused variables
  • Apply formatting guidelines.

IMO, it can be merged even before the CycloneDDS PR is accepted. The only impact is building the entire CycloneDDS suite when we just require the IDL part.


int main(int argc, char **argv) {
const char *keyexpr = "rt/zenoh_log_test";
const char *value = "Pub from Pico IDL!";
Copy link
Member

Choose a reason for hiding this comment

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

This variable is not used.
I did a PR to remove unused variable and apply formatting.

@gabrik gabrik removed their assignment Feb 16, 2023
@Mallets
Copy link
Member

Mallets commented Feb 20, 2023

LGTM. Just pending a merge on last tweaks:

  • Removed unused variables
  • Apply formatting guidelines.

IMO, it can be merged even before the CycloneDDS PR is accepted. The only impact is building the entire CycloneDDS suite when we just require the IDL part.

I believe we can go on with accepting the PR and to clearly state in the README that you'd need to build the entire CycloneDDS suite until the corresponding PR is accepted.

Final tweaks before merging into eclipse-zenoh
@cguimaraes
Copy link
Member

These have been added:

  • Removed unused variables
  • Apply formatting guidelines.

Looks ready to merge.

@cguimaraes cguimaraes merged commit 071eb4f into eclipse-zenoh:master Feb 20, 2023
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