Hi Thomas, thank you for this detailed review! https://github.com/cbor-wg/time-tag/pull/20 is the set of changes I’d like to make based on this review. Detailed responses below. Grüße, Carsten > On 2023-10-09, at 14:44, Thomas Fossati via Datatracker <noreply@xxxxxxxx> wrote: > > Reviewer: Thomas Fossati > Review result: Ready with Issues > > Due premise: I am not a time geek, so many of the subtleties in the document > were utterly inintellegible to me :-) E.g., the editor's note in §3.4 and the > details about the logarithmic scale in §3.5.2. Right. The note in 3.4 is really just an explanation for people who wonder about not having a kitchen sink of timescales. The logarithmic scale in 3.5.2 is actually rather boring math once you see Table 5 in Section 7.6.2.6 of [IEEE1588-2019], an integer scale with discrete steps at 10^-n and 2.5*10^-n. > # Summary > > The document defines three new CBOR tags (i.e., types) for expressing: > * time intervals > * duration, and > * richer (than RFC3339 and POSIX time) representation of time, > i.e.: > 1. POSIX time with higher (than microsecond) resolution > 2. explicit indication of timescale (to allow TAI alongside UTC) > > The document is generally well written. > > As per premise, I am not a time geek so I will refrain to comment on the > technical content. > >> From an artart perspective, there are a couple of things that need fixing > before publication: > * a typo in the CDDL > * a clarification about the registration procedures of the new time tag map > registry > > The rest of my comments is a bunch of editorial suggestions / nits. > > ## global > > * s/Posix/POSIX/ Thanks! > * IEEE1588-2008 (PTPv2) and IEEE1588-2019 (PTPv2.1) are often cited one > alongside the other. I am not sure why is the newer, backwards > compatible IEEE1588-2019 not sufficient? A lot of software uses components (or hints) from PTPv2. We also are based on RFC 8575, which is in turn based on PTPv2. So we think that, where we base something on full PTPv2.1, this requires a little bit of explanation. > ## abstract > > * s/It is/This document is/ OK, for the attention span of the MTV generation :-) > ## introduction > > * possibly missing ref: > * "[...] via an IANA registry ({{Section 9.2 of RFC8949}}).", or > * "[...] via an IANA registry {{IANA.cbor-tags}}." Now adding both. > * Given the preceding para, the sentence "In CBOR, one point of > extensibility is the definition of CBOR tags." is redundant Fixed. > * s/It is/This document is/ Ditto. > ## terminology > > * the whole "Where bit arithmetic is explained, [...] exponentiation." > can be dropped. Right. This is standard boilerplate that can be cut down. Maybe at some point we can write this up in such a way that it only needs to be referenced… But we forgot to explain that 10^9 (plain text rendition) is not the exclusive or of 10 and 9, added that now. > ## objectives > > * "[...] for the map inside the tag" could be made more precise: Right. I first changes this to > * adding an anchor to the Etime CDDL and referencing it from here, or > * saying instead "[...] for the `etime-detailed` map", or > * a combination of the two This. > * "[...] (Section 7.3, Section 3)." could be trimmed down to just > "(Section 7.3)" Actually, you would want to read Section 3 first, which does reference Section 7.3, so I deleted the latter. > * the bullet list with a single bullet could be converted into a normal > paragraph. Done. > ## time format > > * In general, the presentation reads a bit awkward. There are a few > redundancies here and there and fluidity suffers as a consequence. > That said, nothing impacting understanding so it's not a blocking > issue, just style. I’ll skip this; major surgery here would require more reviews. > * s/Base Time/"base time"/ This was meant to alert to “Base Time” being a specific term, but maybe that is not needed (and it is not consistently used anyway). Fixed. > * there's another bullet list with a single bullet that could be > streamlined Well, it is a continuation of the previous bullet list, so I would prefer to keep it this way. > ## key 1 > > * In "The time value indicated by the value under this key can be > further modified by other keys.", maybe s/modified/supplemented/ It is actually modified by some (-3 etc.), so I would like to keep it. > * besides, I suggest adding a backpointer to §3 to restate some of the > needed context: "... as described in {{time-format}}" We are in Section 3… I put in "As described above, " > ## key 4 and 5 > > * "This can be used to include a Decimal or Bigfloat epoch-based float > [TIME_T] in an extended time." > * "... to encode higher resolution timestamps" (or similar) Added: e.g., to achieve higher resolution or to avoid rounding errors. > ## keys -3, -6, ... > > * "More than one of these keys MUST NOT be present in one extended time > data item." > * reads a bit awkwardly. what about: "At most one of these keys MUST > be present [...]" ➔ Each extended time data item MUST NOT contain more than one of these keys. > * s/compare// Fixed. > ## clock quality > > * s/two more/the last two/ Thanks. > ## clockclass > > * "this has updated language as Table 4" > * "this uses the updated language found in Table 4" It is actually -2019 that has the updated language. Fixed. > * "(which is based on Table 5 in Section 7.6.2.4 of [IEEE1588-2008]; > this has updated language as Table 4 in Section 7.6.2.5 of > [IEEE1588-2019])." > * the referenced knowledge is inaccessible to normal humans, so trying > to follow this was quite frustrating :-) > * is it really necessary to have these two IEEE citations here? > Wouldn't RFC8575 be sufficient? As discussed above, RFC8575 is PTPv2 (-2008). I know this is painful, but you do need to get those IEEE documents to do any precision time. > * "[...] as that is the range defined there" > * can we be a bit more precise as of where? Replaced “there” by IEEE 1588 (no revision given). > ## clockaccuracy > > * Same "behind paywall" consideration as above :-( We did try to minimize the dependency more successfully than for clockclass (which is a random shopping list); the range and the formula is really what you need to know. > ## uncertainty > > * s/extended uncertainty/expanded uncertainty/ Oops. > * stray "k = 2". I guess it refers to the formula in §6.2.1 of [GUM]? > If so, add a reference. Yes. Done. > * "[...] for a duration that represents an uncertainty [...]" > * maybe add a forward reference to §4 after "duration" This sentence is “duration” in a general sense, not just Section 4 Duration. A reference to Section 4 is in the first paragraph of the section. > * "Implementations are free to reduce a guarantee [...]" > * not sure I understand what is meant by "reducing a guarantee" here, > and what we are telling implementers they should do We are giving them some liberty to maybe get some more interoperability. If a guarantee is overspecified, please reduce to what you can process. Now "reduce the information contained in an uncertainty” > ## keys -10, 10 ... > > * "Keys -10 and 10 supply supplementary information, where key 10 is > critical." > * you can safely drop this as it's completely made redundant by the > following para. As introductions often are… I’d like to keep this. > ## keys -11, 11 ... > > * "Keys -11 and 11 supply supplementary information, where key 11 is > critical." > * ditto Same. > ## IANA time tag map registry > > * "[...] and a specification reference (RFC)" > * however, the registration procedure is spec required. > Is "RFC > required" what was really meant? No. Good catch (copy pasta). > ## period format > > * In the CDDL definition of Period and clumsy-Period it looks that > `Time` should rather be `time`: time (from CDDL prelude, Appendix D of RFC 8610) is tag 1. Here we mean Etime. Thanks for catching this! > ``` > Period = #6.1003([ > start: ~Time / null > end: ~Time / null > ? duration: ~Duration / null > ]) > > clumsy-Period = #6.1003([ > (start: ~Time, > ((end: ~Time, > ? duration: null) // > (end: null, > duration: ~Duration))) // > (start: null, > end: ~Time, > duration: ~Duration) > ``` > > ## references > > * in the note to the IEEE1588-2019 entry: s/in way that/in a way that/ Fixed. Thanks again for all the improvements... -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
