[TICTOC] Comments on YANG module of draft-ietf-tictoc-1588v2-yang

[Rodney Cummings <[e-mail]>]

Hello all,

I performed a detailed review of section 3 (YANG module), and I have some suggestions. I apologize to my co-authors for not raising these issues sooner (I was more focused on the other sections).

1. Upload the YANG module to https://github.com/YangModels/yang/tree/master/standard/ietf/DRAFT

Now that this module is WG draft, I assume this is appropriate, and it will increase visibility with the YANG community.

2. For data set members specified as "implementation-specific" in 1588-2008, use the data type from clause 15.

The "implementation-specific" text refers to use of the data inside the implementation, and does not refer to management. Management needs a data type, and 1588-2008 provides this for its own management protocol (clause 15). This module should use the data type from clause 15, since YANG is another form of management. As a side note, the revision of 1588 is clarifying this issue, so use of the 1588-2008 clause 15 type will align in the future.

Details:
- current-ds-entry.offset-from-master: Use TimeInterval as type (see suggestion #3, and 15.5.3.4.1.2)
- current-ds-entry.mean-path-delay: Use TimeInterval as type (see suggestion #3, and 15.5.3.4.1.3)
- time-properties-ds-entry.current-utc-offset: Use int16 (see 15.5.3.6.1.1)

3. Create and use a grouping for each data type in 1588-2008 subclause 5.3 (derived data type specifications).

1588-2008 specifies a "struct" for many types, and the data set members use those types. We should repeat this practice in the YANG module for consistency, and to reduce duplication.

Details:
- Create a grouping for TimeInterval of 5.3.2, and use that grouping for:
	* current-ds-entry.offset-from-master
	* current-ds-entry.mean-path-delay
	* port-ds-entry.peer-mean-path-delay
	* transparent-clock-port-ds-entry.peer-mean-path-delay
- Create a grouping for ClockIdentity of 5.3.4, and use that grouping for:
	* default-ds-entry.clock-identity
	* parent-ds-entry.grandmaster-identity
	* transparent-clock-default-ds-entry.clock-identity
- Create a grouping for PortIdentity of 5.3.5, and use that grouping for:
	* parent-ds-entry.parent-port-identity
	* port-ds-entry.port-identity
	* transparent-clock-port-ds-entry.port-identity
- Create a grouping for ClockQuality of 5.3.7, and use that grouping for:
	* default-ds-entry.clock-quality
	* parent-ds-entry.grandmaster-clock-quality

4. Update the description of instance-list.

It is correct to say that each instance represents a distinct domain, but not necessarily a distinct domain-number. To be clearer we could replace the description with: 

"List of one or more PTP datasets in the device, one for each domain (see IEEE 1588-2008 subclause 6.3). Each PTP dataset represents a distinct instance of PTP implementation in the device (i.e. distinct Ordinary Clock or Boundary Clock)."

5. Create and use a typedef for each Enumeration data type in 1588-2008.

1588-2008 uses the data type Enumeration8 (or 16) for data that references a table of name/number pairs. As the 1588 standard evolves with each revision, new name/number pairs will be added.
 
The current draft uses a YANG enumeration for:
	* port-ds-entry.delay-mechanism
	* transparent-clock-default-ds-entry.delay-mechanism
Since 1588-2008 has a single table for both, in the YANG we should create a typedef for this, and use that typedef for both data set members.

The current draft uses YANG "uint8" for other enumerations in 1588-2008. We should consider creating typedefs with an enumeration for those as well:
	* port-ds-entry.port-state
	* time-properties-ds-entry.time-source

Other modules seem to use YANG "identityref" for enumerations (e.g. draft 802.1Q modules), but I admit to ignorance as to why. I would like to understand the advantages/disadvantages of enumeration and identityref, to help ensure that we are using the best type for 1588-2008.

Rodney