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

new package and interfaces for describing other types #146

Merged
merged 9 commits into from Mar 2, 2023

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Oct 11, 2022

Still in development, but related to "evolving message types" REP-2011: ros-infrastructure/rep#358

There are still TODOs and known issues, but I think can iterate on those after merging this initial MVP. Merging will also unblock others working on related features.

@wjwwood wjwwood self-assigned this Oct 11, 2022
@jhurliman
Copy link

Is this ready for review, or what components are missing / still in flux?

@wjwwood
Copy link
Member Author

wjwwood commented Dec 6, 2022

@jhurliman There are some TODO's left for me to update. We've been working on other technical parts of the REP lately so this hasn't made any progress lately. However, we will be returning to it before long.

@wjwwood
Copy link
Member Author

wjwwood commented Dec 6, 2022

Also, we took some time to reconsider having our own definition versus re-using something existing like the dds-xtypes_typeobject.idl from DDS-XTypes, but we decided to recommend against that and keep our custom format. I have to write up those thoughts into the REP as well.

@emersonknapp
Copy link
Collaborator

@clalancette @wjwwood what next steps do you think we need to take to get this landed? perhaps a set of consolidated TODO items will help us get it going. are any further updates needed to REP-2011 before we can move forward with these interfaces, or have we solidified that documented design well enough that it's only implementation details now?

@emersonknapp
Copy link
Collaborator

@wjwwood would you mind also rebasing this PR on rolling, when checked out it's an issue because of new service_msgs package being required by dependents

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Feb 15, 2023

Rebased it on rolling.

@wjwwood wjwwood marked this pull request as ready for review February 15, 2023 01:47
@wjwwood wjwwood requested a review from gbiggs as a code owner February 15, 2023 01:47
@wjwwood
Copy link
Member Author

wjwwood commented Feb 15, 2023

I've moved this out of draft pr mode, and I think it's ready for review.

Copy link

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

Small nits, but other than that I can't think of any fundamental issues with the message definitions

type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
type_description_interfaces/CMakeLists.txt Show resolved Hide resolved
type_description_interfaces/package.xml Show resolved Hide resolved
Copy link

@allenh1 allenh1 left a comment

Choose a reason for hiding this comment

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

Looks good to me as is.

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM (after fixing typos)

Copy link

@allenh1 allenh1 left a comment

Choose a reason for hiding this comment

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

@wjwwood on second pass, I have a couple of suggestions.

type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
type_description_interfaces/msg/FieldType.msg Show resolved Hide resolved
type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
type_description_interfaces/msg/FieldType.msg Outdated Show resolved Hide resolved
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor

clalancette commented Mar 1, 2023

@allenh1 @wjwwood @methylDragon @emersonknapp @james-rms I've now done the updates to this PR that we discussed last night. I think this should be ready for final review. Here is CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Member Author

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@allenh1 allenh1 left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Looks good to me.

@clalancette
Copy link
Contributor

CI was...very yellow. Even though I don't think it is the fault of this PR, here is another try:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

The flake8 failures are known (and fixed elsewhere). The one additional failure is on Windows, but that is a known flake for right now. So this all looks good, I'm going to go ahead and merge this in.

@clalancette clalancette merged commit 18d891a into rolling Mar 2, 2023
@clalancette clalancette deleted the type_description_interfaces branch March 2, 2023 18:31
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.

None yet

7 participants