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

feat: add message definition for encoded state #132

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Jul 16, 2024

Description

As described in the parent issue, it would be better to define our EncodedState explicitly and not as pure typedef. These changes accomplish that with the minimal amount of change. Two things:

  1. I'm not sure if the layout field in the message definitions helps with serializing or anything (ROS internal) so I'm not sure if it's worth keeping
  2. The typedef in modulo core is now a bit redundant. I could remove those and just change all the includes which have the same result with more time effort but is maybe a bit cleaner?

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

@domire8 domire8 linked an issue Jul 16, 2024 that may be closed by this pull request
LouisBrunner
LouisBrunner previously approved these changes Jul 16, 2024
Copy link

@LouisBrunner LouisBrunner left a comment

Choose a reason for hiding this comment

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

From what I understand, none of the usage have to change for this? Having a message with a single data is equivalent to what Uint8Array used to do? If so LGTM

Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

Needs a changelog entry. API side, nothing changes because of the convenient typedef. Might still be ABI breaking though, not sure; it would be nice to be part of 5.0 proper, so I'm glad we have this release candidate.

source/modulo_interfaces/msg/EncodedState.msg Outdated Show resolved Hide resolved
@domire8 domire8 merged commit 0160ca2 into main Jul 18, 2024
4 checks passed
@domire8 domire8 deleted the feat/encoded-state branch July 18, 2024 11:33
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ROS message type for Protobuf messages
4 participants