Skip to content

Conversation

@saikishor
Copy link
Member

This PR adds the StringArray and ValuesArray message needed for the generic state broadcaster or the interface state broadcaster

Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

shouldnt the names be more specific? Like interface_names and interface_values or something like this? And why using two messages? If you want to go in the direction of pal_statistics, why not just use the messages from there?

@saikishor
Copy link
Member Author

shouldnt the names be more specific? Like interface_names and interface_values or something like this? And why using two messages? If you want to go in the direction of pal_statistics, why not just use the messages from there?

I thought of having new messages that are generic and fix different use cases at the same time. The idea of using 2 messages is to reduce the size of the message. As per @bmagyar suggestion, I think it is a good idea to go with 2 different publishers, one as a transient_local for names and another for the list of values

Regarding pal_statistics messages, I didn't want to depend on pal_statistics messages per se, as they semantically do not fit the current use case. Moreover, we will have only 1 version and won't be using the version tag

@christophfroehlich
Copy link
Member

I thought of having new messages that are generic and fix different use cases at the same time.

Following the strategy of removing generic message types from std_msgs, I'd avoid "generic" messages but be more descriptive here.

The idea of using 2 messages is to reduce the size of the message. As per @bmagyar suggestion, I think it is a good idea to go with 2 different publishers, one as a transient_local for names and another for the list of values

This is fine.

Regarding pal_statistics messages, I didn't want to depend on pal_statistics messages per se, as they semantically do not fit the current use case. Moreover, we will have only 1 version and won't be using the version tag

Ok.

@saikishor
Copy link
Member Author

Following the strategy of removing generic message types from std_msgs, I'd avoid "generic" messages but be more descriptive here.

@christophfroehlich What you say is very valid. I understand it. I was about to make the change and unfortunately found this InterfaceValue.msg this doesn't allow me to rename the message. Any help with alternative naming?

@christophfroehlich
Copy link
Member

Maybe InterfaceNames and InterfaceValues? This would be similar to pal_statistics_msgs?
Or InterfacesNames and InterfacesValues?

@saikishor saikishor changed the title Add StringArray and ValuesArray messages Add InterfacesNames and InterfacesValues messages Nov 13, 2025
Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

pre-commit, apart from that LGTM

@saikishor saikishor force-pushed the add/string_and_values_array branch from fe1aed5 to 2c194f7 Compare November 16, 2025 00:51
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Copy link

@Juliaj Juliaj 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

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

I would recommend a lot more documentation about when/where to use this and how it compares to InterfaceValue.msg which is a neighbor and appears equivalent. When should one be used versus the other?

I believe that these are assuming correlation of ordering and length between these two messages. That should be documented.

Will it make sense to capture a sense of scope for what these interfaces are covering? If I was introspecting a rosbag I'm not sure that I could render these values helpfully without more information.

@christophfroehlich
Copy link
Member

christophfroehlich commented Nov 24, 2025

I understand your point of view. We just maybe are thinking from the other end, we plan to implement a new broadcaster: Together with its usage the message make sense (for us). With having a look on the messages only, it might not.
We can at least document the advantages of the name-value messages in the msg file itself?

@christophfroehlich christophfroehlich moved this from Needs review to WIP in Review triage Dec 5, 2025
@saikishor saikishor changed the title Add InterfacesNames and InterfacesValues messages Add Keys and Float64Values messages Dec 8, 2025
@saikishor
Copy link
Member Author

As per the voting to rename the messages during the last PMC meeting (03rd December, 2025) I renamed the messages to Keys.msg and Float64Values.msg

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
@christophfroehlich christophfroehlich merged commit b4b8a0d into master Dec 9, 2025
11 of 12 checks passed
@christophfroehlich christophfroehlich deleted the add/string_and_values_array branch December 9, 2025 18:00
@github-project-automation github-project-automation bot moved this from WIP to Done in Review triage Dec 9, 2025
@christophfroehlich christophfroehlich added the backport-jazzy Triggers PR backport to ROS 2 jazzy. label Dec 15, 2025
mergify bot pushed a commit that referenced this pull request Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-jazzy Triggers PR backport to ROS 2 jazzy.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants