Re: [quicwg/base-drafts] Alternative packet format presentation (#3573)

[Mike Bishop <[e-mail]>]

@MikeBishop commented on this pull request.

But...!  But...!  *Muh ASCII art!*

More seriously, this is probably a net improvement for readability.  There's value in conforming to existing paradigms where possible, but we'd already had to define custom extensions to the notation from RFC2360, so this is probably no worse.

I'm really not a fan of the trailing comma in the lists, but that's neither here nor there.

> +  Protected Payload (0..24), # Skipped
+  Protected Payload (128),   # Sampled

```suggestion
  Protected Payload (0..24), # Skipped Part
  Protected Payload (128),   # Sampled Part
```
Inconsistent with the below.  Resolve either direction, but I don't see a reason to make them different.

> +  Protected Payload (128),   # Sampled
+  Protected Payload (..)     # Remainder
+}
+
+Short Header Packet {
+  Header Form (1) = 0,
+  Fixed Bit (1) = 1,
+  Spin Bit (1),
+  Reserved Bits (2),         # Protected
+  Key Phase (1),             # Protected
+  Packet Number Length (2),  # Protected
+  Destination Connection ID (0..160),
+  Packet Number (8..32),     # Protected
+  Protected Payload (0..24), # Skipped Part
+  Protected Payload (128),   # Sampled Part
+  Protected Payload (..)     # Remainder

```suggestion
  Protected Payload (..),    # Remainder
```
I'm not really a fan of the trailing commas, but for consistency sake, you missed this one.

> +  a mimumum of zero bits and B can be omitted to indicate no set upper limit;
+  values always end on an octet boundary

```suggestion
  a mimumum of zero bits and B can be omitted to indicate no set upper limit;
  values in this format always end on an octet boundary
```
Just to be clear, since not all values end on an octet boundary.  I do think separate sentences would be more readable, but then it would be inconsistent with the others.

> +
+x (?) = C:
+: Indicates that x has a fixed value of C
+
+x (?) = C..D:
+: Indicates that x has a value in the range from C to D, inclusive
+
+\[x (E)\]:
+: Indicates that x is optional (and has length of E)
+
+x (E) ...:
+: Indicates that x is repeated zero or more times (and that each instance is
+  length E)
+
+By convention, individual fields reference a complex field by using the name of
+that the complex field.

```suggestion
the complex field.
```

> +  Variable-Length Field (8..24),
+  Field With Minimum Length (16..),

```suggestion
  Arbitrary-Length Field (..),
  Variable-Length Field (8..24),
  Field With Minimum Length (16..),
```

Omitting both ends is used in the document and consistent with the definition, but not clearly spelled out or used in an example.

> @@ -3926,11 +3938,11 @@ DCID Len:
 
 Destination Connection ID:
 
-: The Destination Connection ID field follows the DCID Len and is between 0 and
-  20 bytes in length. {{negotiating-connection-ids}} describes the use of this
-  field in more detail.
+: The Destination Connection ID field follows the DCID Length and is between 0

```suggestion
: The Destination Connection ID field follows the DCID Length field and is between 0
```

> @@ -3942,7 +3954,7 @@ SCID Len:
 
 Source Connection ID:
 
-: The Source Connection ID field follows the SCID Len and is between 0 and 20
+: The Source Connection ID field follows the SCID Length and is between 0 and 20

```suggestion
: The Source Connection ID field follows the SCID Length field and is between 0 and 20
```

> -+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                     Packet Number (8/16/24/32)              ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                     Protected Payload (*)                   ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+Short Header Packet {
+  Header Form (1) = 0,
+  Fixed Bit (1) = 1,
+  Spin Bit (1),
+  Reserved Bits (2),
+  Key Phase (1),
+  Packet Number Length (2),
+  Destination Connection ID (0..160),
+  Packet Number (8..32),
+  Packet Payload (..),
+}
 ~~~~~
 {: #fig-short-header title="Short Header Packet Format"}
 

GitHub won't let me put a comment down there (too far from a change), but you can remove the (S), (R), (K), (P) notations from the field definitions, since they're not needed to correlate with the diagram.

> @@ -5354,13 +5257,11 @@ with error STREAM_STATE_ERROR.
 The MAX_STREAM_DATA frame is shown in {{fig-max-stream-data}}.
 
 ~~~
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                        Stream ID (i)                        ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                    Maximum Stream Data (i)                  ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+MAX_STREAM_DATA Frame {
+  Type (i) = 0x11,
+  Stream ID (i),
+  Maximum Data (i),

Orthogonal to this PR, but perhaps we should align the "Maximum Data" and "Data Limit" terms which reference the same limit in different frames?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3573#pullrequestreview-390801974