Re: Gen-ART Telechat review of draft-ietf-tictoc-ptp-mib-08

[vinays <[e-mail]>]

Have addressed the MIB related comments below. The remaining will be 
addressed
by Tim in the latest revision to be submitted tomorrow morning UK time.

Please see inline at VC.


On 4/17/16 4:18 PM, Peter Yee wrote:
> I am the assigned Gen-ART reviewer for this draft. The General Area Review
> Team (Gen-ART) reviews all IETF documents being processed by the IESG for
> the IETF Chair. Please wait for direction from your document shepherd or
> AD before posting a new version of the draft.
>
> For more information, please see the FAQ at
> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>
>
> Document: draft-ietf-tictoc-ptp-mib-08
> Reviewer: Peter Yee
> Review Date: March 8, 2016
> IETF LC End Date: March 8, 2016
> IESG Telechat date: April 21, 2016
>
> Summary: This draft is basically ready for publication as a Standards
> Track RFC, but has nits and a couple of minor issues that should be
> fixed/considered before publication. [Ready with issues]
>
> This draft defines an SMIv2 MIB for use with the Precision Time Protocol
> v2 (IEEE Std. 1588-2008).  I do not have access to this standard, so my
> review primarily addresses nits within the module and not whether the
> module itself is a fair representation of a MIB suitable for use with PTP
> v2.
>
> The following review was originally submitted for the LC (see
> http://www.ietf.org/mail-archive/web/gen-art/current/msg13014.html).  As
> no update has been issued for the draft, the content of that review is
> copied below.
>
> Major issues: None
>
> Minor issues:
>
> Given that the draft seems to quote a fair amount of text (at least 5
> pages) from IEEE Std. 1588-2008 in the definitions section if not
> elsewhere, it might be good to make sure that there are no copyright
> issues.
>
> Many of the object descriptions use identical text.  While that's helpful
> from the direct lookup point of view, it doesn't help to distinguish these
> items from others of their ilk.  I'm not sure if this matters, but perhaps
> enhancing the descriptions in their contexts might make them more useful.
>
> Nits:
>
> General:
>
> Replace all occurrences of "create logical group" with "create a logical
> group".

VC: done.

>
> Watch the case and spacing of object names in their descriptions.
> Sometime there's a space between two words, sometimes not.  Some words are
> capitalized in one place and not another.  Use of camel case conventions
> comes and goes.  It's probably best to treat the object names as English
> words in the descriptions, separating words with spaces and generally
> writing them in lower case.  Sure, the inconsistency doesn't hurt a lot,
> but it ends up looking a wee bit sloppy.  The RFC Editor may not be sure
> how to handle the inconsistency as some word combinations might appear to
> be technical MIB terms that should be left as is.
>
> Drop the draft name from the center, top header.  Generally a shortened
> form of the draft title is placed here.  Perhaps you could use "PTPv2 MIB"?
>
> I'll leave the serial comma be.  It appears in some places and not others.
> I like them, but there are enough missing ones that I'm just going to let
> it slide. ;-)
>
> When citing the official name of IEEE 1588-2008, use a lower case "a”
> before "Precision".  That aligns with IEEE's title.
>
> The authors' addresses in Section 9 have way too many punctuation marks in
> them.  A few less commas would go a long way.  Maybe change some into
> serial commas for sprinkling elsewhere in the document? ;-)
>
> Specific:
>
> Page 2, Section 1, 1st paragraph, 2nd sentence: delete "the” before
> "ordinary".  Make "ordinary clock" and "transparent clock" plural.  Insert
> a comma and "and" after "transparent clocks".
>
> Page 3, Section 1.1, 1st paragraph, 2nd sentence: append a comma after
> "e.g.".
>
> Page 5, 1st bullet item: delete the comma after "STD 62".
>
> Page 5, 2nd bullet item, 2nd sentence: insert "is" before "described".
>
> Page 5, 3rd bullet item, 2nd sentence: insert "is" before "described".
>
> Page 5, 3rd bullet item, last sentence: insert "is" before "described".
>
> Page 7, MIB Description, 1st paragraph, 1st sentence: change "MIB is to
> support" to "MIB supports".
VC: done.

> Page 8, Acronyms, EUI: delete the period after the acronym expansion.

VC: done.


>
> Page 12, PTP node definition: insert "A" before the sentence.

VC: done.

>
> Page 16, ClockIdentity description: the term "MAC-48" has not been defined
> or previously referenced in the draft.  It's also not quite an exact
> synonym for EUI-48, so perhaps it should be defined.

VC: done. Have added MAC-48 in the acronym section.

>
> Page 17, ClockMechanismType description: replace "End to End” with
> "end-to-end".  Replace "peer to peer" with "peer-to-peer".

VC: done.


>
> Page 19, ClockPortTransportTypeAddress description, 2nd sentence: change
> "Transport" to "transport".

VC: done.


>
> Page 19, ClockPortTransportTypeAddress description, 4th sentence: insert
> "an" before "address".  Change "and" to "or".

VC: done.


>
> Page 20, ClockQualityAccuracyType description: change "section” to
> "sections".  Insert "of" before "RFC 2578", which should be placed in
> square brackets.  Change "to start with" to "starting at".

VC: done.

>
> Page 23, ClockStateType description: insert "a" before "PTP".

VC: done.
>
> Page 24, Holdover state text: change "ptp" to "PTP.  Change "FREERUN” to
> "Freerun".
>
> Page 25, ClockTimeSourceType description: change "section" to "sections”.
> Insert "of" before "RFC 2578", which should be placed in square brackets.
> Change "to start with" to "starting at".
>
> Page 26, ClockTimeSourceType reference: change "Section" to "Sections".
>
> Page 32, ptpbaseClockCurrentDSOffsetFromMaster and
> ptpbaseClockCurrentDSMeanPathDelay references: the indentation looks a bit
> off here.
>
> Page 32, ptpbaseClockCurrentDSMeanPathDelay description: change "measure”
> to "measured".
>
> Page 35, ptpbaseClockParentDSOffset description (near top of page): change
> "clocks" to "clock's".
>
> Page 35, ptpbaseClockParentDSClockPhChRate description: change "clocks” to
> "clock's".
>
> Page 41, ptpbaseClockRunningState description: insert "a" before "PTP”.
> Append a comma after "engine".
>
> Page 42, top: Rather than duplicate the state descriptions information
> here, why not merely point back to ClockStateType?  Btw, PTP engine hasn't
> been defined.
>
> Page 42, Holdover state, 1st sentence: if you do not remove this paragraph
> as suggested in the previous comment, change "ptp" to "PTP".
>
> Page 42, ptpbaseClockRunningPacketsSent description: replace "packet
> Unicast and multicast" with "unicast and multicast packets".
>
> Page 42, ptpbaseClockRunningPacketsReceived description: replace “packet
> Unicast and multicast" with "unicast and multicast packets".
>
> Page 44, ptpbaseClockTimePropertiesDSCurrentUTCOffsetValid description:
> insert "the" before "UTC".
>
> Page 45, ptpbaseClockTimePropertiesDSCurrentUTCOffset description (at top
> of page): insert "the" before "UTC".
>
> Page 49, ptpbaseClockTransDefaultDSDelay description (at top of page):
> change "shall be" to "of".  Change "If" to "if".
>
> Page 56, ptpbaseClockPortRunningTable description: should that be “a
> dataset" or "datasets".  Plain "dataset" doesn't work.
>
> Page 57, ptpbaseClockPortRunningEntry description: change "runing” to
> "running" unless Viking writing is involved.
>
> Page 58, ptpbaseClockPortRunningState description: you probably don't need
> to include the states here since they are already found in the definition
> of ClockPortState.
>
> Page 60, ptpbaseClockPortRunningEncapsulationType description: change
> "eg.” to "e.g.,".
>
> Page 60, ptpbaseClockPortRunningRxMode description: change "specifie” to
> "specifies".
>
> Page 63, ptpbaseClockPortTransDSPeerMeanPathDelay description: delete the
> parentheses and append a comma after PTP.  On the following page, change
> "value is" to "value of".
>
> Page 64, ptpbaseClockPortAssociateTable description: change "which” to
> "that".
>
> Page 75, ptpbaseMIBClockPortTransDSGroup description: does “ TransparentDS
> Dataset" contain a redundancy?
>
> Page 76, 4th paragraph, 1st sentence: delete "the" before "implementers”.
> Delete "as".
>
> Page 76, 5th paragraph, 1st sentence: change "Further" to "Furthermore".
>
>