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

Deprecate robotlocomotion_lcmtypes and bot2_core_lcmtypes #14884

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Apr 11, 2021

Port existing Drake code to use the new types.

Add helper header drake/systems/sensors/robotlocomotion_compat.h to alias the old names onto the new names, along with a deprecation warning.

Requires #14885 first. Towards #14362.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

\CC @RussTedrake on inbound change coming next week -- robotlocomotion.image_t and robotlocomotion.image_array_t will be renamed to drake.lcmt_image and drake.lcmt_image_array, alongside all of our usual Drake LCM message types. The manipulation_station uses these messages for its cameras.

The deprecation affordances here were enough to pass all Anzu CI tests, and I didn't immediately see anything in the underactuated or manipulation repositories that used these messages. But possibly you'd like to check this over.

@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-snopt-packaging please in case you need binaries to test on.

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: with one small and impractical misgiving below.

Reviewed 32 of 32 files at r1.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)


lcmtypes/BUILD.bazel, line 74 at r1 (raw file):

drake_lcm_cc_library(
    name = "header",
    lcm_package = "drake",

minor: This seems to me to violate the spirit of the namespace/directory rule ("The code in that namespace should usually be in a directory whose basename matches the namespace name") but not the letter of it ("... or in a subdirectory thereof" which would seem to argue that the bare drake namespace is always appropriate).

This is probably too much of a pain to deal with anyway. Author's discretion.

@jwnimmer-tri jwnimmer-tri force-pushed the lcmtypes-robotlocomotion-deprecation branch from 0dba39a to 126c149 Compare April 12, 2021 14:01
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review April 12, 2021 14:02
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.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)


lcmtypes/BUILD.bazel, line 74 at r1 (raw file):

Previously, ggould-tri wrote…

minor: This seems to me to violate the spirit of the namespace/directory rule ("The code in that namespace should usually be in a directory whose basename matches the namespace name") but not the letter of it ("... or in a subdirectory thereof" which would seem to argue that the bare drake namespace is always appropriate).

This is probably too much of a pain to deal with anyway. Author's discretion.

OK Fixing the packaging and include paths is part of the #14362 scope already, but will happen later.

@jwnimmer-tri
Copy link
Collaborator Author

Tagging +@ggould-tri officially for feature review.

+@EricCousineau-TRI for platform review per schedule, 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.

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI)


lcmtypes/BUILD.bazel, line 74 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK Fixing the packaging and include paths is part of the #14362 scope already, but will happen later.

👍

Copy link
Contributor

@EricCousineau-TRI EricCousineau-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: with two minor nits.

Reviewed 26 of 32 files at r1.
Reviewable status: 2 unresolved discussions (waiting on @jwnimmer-tri)


systems/sensors/BUILD.bazel, line 120 at r2 (raw file):

# TODO(jwnimmer-tri) This header assists with the deprecation of the
# @lcmtypes_robotlocomotion repository; remove it on or after 2021-08-01.
drake_cc_library(

nit This is presently public (per default visibility).

Can you make this private?


systems/sensors/robotlocomotion_compat.h, line 20 at r2 (raw file):

namespace robotlocomotion {

using header_t DRAKE_DEPRECATED("2021-08-01",

nit From brief inspection, not sure if I see these aliases tested.

Consider adding minor unittest, e.g. static_assert(std::is_same_v<robotlocomotion::header_t, drake::lcmt_header>, ""), etc.?

Port existing Drake code to use the new types.

Add helper header systems/sensors/robotlocomotion_compat.h to alias the old
names onto the new names, along with a deprecation warning.
@jwnimmer-tri jwnimmer-tri force-pushed the lcmtypes-robotlocomotion-deprecation branch from 126c149 to 7299ed1 Compare April 12, 2021 21:06
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.

Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI and @ggould-tri)


systems/sensors/BUILD.bazel, line 120 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This is presently public (per default visibility).

Can you make this private?

I think it should be public. Anyone who wants to ease their transition to the new spellings would want to #include this?


systems/sensors/robotlocomotion_compat.h, line 20 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit From brief inspection, not sure if I see these aliases tested.

Consider adding minor unittest, e.g. static_assert(std::is_same_v<robotlocomotion::header_t, drake::lcmt_header>, ""), etc.?

Done.

That particular test is useless -- the compiler already catches those kind of errors. Instead, I forked one of the unit tests from prior to this PR, checking that it still passes (with the only change being that the #include "robotlocomotion/..." line has to be removed.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-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 32 files at r1, 2 of 2 files at r3.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),EricCousineau-TRI(platform) (waiting on @jwnimmer-tri)


systems/sensors/BUILD.bazel, line 120 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think it should be public. Anyone who wants to ease their transition to the new spellings would want to #include this?

OK SGTM!

FWIW Would have been clearer w/ explicit visibility tag and comment but meh for now.


systems/sensors/robotlocomotion_compat.h, line 20 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done.

That particular test is useless -- the compiler already catches those kind of errors. Instead, I forked one of the unit tests from prior to this PR, checking that it still passes (with the only change being that the #include "robotlocomotion/..." line has to be removed.

OK Works for me! (not 100% on board with "useless", but I'm like 70% on board w/ it)

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.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),EricCousineau-TRI(platform) (waiting on @jwnimmer-tri)


systems/sensors/robotlocomotion_compat.h, line 20 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Works for me! (not 100% on board with "useless", but I'm like 70% on board w/ it)

I could not imagine an implementation error that such a test would find, but would otherwise go undetected.

@jwnimmer-tri jwnimmer-tri merged commit ec769b1 into RobotLocomotion:master Apr 12, 2021
@jwnimmer-tri jwnimmer-tri deleted the lcmtypes-robotlocomotion-deprecation branch April 12, 2021 22:13
@EricCousineau-TRI
Copy link
Contributor


systems/sensors/robotlocomotion_compat.h, line 20 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I could not imagine an implementation error that such a test would find, but would otherwise go undetected.

Could ensure that it's an alias to the right type (non-error typo).
True, should be caught via code review, but add'l active check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants