Last Call Review of draft-ietf-ccamp-layer1-types-04
review-ietf-ccamp-layer1-types-04-yangdoctors-lc-wilton-2019-12-13-00

Comments on the document:

1) Please check the English grammar for the earlier paragraphs.

2) If you change the prefix (as in the YANG comments below) then there are
references in the document that will also need to be updated (e.g. section 3,
and the IANA section).

3) Security section, "onsidered" => "considered"

Comments on the YANG module:

1) I would suggest changing the YANG prefix to "l1-types" to help keep it short
(particularly for the identities).

2) I would suggest making the module "yang-version 1.1;", because the behaviour
of YANG 1.1 is better specified.  In fact, I would recommend that all IETF YANG
modules are defined as YANG 1.1.

3) As per my comments in the layer-0-types module, I would suggest using a
decimal64 for the tributary-slot-granularity, or perhaps even a uint32 if it is
specified in Mega rather than Giga.  Also, should the units be Ghz?

4) For all the ODU-type definitions, I would recommend splitting the
description from the reference.  I also don't think that the "which is
categorized as standards track is necessary/useful.  Or if you don't want to
keep it, then I would retain it only for types that are not standard."

E.g.
OLD:

 identity ODU0 {
   base odu-type;
   description
     "ODU0 protocol (1.24G), RFC7139/ITU-T G.709, which is
      categorized as standards track .";
 }

NEW:
 identity ODU0 {
   base odu-type;
   description "ODU0 protocol (1.24G)"
   reference "RFC7139/ITU-T G.709";
 }

5) Some of the ODU-types don't have references.  I would be good to add
references to everything that you can (e.g. also the client-signal identities)

6) For the description of ETH-10Gb-WAN, it might be helpful to also specify the
actual rate (9.95Gb/s) in the description, since it may not be obvious.

7) Looking at how they are used, it might be better for otn-label-range-type to
be represented as a enum.

8) In grouping otn-label-range-info:
 - I suggest making range type an enum rather than identity.
 - Perhaps add a when statement to the "tsg" leaf, so that it can only be
 populated when the enum is a "trib-slot"?  Or otherwise the description is
 unclear. - I wasn't sure how familiar "tsg" is, and perhaps it would be better
 in its expanded form. - Please add more description to Priority.

9) In grouping otn-label-start-end:
 - Probably expand the "tpn" and "ts" names?
 - Probably define typedefs for "tpn" and "ts" uint16's, since they are used in
 multiple grouping definitions.

10) In grouping otn-label-hop:
 - Expand "tpn" name and use the typedef (as per previous comment).
 - Expand "tsg" name
 - Expand the ts-list name, and perhaps enhance the description to state that
 values must be in non-overlapping ascending ordering.   Or you could borrow
 some text for section 9.2.4 of RFC 7950.

11) In grouping "otn-label-step" the "default" statements don't really have any
effect.  I think that either you need to add a default statement to the choice,
or you should delete the default statements under the tpn and ts leaves.

12) identity coding-func.
- All of these identities should be before all of the groupings.
- They also need to have their indentation fixed (e.g., "pyang -f yang
--yang-line-length 69") - Are the references to MEF 63 the right choice, or
should these references be to IEEE Std 802.3?

13) identity optical-interface-func
- Are the references to MEF 63 the right choice, or should these references be
to IEEE Std 802.3? - "base identity" => "Base identity" - Do you definitely
want the "clause-XX" as part of each identity name?  Is it possible that these
clauses could ever change or be renumbered?

14) identity service-performance-metric:
- Rather than "list of service-specific ..." perhaps "Base identity for service
specific ..." - Perhaps change "One-way-..." to "One-Way-...", it looks strange
to have a single character not capitialized. - Probably remove the hypens from
the descriptions?

15) identity network-performance-metric
- Should any identities be defined here?
- Rather than "list of network-specific ..." perhaps "Base identity for network
specific ..."