Subject: [rohc] FN draft review

From: "West, Mark" <[email-masked]>
Date: Tue, 31 Oct 2006 13:34:58 -0000

Hi all,

Herewith a review of the RoHC FN draft (-11).

A general comment: this draft looks to be in pretty good shape; I, for one, find it more readable than on its previous outings.  My thanks, therefore, to the authors and previous reviewers for their efforts in making that the case!

I've split this review into 'real' comments / questions first, and left a bunch of NITs to the end.

Cheers,

Mark.

Comments:

Section 2

* "Context", certainly at the compressor, a context would be expected to contain a list of 1 or more values?

* "Encoding method", is there any particular reason to use both 'function' and 'relation', rather than just 'relation' (or 'relationship')?

Section 3.2.1

* "one unspecified data object and one specified object" -- is it possible to bind two unspecified objects?

* "At the decompressor, the operation is reversed..." -- presumably decoding fails if the binding fails?

* "For example, the "static" encoding method creates a binding..." is true but incomplete, because it has also to bind the lengths.  It might be nice to spell this out?

* "Both the compressed and uncompressed forms of each field are mapped to 'bits on the wire'... -- what does it mean to map an _uncompressed_ field 'on the wire'?  Or does this simply refer to bit/byte ordering issues?

Section 3.2.2

* "control fields, that are additional information added to the compressed format during compression" -- seems a little confusing (to me, at least!)
Perhaps "control fields, that represent context information separate from that contained in the uncompressed header", or something..?

* "this is possible because ctrl_field_3 has no 'uncompressed' value..."  
This seems to contradict previous statements about the nature of the binding in FN?  (i.e. during decompression you do not have any undefined value, but 2 defined values that have to match?)  I guess I'm still a bit unclear as to why this isn't done through a control field, but I think it makes sense...

Section 3.3

* "It is a fail-safe encoding method that can be used..." -- I'm not sure that I believe this.  Or at least, not without the caveat, 'subject to the previous conditions of use'.
At least I can certainly conceive a field (null-terminated value?) that cannot be compressed by 'irregular'.

Section 4.2

* "It is illegal to have two or more identifiers that differ from each other only in capitalisation" -- so is it case sensitive, or not, or..?

Section 4.4.2

* Given that 'v' and 'l' are introduced here just for this expression, and that 'l' is more that slightly ambiguous with '1' (at least in my courier font!), why not use 'VALUE' and 'LENGTH', or something?

* What does it mean to bind comp_value(-1) comp_length(8) to uncomp_value(unspec) uncomp_length(16)?  (Assuming appropriate length bindings)  Do I get 0x00ff or 0xffff out?

Section 4.5

* Can you do the reverse (i.e. separate compressed representations)?

Section 4.7

* I can't believe that the list of exclusions is exhaustive?  However, this probably doesn't matter too much.

Section 4.9

* Would it make sense to give an example for case (3) of the ENFORCE binding?
  For example, I assume that I could use:
    ENFORCE(foo.ULENGTH == bar.ULENGTH)
  to force two fields to be the same (currently unknown) value?

I also assume that, e.g., ENFORCE(foo.UVALUE != 20) allows the decompressor to pick any value for foo.UVALUE that does not contradict any other binding?

Section 4.10

* Presumably the enforcement of lengths can be used to specify the length of one field in terms of the value of another (c.f. TCP SACK block); might be nice to make this explicit (I don't /think/ it currently is...)

Section 4.11

* I don't understand why methods aren't specified 'formally' using a set of ENFORCE statements.  This is typically trivial and adds a degree of rigo(u)r to the definition?

  e.g. 'uncompressed_value' ==>
	ENFORCE(f.ULENGTH = len)
	ENFORCE(f.UVALUE  = val)
	ENFORCE(f.CLENGTH = 0)

Section 4.11.6

* Does the calculation of the CRC include the CRC?  And how is the checking of the CRC defined?

Section 4.12.1.2

* I assume that it is 'illegal' to have an encoding specified for both "UNCOMPRESSED" and "COMPRESSED" encoding methods.  However, I don't think that this is explicitly stated?

Section 4.12.1.4

* Presumably using a binding which relies upon a context value will simply fail or leave the field with an undefined value?  (In other words, it should not be done, but the definition of binding within the notation seems to cover this case.)

NITs

Section 1

* "this sometimes showed to be unclear" -> "this was sometimes unclear"

* "previously existed" -> "exists"

* "is a function that performs" -> "defines" (maybe?  'function' seems like a provocative word!)

Section 2

* in "Uncompressed format", is the text "to be compressed" necessary?

Section 3.1

* "define the inverse, how to relate" -> "define the inverse: how to relate"

* "outline compression strategy principles" -> 
    "outline compression strategy" or
    "outline compression princinples" (but not both!)

Section 3.2.1

* "indicates how long this field is before it is compressed."
    -> "indicates the uncompressed length of the field."
(otherwise you are implying a temporal ordering that is not present in the mapping?)

* "This is similar to the unification process in logic."
    Is this worth a reference?

* "where "length" is the value attribute" -> 
    "where "length" is the length attribute" ???

Section 3.3

* "help the reader's understanding" -> "help the readers' understanding" ???

* "This is notated explicitly by specifying in square brackets a length of 0 
   for the length of the checksum field" ->
    "This is notated explicitly by specifying, in square brackets, a length
     of 0 for checksum field"

Section 4.4

* "Fields are the units which headers are divided into"
    -> "Fields are the units into which headers are divided"

Section 4.11.3

* "<expression> -> "<length_expression>"

Section 4.11.6

* "I.e." -> "i.e."

Section 4.12.1

* "a second field to represent the salient part" ->
    "a second field to represent the rapidly changing part" ->

_______________________________________________
Rohc mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/rohc