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

Extract ROS 2 bridge header #228

Merged
merged 6 commits into from May 17, 2023

Conversation

mvukov
Copy link
Contributor

@mvukov mvukov commented May 17, 2023

Public-Facing Changes

Extracts ROS 2 bridge header such that the node class can be used directly, not only as a component.

Description

I want to compile and use the bridge in https://github.com/mvukov/rules_ros2. I don't have support for components, yet, so I'd like to use the bridge library itself. For that reason, I extracted the header file.

There are no functional changes.

@CLAassistant
Copy link

CLAassistant commented May 17, 2023

CLA assistant check
All committers have signed the CLA.

@achim-k
Copy link
Collaborator

achim-k commented May 17, 2023

In general this LGTM, thanks Milan!

Some comments:

  • You will have to add a install rule for that public header (see below)
  • I think we can rename the header from ros2_foxglove_bridge.hpp -> foxglove_bridge.hpp
  • Could you change ros2_foxglove_bridge_node.cpp to use that new header?

Potential install rule:

    install(FILES ros2_foxglove_bridge/include/foxglove_bridge/foxglove_bridge.hpp
      DESTINATION include/${PROJECT_NAME}/
    )

@mvukov
Copy link
Contributor Author

mvukov commented May 17, 2023

I think we can rename the header from ros2_foxglove_bridge.hpp -> foxglove_bridge.hpp

Should I also rename the .cpp file for consistency?

Could you change ros2_foxglove_bridge_node.cpp to use that new header?

AFAICS, the node doesn't need the header as it loads the component in runtime. WDYT?

@achim-k
Copy link
Collaborator

achim-k commented May 17, 2023

Should I also rename the .cpp file for consistency?

Yes, good idea

AFAICS, the node doesn't need the header as it loads the component in runtime. WDYT?

Not super important actually. The node starts an extra component manager node which we could avoid when launching the foxglove bridge node directly (using the header). But this is something I can always do later, so indeed no need to do this now.

@mvukov
Copy link
Contributor Author

mvukov commented May 17, 2023

In fact, I can't rename the header to foxglove_bridge.hpp because there already exists one in the base library. The import path foxglove_bridge/foxglove_bridge.hpp is then ambiguous. Please let me know how you'd like to proceed.

@mvukov
Copy link
Contributor Author

mvukov commented May 17, 2023

You will have to add a install rule for that public header (see below)

Done. @achim-k PTAL.

@achim-k achim-k merged commit eb02171 into foxglove:main May 17, 2023
7 checks passed
@mvukov mvukov deleted the refactor/ros2_bridge_header branch May 17, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants