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

Bmw 631/add more info #48

Merged
merged 4 commits into from
May 9, 2023
Merged

Bmw 631/add more info #48

merged 4 commits into from
May 9, 2023

Conversation

AlexSeongJu-sr
Copy link
Contributor

I feel like
map<string, string> extra_info
is rather messy.

so I just added sensor_name and topic.

Copy link
Member

@thor-sr thor-sr left a comment

Choose a reason for hiding this comment

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

This will break backwards compatibility, no?

Status status = 1;
map<string, SensorStatus> sensors = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

@thor-sr @sebastian-seoul-robotics FYI, we output pairs of ROS topic and sensor status, where users have no idea about which topic corresponds to which lidar. Alex is changing the key to use the UID and putting extra info (e.g., lidar name in SENSR and topic name for backward compatibility). Here's the link to the relevant conversation: https://seoulrobotics.atlassian.net/browse/BMW-631?focusedCommentId=32983

output.proto Outdated Show resolved Hide resolved
@AlexSeongJu-sr
Copy link
Contributor Author

image
when changed with this proto msg, output will look like this.

@sebastian-seoul-robotics
Copy link
Contributor

This will break backwards compatibility, no?

It will break backwards compatibility. As long as it not applied to sensr_i_release/3.1.x, there should not be a problem. The next release will be a major update (4.0). So no backwards compatibility required

@mogandaz fyi

@AlexSeongJu-sr
Copy link
Contributor Author

AlexSeongJu-sr commented May 9, 2023

so I guess it is fine to merge to master as long as I don't merge to sensr_i_release.
could someone approve ?

@thor-sr thor-sr self-requested a review May 9, 2023 08:52
output.proto Outdated
Status status = 1;
map<string, SensorStatus> sensors = 3;
map<string, SensorInfo> sensors = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be safe, let's reserve 3 and change the ID to 4. Reusing the id is strictly forbidden. Then this would not break the existing client applications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds promising

@AlexSeongJu-sr AlexSeongJu-sr merged commit 2c65524 into master May 9, 2023
@AlexSeongJu-sr AlexSeongJu-sr deleted the BMW-631/add_more_info branch May 9, 2023 09:03
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