Skip to main content

Last Call Review of draft-ietf-lpwan-schc-yang-data-model-14
review-ietf-lpwan-schc-yang-data-model-14-yangdoctors-lc-clarke-2022-07-15-00

Request Review of draft-ietf-lpwan-schc-yang-data-model
Requested revision No specific revision (document currently at 21)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2022-08-02
Requested 2022-07-12
Requested by Éric Vyncke
Authors Ana Minaburo , Laurent Toutain
I-D last updated 2022-07-15
Completed reviews Yangdoctors Early review of -04 by Carl Moberg (diff)
Yangdoctors Last Call review of -14 by Joe Clarke (diff)
Intdir Last Call review of -15 by Donald E. Eastlake 3rd (diff)
Genart Last Call review of -14 by Meral Shirazipour (diff)
Secdir Last Call review of -14 by Carl Wallace (diff)
Iotdir Telechat review of -15 by Qin Wu (diff)
Assignment Reviewer Joe Clarke
State Completed
Request Last Call review on draft-ietf-lpwan-schc-yang-data-model by YANG Doctors Assigned
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/WPk5v2OEa5ayXnjtwhE5gZz38Ho
Reviewed revision 14 (document currently at 21)
Result Not ready
Completed 2022-07-15
review-ietf-lpwan-schc-yang-data-model-14-yangdoctors-lc-clarke-2022-07-15-00
I have been tasked to perform a LC review on this draft on behalf of YANG
Doctors.  This document defines a YANG module to codify the rules for Static
Context Header Compression.  While I say that it is "Not Ready" from a LC
perspective, it may be more ready than that lets on.  I have a few questions of
the authors to gain perspective on what they're trying to achieve.  I have also
found a number of issues in the module itself.  See below:

- YANG tree has subtle differences from the YANG module; regenerate
- Run the module through `pyang -f yang` to generate a canonical formatted
version - No linting errors, though the copyright needs to be updated - Pet
peeve: I prefer descriptions to begin with a capital letter and end with a
full-stop '.' - Many descriptions (especially of identities) have an RFC in
them.  Please make this a reference as well. - For fl-type, if this is a
positive integer, why not use an unsigned type? - appiid and deviid should be
AppIID and DevIID as stated in RFC8724 - In grouping tv-struct's description:
s/enconding/encoding/ - Leaf "index" in tv-struct grouping: s/indicia/index/ -
In leaf field-position: s/occurence/occurrence/ - You use "YANG referenceid" a
few times, but this isn't a thing per se.  You tend to use
  this to mean an identity reference.  In all cases, though, I think it would be
  best to more clearly state what the leaf is/does and leave out that fact that
  it uses an identityref
- In grouping compression-rule-entry: s/identifer/identifier/, but again, I'd
leave
  out these YANG bits from descriptions.
- In your tv-struct grouping, define "index" as the first leaf.  That seems a
bit more logical to me. - For leaf comp-decomp-action, your description is a
tautology (which is another pet peeve of mine).
  Can you sweep descriptions and make sure they provide some additional context
  or at least a reference?
- For something like di-type, does it make sense to be an identity?  Seems like
this could be an enumeration
  as I don't think you'll have directions other than up, down, bi?  But maybe
  you think these types may be extended?  Just curious.
- In leaf direction: s/bidirectionnal/bidirectional/ and s/forbiden/forbidden/
- For w-size, why not use derived-from-or-self here (same in other places where
fragmentation-mode is referenced)? - In grouping compression-content:
s/identifed/identified/ - See Section 3.4 of
https://datatracker.ietf.org/doc/html/rfc8407 on how to reference YANG tree
diagrams - Why do you have empty cases for the mode choice (e.g., no-ack,
ack-always)?  It's not clear you need a choice here.
  And if you need it, you could just have one "when" clause on the case itself.
- There is another empty case for the no-compression case in the nature choice.
 The description there says that a rule is required. - Can one have both
features for compression and fragmentation?  The choice seems to imply no, but
I am curious.  I didn't get the impression that they were mutually exclusive
from 8724.