Conversation
Member
|
LGTM |
Member
|
+1 |
1 similar comment
Contributor
|
+1 |
Contributor
|
To send email to community for comment. |
|
+1 I think the rational is solid. Elimination of the inconsistent seq usage is definitely welcome. It was always a guessing game who (pub/sub) was supposed to set it, depending on the package. |
|
+1 |
1 similar comment
|
+1 |
|
+1 |
Member
|
Lots of +1. Please merge. |
Member
Author
|
Ok, thanks for everyone's feedback. If you have positive comments or constructive criticism please feel free to continue the discussion here. |
wjwwood
added a commit
that referenced
this pull request
Jul 14, 2015
remove seq number from Header.msg
|
+1 Late, but for the record. |
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: |
Macbull
added a commit
to code-name-57/Robotics-Nav2-SLAM-Example
that referenced
this pull request
Dec 23, 2021
Added sequence to header message in tf related scrpits. header.seq is deprecated and not generally used ros2/common_interfaces#2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This has been proposed as far back as six years ago, but was avoided in ROS 1 to prevent disruption to existing code. The idea is that having a sequence number that is inconsistently set by the middleware, and often set by users, only to have the middleware override it, doesn't really make sense. Rather the middleware should give the sequence number to the subscriber through a separate channel (not as part of the message struct), and prevent the publishing user from setting it manually (it doesn't make sense for them to do so). Therefore it doesn't need to be a field in the
.msg.Also, unlike
stampandframe_id,seqis not needed to relate one piece of data to another. Bothstampandframe_idare values with global significance and are values that the middleware cannot realistically set automatically. Whereasseqis specific to a single publishing source, or topic depending on the middleware's behavior, and doesn't help coordinate the message instance with message instances from other sources or topics. It might be valid for messages to have sequence numbers if the source of the sequence numbers is generated by the user rather than the middleware, but is not needed inHeader.msginterface. Another reason for keepingstampin the header whileseqshould be removed, is that the middleware might provide the subscriber with a message received timestamp, but that is typically distinctly different from the creation of the data timestamp in theHeader.msg.Links to previous discussions about this issue are in this comment:
#1 (comment)
Request for comment.