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

[lcm] Eschew LCM C++ API (use C instead) #20116

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Sep 2, 2023

Towards #17231.
See also #20115.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added the release notes: none This pull request should not be mentioned in the release notes label Sep 2, 2023
@jwnimmer-tri
Copy link
Collaborator Author

+@ggould-tri for feature review, please.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm_strong: Much simpler!

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers


-- commits line 2 at r1:
nit: Mention the alternative, since most people don't know LCM's full API buffet.

Suggestion:

- 2500a1e: [lcm] Eschew LCM C++ API (use C API instead)

lcm/drake_lcm.cc line 10 at r1 (raw file):

#include <glib.h>
#include <lcm/lcm-cpp.hpp>

BTW: Took me a while to figure out why this was still here -- that styleguide prohibition on forward declaration really proves its point! :-)

C++ is just pointlessly slower, more complicated, and less ABI-stable.
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@sammy-tri for platform review per schedule, please.

Reviewable status: LGTM missing from assignee sammy-tri(platform)

@jwnimmer-tri jwnimmer-tri changed the title [lcm] Eschew LCM C++ API [lcm] Eschew LCM C++ API (use C instead) Sep 5, 2023
Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform)

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),sammy-tri(platform)

@sammy-tri sammy-tri merged commit d34b2dc into RobotLocomotion:master Sep 5, 2023
1 check passed
@jwnimmer-tri jwnimmer-tri deleted the lcm-c-abi branch September 5, 2023 15:47
@RussTedrake
Copy link
Contributor

After this commit, every bazel test I run in the manipulation repo that calls ApplyDefaultVisualization now fails with

connect: Network is unreachable

No route to 246.127.42.142

LCM requires a valid multicast route.  If this is a Linux computer and is
simply not connected to a network, the following commands are usually
sufficient as a temporary solution:

   sudo ifconfig lo multicast
   sudo route add -net 224.0.0.0 netmask 240.0.0.0 dev lo

For more information, visit:
   http://lcm-proj.github.io/lcm/multicast_setup.html

@jwnimmer-tri
Copy link
Collaborator Author

For reference, a probably-relevant detail here is that Russ's repositories block all kernel socket() calls by default:

https://github.com/RussTedrake/htmlbook/blob/bbc905e270eb4ca58bf059c812555b5a52d752b5/tools/python/defs.bzl#L19-L22

This was supposed to be a behavior-neutral change, so I'm reverting in #20136 until I understand why it changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants