Skip to content

remove seq number from Header.msg#2

Merged
wjwwood merged 1 commit intomasterfrom
remove_seq
Jul 14, 2015
Merged

remove seq number from Header.msg#2
wjwwood merged 1 commit intomasterfrom
remove_seq

Conversation

@wjwwood
Copy link
Copy Markdown
Member

@wjwwood wjwwood commented Jun 17, 2015

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 stamp and frame_id, seq is not needed to relate one piece of data to another. Both stamp and frame_id are values with global significance and are values that the middleware cannot realistically set automatically. Whereas seq is 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 in Header.msg interface. Another reason for keeping stamp in the header while seq should 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 the Header.msg.

Links to previous discussions about this issue are in this comment:

#1 (comment)

Request for comment.

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Jun 17, 2015
@dirk-thomas
Copy link
Copy Markdown
Member

LGTM

@esteve
Copy link
Copy Markdown
Member

esteve commented Jun 17, 2015

+1

1 similar comment
@tfoote
Copy link
Copy Markdown
Contributor

tfoote commented Jun 18, 2015

+1

@wjwwood wjwwood self-assigned this Jun 19, 2015
@tfoote
Copy link
Copy Markdown
Contributor

tfoote commented Jun 23, 2015

To send email to community for comment.

@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 25, 2015
@dirk-thomas dirk-thomas changed the title remove seq number from Header.msg remove seq number from Header.msgthe specific Jul 1, 2015
@rethink-imcmahon
Copy link
Copy Markdown

+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.

@jack-oquin
Copy link
Copy Markdown

+1

1 similar comment
@jacquelinekay
Copy link
Copy Markdown

+1

@wjwwood wjwwood changed the title remove seq number from Header.msgthe specific remove seq number from Header.msg Jul 8, 2015
@baalexander
Copy link
Copy Markdown

+1

@gerkey
Copy link
Copy Markdown
Member

gerkey commented Jul 14, 2015

Lots of +1. Please merge.

@wjwwood
Copy link
Copy Markdown
Member Author

wjwwood commented Jul 14, 2015

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
@wjwwood wjwwood merged commit 22b9317 into master Jul 14, 2015
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jul 14, 2015
@wjwwood wjwwood deleted the remove_seq branch July 14, 2015 21:34
@adolfo-rt
Copy link
Copy Markdown

+1 Late, but for the record.

@ros-discourse
Copy link
Copy Markdown

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/foxy-message-api-review/13105/8

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
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.