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

Merge ACK and ACK_ECN #1801

Merged
merged 11 commits into from Sep 27, 2018
Merged

Merge ACK and ACK_ECN #1801

merged 11 commits into from Sep 27, 2018

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Sep 26, 2018

Fixes #1778

Also changes the type value for ACK, ACK_ECN, and RETIRE_CONNECTION_ID in order to give the ACK frame two adjacent values.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

I might have done a more thorough renumber, but this is a nice minimal change. We can kick around the renumbering thing later.


Gap and ACK Block fields use a relative integer encoding for efficiency. Though
each encoded value is positive, the values are subtracted, so that each ACK
Block describes progressively lower-numbered packets. As long as contiguous
ranges of packets are small, the variable-length integer encoding ensures that
each range can be expressed in a small number of octets.

The ACK frame uses the least significant bit(type=0x1b) to indicate ECN feedback
Copy link
Member

Choose a reason for hiding this comment

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

"bit (that is, type 0x1b)"

Receivers send ACK frames (type=0x0d) to inform senders which packets they have
received and processed. The ACK frame contains any number of ACK blocks.
ACK blocks are ranges of acknowledged packets.
Receivers send ACK frames (types=0x1a - 0x1b) to inform senders of packets they
Copy link
Member

Choose a reason for hiding this comment

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

(types 0x1a and 0x1b)

received and processed. The ACK frame contains any number of ACK blocks.
ACK blocks are ranges of acknowledged packets.
Receivers send ACK frames (types=0x1a - 0x1b) to inform senders of packets they
have received and processed. The ACK frame contains one or more ACK blocks. ACK
Copy link
Member

Choose a reason for hiding this comment

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

should we be capitalizing the B in ACK Blocks?

@ianswett
Copy link
Contributor Author

I decided against the big renumber. I thought it'd be best done interactively by 2 or 3 people together.

Block wasn't consistently capitalized, so I made it so everywhere.

@ianswett
Copy link
Contributor Author

Any other comments? Martin or Jana, would you be willing to merge this?

@martinthomson martinthomson merged commit 7bfe615 into master Sep 27, 2018
@martinthomson martinthomson deleted the ianswett-ack-ecn-merge branch September 27, 2018 17:17
alagoutte added a commit to alagoutte/base-drafts that referenced this pull request Sep 30, 2018
- Merge ACK and ACK_ECN (quicwg#1801)
- Add 2 transport parameters: max_ack_delay(12) and original_connection_id(13) (quicwg#981, quicwg#1710, quicwg#1486)
- Remove sequence field from NEW_CONNECTION_ID (quicwg#1742)
- Add RETIRE_CONNECTION_ID type (quicwg#1742)
MikeBishop pushed a commit that referenced this pull request Oct 2, 2018
- Merge ACK and ACK_ECN (#1801)
- Add 2 transport parameters: max_ack_delay(12) and original_connection_id(13) (#981, #1710, #1486)
- Remove sequence field from NEW_CONNECTION_ID (#1742)
- Add RETIRE_CONNECTION_ID type (#1742)
MikeBishop pushed a commit that referenced this pull request Oct 2, 2018
- Merge ACK and ACK_ECN (#1801)
- Add 2 transport parameters: max_ack_delay(12) and original_connection_id(13) (#981, #1710, #1486)
- Remove sequence field from NEW_CONNECTION_ID (#1742)
- Add RETIRE_CONNECTION_ID type (#1742)
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.

None yet

2 participants