Agree with Tom here, if it is not a big issue I'd prefer to keep them in capital letter as they show up in capital letters in all the draft/RFC and IANA registries.
BR
Daniele
> -----Original Message-----
> From: t.petch <ietfc@xxxxxxxxxxxxx>
> Sent: martedì 12 giugno 2018 06:56
> To: Jonas Ahlberg <jonas.ahlberg@xxxxxxxxxxxx>; Jan Lindblad <janl@tail-
> f.com>
> Cc: yang-doctors@xxxxxxxx; ccamp@xxxxxxxx; Yemin (Amy)
> <amy.yemin@xxxxxxxxxx>; draft-ietf-ccamp-mw-yang.all@xxxxxxxx; ietf
> <ietf@xxxxxxxx>
> Subject: Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-
> yang-05 capitals
>
> Top posting and tweaking the subject line slightly since I am picking up one
> minor point only
>
> > There are a couple of identities with capitals. Consider changing to
> all lowercase; that is the YANG convention.
> >
> > identity E1 {
> > identity STM-1 {
> > [Amy] will fix it.
> > [Jonas] OK.
>
> Disagree! If I see e1 I will probably think exponent - if I see E1, I know exactly
> what is intended, in a wide range of contexts. YANG guidelines allows
> "Upper-case characters, the period character,
> and the underscore character MAY be used if the identifier represents
> a well-known value that uses these characters. "
> and I think that E1 and STM-1 fall within that MAY.
>
> I see
> identity client-signal-OC3_STM1
> in
> draft-ietf-ccamp-otn-tunnel-model-01
> and
> identity STM-1 {
> in
> draft-fioccola-ccamp-l1csm-yang-01
>
> so I think that CCAMP needs to decide between STM-1 and STM1 - the
> former works for me - and then tell the YANG doctors that that is how it
> is:-) YMMV
>
> Tom Petch
>
> ----- Original Message -----
> From: "Jonas Ahlberg" <jonas.ahlberg@xxxxxxxxxxxx>
> To: "Jan Lindblad" <janl@xxxxxxxxxx>
>
>
> > Hi Jan & Amy,
> > See comments & questions in green [Jonas2].
> > Regards
> > JonasA
> >
> > From: Jan Lindblad <janl@xxxxxxxxxx>
> > Sent: den 11 juni 2018 15:29
>
> >
> > Jonas,
> >
> > Thank you for your review and valuable comments.
> > I’ve not responded to the latest mail since I have added comments to
> some of the items removed in that mail.
> > I’ve copied and pasted your comments into this mail instead, in order
> to make the thread of comments complete.
> >
> > I’ve added my comments in green [Jonas].
> >
> > [janl2] Very good. Sorry for deleting text prematurely ;-)
> >
> > I would like to ask CCAMP for support with the question about if the
> protection group is likely to be reused elsewhere or if it can be merged with
> the microwave module again.
> >
> > Regards
> > JonasA
> >
> > From: Yemin (Amy)
> <amy.yemin@xxxxxxxxxx<mailto:amy.yemin@xxxxxxxxxx>>
> > Sent: den 8 juni 2018 11:37
> >
> > Hi Jan,
> >
> > Thanks very much for the comments. I found them are quiet useful to
> improve the models.
> > Please see my reply below in blue.
> > The draft co-authors may add more reply to your comments.
> >
> > BR,
> > Amy
> > -----Original Message-----
> > From: CCAMP [mailto:ccamp-bounces@xxxxxxxx] On Behalf Of Jan Lindblad
> > Sent: Thursday, June 07, 2018 6:18 PM
> >
> > Reviewer: Jan Lindblad
> > Review result: Ready with Issues
> >
> > YD review of draft-ietf-ccamp-mw-yang
> >
> > I have now reviewed the YANG modules and corresponding examples of
> the -05 version of this draft. I have not read much of the RFC text, so I can't
> vouch for how well the text aligns with the model. I find the proposed
> modules in good shape. Most of my comments below are alternate ways of
> modeling something that the wg may consider, or a few things where I
> propose a better option for something that would be acceptable even as it is.
> There is a single issue that must be fixed IMO, see #8 below.
> >
> > Let's start with module ietf-microwave-types.
> >
> > #1) Consider adding structure to related identities
> >
> > This module consists mostly of a long list of identities based off of
> coding-modulation. If it makes sense that in the future someone might be
> interested in doing something with all qam-4096 based identities or all -
> strong identities, say, it may make sense to model the identities as based on
> each other in a tree style. E.g.
> >
> > identity qam-4096 {
> > base coding-modulation;
> > description
> > "4096 QAM coding and modulation scheme.";
> > }
> >
> > identity qam-4096-strong {
> > base qam-4096;
> > description
> > "4096 QAM strong coding and modulation scheme.";
> > }
> >
> > identity qam-4096-light {
> > base qam-4096;
> > description
> > "4096 QAM light coding and modulation scheme.";
> > }
> >
> > Or even go to "multiple inheritance" with multiple bases for
> identities, e.g.
> > for qam-4096 and strong. This would allow future applications to
> filter the identities on such criteria. Just a thought.
> > [Amy] change to base qam-4096. Multiple bases seem not applicable
> here.
> > [Jonas] I agree.
> >
> > #2) Convention to use all lowercase in YANG symbols
> >
> > There are a couple of identities with capitals. Consider changing to
> all lowercase; that is the YANG convention.
> >
> > identity E1 {
> > identity STM-1 {
> > [Amy] will fix it.
> > [Jonas] OK.
> >
> > Next, let's look at module ietf-interface-protection.
> >
> > I can't say I understand exactly why this is a separate module. It
> publishes a single grouping, which is required by ietf-microwave-radio-link,
> and as far as I understand would probably never be used anywhere else.
> When the grouping is used a single time in ietf-microwave-radio-link, it is
> immediately refined.
> > Would probably reduce the clutter by merging the two modules and
> resolving the refine.
> > [Amy] They were in one model. During the WG discussion, comment was
> raised that the interface protection function could be generic and be used by
> other technologies in future, so we split the models.
> > I’m open to discuss about this.
> > [janl] Ok, I didn't quite see how the grouping would be used anywhere
> else, but if it indeed is usable elsewhere, having it in a separate module isn't
> a bad idea.
> > [Jonas] I’m in favor of reverting back to one single module gain,
> since it will simplify the model as you mention above. The argument for
> breaking it out into a separate module is that it can be used elsewhere.
> I’m doubtful if it ever will be, but here I think we need input from the rest of
> the community (at least from the CCAMP WG).
> >
> >
> > #3) Config true leaf name status
> >
> > I find it counter-intuitive that a leaf called status (or state) is a
> configuration item. I had to re-read the model several times to get my head
> around the fact that this is indeed meant to be config true.
> Perhaps consider a name change?
> >
> > leaf status {
> > [Amy] status should be config false.
> > [Jonas] Correct. Our mistake.
> >
> > #4) Action external-commands
> >
> > There is a single action called external-commands (even in plural). It
> takes a single argument, which is the name of the operation to execute.
> No output. To me, a more natural modeling would be to make each of the
> external commands an action, over time possibly with different input and
> output.
> > [Amy] add output to describe the action result (success, fail,
> inprogress). But prefer to use one action.
> > Change the name to external-command.
> > [janl] Hmm, I don't quite understand your preference for a single
> action. I see how this choice closes doors, but no real benefit. This is no big
> deal, though, what you have works. Just feels less evolvable for no reason.
> > [Jonas] I’m not quite sure I’ve understood what you propose. Do you
> mean that we should replace the single action statement and the list of
> identifiers with several action statements:
> > action manual-switch-working;
> > action manual-switch-protection;
> > …
> > action clear;
> > The reason for using identities was that we wanted to make it easy to
> extend the module with additional, potentially vendor specific, commands by
> augmentation.
> > But maybe there are other ways to handle that in case of using the
> alternative approach you suggest.
> >
> > [janl2] Yes, exactly. Separate actions like this has the advantage
> that they can take (different) input and output parameters, and that they
> can be access controlled separately using NACM. This way different
> operators could have/not have access to different operations. Future
> standard or vendor modules could still augment whatever additional actions
> they like to the same location in the tree.
> > [Jonas2] OK I understand and I see the advantage of this approach.
> Amy, should we consider change to this or are there other benefits with the
> original approach?
> >
> > Finally, we have module ietf-microwave-radio-link.
> >
> > #5) Use derived-from when comparing identities
> >
> > It's more future-proof and more likely to be interoperable if you use
> proper XPATH functions to determine kinship than using plain equality
> >
> > augment "/if:interfaces/if:interface" {
> > when "if:type = 'mw-types:radio-link-terminal'";
> >
> > is better written as
> >
> > augment "/if:interfaces/if:interface" {
> > when "derived-from-or-self(if:type,
> 'mw-types:radio-link-terminal')";
> >
> > This allows future sub-typing (sub-classing) of radio-link-terminal,
> i.e. new identities that are based on radio-link-terminal to reflect some
> special kind of RLT. It also improves chances of interoperability.
> > [Amy] will adopt this.
> > [Jonas] Agree.
> >
> > #6) Blank id reasonable?
> >
> > Leafs that function as an id usually do not have defaults. Does a
> blank id make sense here? If it does, maybe it would make more sense to
> leave it without a default and explain what happens if not set in the
> description instead? Or mark it as mandatory if it has to be set.
> >
> > leaf id {
> > type string;
> > default "";
> > [Amy] remove default. Add mandatory.
> > [Jonas] I think that the name of the attribute could be misleading. It
> ’s not a formal id of the terminal, it is a text string that is used by an optional
> system feature in a far-end RLT (on the opposite end of the microwave link)
> to verify that the correct two RLTs (far-end and
> near-end) are connected.
> > I don’t think we should make it mandatory. I would suggest to leave it
> without default and clarify the consequences in the description.
> >
> > #7) Use derived-from when comparing identities (again)
> >
> > leaf-list carrier-terminations {
> > type if:interface-ref;
> > must "/if:interfaces/if:interface[if:name = current()]"
> > + "/if:type = 'mw-types:carrier-termination'" {
> >
> > is better written as
> >
> > must "derived-from-or-self(/if:interfaces/if:interface[if:name =
> > current()]"
> > + "/if:type, 'mw-types:carrier-termination')" {
> >
> > It is possible to write this in a more compact way, but there's
> nothing wrong with the above.
> >
> > must "derived-from-or-self(deref(current())/.."
> > + "/if:type, 'mw-types:carrier-termination')" { [Amy] will
> > adopt this.
> > [Jonas] Agree.
> >
> > #8) Badly broken frequency duplex config
> >
> > If you read the descriptions in these related leafs:
> >
> > leaf tx-frequency {
> > type uint32;
> > units "kHz";
> > mandatory true;
> > description
> > "Selected transmitter frequency.";
> > }
> > leaf rx-frequency {
> > type uint32;
> > units "kHz";
> > description
> > "Selected receiver frequency.
> > Overrides existing value in duplex-distance.
> > Calculated from tx-frequency and duplex-distance if
> > only duplex-distance is configured.
> > Must match duplex-distance if both leaves are
> > configured in a single operation.";
> > }
> >
> > leaf duplex-distance {
> > type uint32;
> > units "kHz";
> > description
> > "Distance between Tx & Rx frequencies.
> > Used to calculate rx-frequency when
> > rx-frequency is not specifically configured.
> > Overrides existing value in rx-frequency.
> > Calculated from tx-frequency and rx-frequency if only
> > rx-frequency is configured.
> > Must match rx-frequency if both leaves are configured
> > in a single operation.";
> > }
> >
> > It appears that the author intends the system to fill in the value for
> one of these leaves based on the value set for the other. This is a big no-no.
> A system should never alter its own configuration, or automation flows
> (which is the whole purpose with YANG and NETCONF, remember) will
> break. Also, the validity of the configuration should not depend on how
> many operations are used to inject it.
> >
> > I find this a serious flaw that must be fixed before the module can be
> released.
> >
> > I propose fixing it like this:
> >
> > leaf tx-frequency {
> > type uint32;
> > units "kHz";
> > mandatory true;
> > description
> > "Selected transmitter frequency.";
> > }
> > choice freq-or-distance {
> > leaf rx-frequency {
> > type uint32;
> > units "kHz";
> > description
> > "Selected receiver frequency."
> > }
> > leaf duplex-distance {
> > type uint32;
> > units "kHz";
> > description
> > "Distance between Tx & Rx frequencies."
> > }
> > }
> >
> > If you would like to have read-only computed values accessible in the
> model, maybe you could add:
> >
> > leaf actual-rx-frequency {
> > type uint32;
> > units "kHz";
> > description
> > "Computed receiver frequency."
> > config false;
> > }
> > leaf actual-duplex-distance {
> > type uint32;
> > units "kHz";
> > description
> > "Computed distance between Tx & Rx frequencies."
> > config false;
> > }
> >
> > Many other ways of doing this properly are also possible. Let me know
> if you'd like to discuss options.
> > [Amy] I think choice is a good way to model those leafs. I suggest to
> use it.
> > That’s the real value of YANG doctors! Thanks.
> > [Jonas] Thanks for your help. We have discussed this a lot without
> finding a good solution. I agree that we should use it.
> > The only question I have is the name of the attributes. Should the
> attribute used for configuration or the attribute showing the resulting be
> called rx-frequency?
> > config-rx-frequency & rx-frequency v.s. rx-frequency &
> actual-rx-frequency
> >
> > [janl2] I used actual-rx-frequency because you had some other leafs
> called actual-...
> >
> > Yet another possibility could be to model it like this (pseudo YANG):
> >
> > leaf tx { ... }
> > choice freq-or-dist {
> > container by-frequency {
> > leaf rx { ... }
> > leaf dist { config false; }
> > }
> > container by-distance {
> > leaf dist { ... }
> > leaf rx { config false; }
> > }
> > }
> >
> > I.e. three leaves with the same names, just different paths (since the
> container names differ) and different configness. I think my initial proposal is
> a little cleaner, tho, which is why I went with that one earlier. Or you could
> move the actual-... leaves into the choice, so that only the one that's
> meaningful exists.
> >
> > [Jonas2] I think that your first proposal has a benefit of always
> providing access to the actual configuration through the same leafs (and
> path) (tx-frequency, actual-rx-frequency, actual-duplex-distance,
> independent of how they have been configured.
> > One use-case we have discussed is that an operator uses
> duplex-distance for the initial configuration of the rx-frequency and then in a
> second step fine tuning it by setting the rx-frequency directly. Would that
> work with a choice statement, i.e. that you first choose duplex-distance and
> then in a subsequent operation overrides that and choose rx-frequency?
> >
> > #9) Check that lower threshold is less than upper threshold
> >
> > Would it make sense to add a must statement checking that the lower
> threshold is less than (or equal?) to the upper threshold?
> >
> > leaf atpc-lower-threshold {
> > when "../power-mode = 'atpc'";
> > type power {
> > range "-99..-30";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "The lower threshold for the input power at far-end
> > used in the ATPC mode.";
> > reference "ETSI EN 302 217-1";
> > }
> > leaf atpc-upper-threshold {
> > [Amy] can add the must statement.
> > [Jonas] Good!
> >
> > #10) Choice more convenient
> >
> > There are a few leafs that act as discriminators for when clauses in
> other leafs. Such constructs might be a little smoother when modeled as a
> choice instead. I'll take one and show as an example. This power-mode
> construct:
> >
> > leaf power-mode {
> > type enumeration {
> > enum rtpc {
> > description
> > "Remote Transmit Power Control (RTPC).";
> > reference "ETSI EN 302 217-1";
> > }
> >
> > enum atpc {
> > description
> > "Automatic Transmit Power Control (ATPC).";
> > reference "ETSI EN 302 217-1";
> > }
> > }
> > mandatory true;
> > description
> > "A choice of Remote Transmit Power Control (RTPC)
> > or Automatic Transmit Power Control (ATPC).";
> > }
> >
> > leaf maximum-nominal-power {
> > type power {
> > range "-99..40";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "Selected output power in RTPC mode and selected
> > maximum output power in ATPC mode. Minimum output
> > power in ATPC mode is the same as the system
> > capability, available-min-output-power.";
> > reference "ETSI EN 302 217-1";
> > }
> >
> > leaf atpc-lower-threshold {
> > when "../power-mode = 'atpc'";
> > type power {
> > range "-99..-30";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "The lower threshold for the input power at far-end
> > used in the ATPC mode.";
> > reference "ETSI EN 302 217-1";
> > }
> > leaf atpc-upper-threshold {
> > when "../power-mode = 'atpc'";
> > type power {
> > range "-99..-30";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "The upper threshold for the input power at far-end
> > used in the ATPC mode.";
> > reference "ETSI EN 302 217-1";
> > }
> >
> > could be modeled as:
> >
> > choice power-mode {
> > container rtpc {
> > description
> > "Remote Transmit Power Control (RTPC).";
> > reference "ETSI EN 302 217-1";
> > leaf maximum-nominal-power {
> > type power {
> > range "-99..40";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "Selected output power.";
> > reference "ETSI EN 302 217-1";
> > }
> > }
> > container atpc {
> > description
> > "Automatic Transmit Power Control (ATPC).";
> > reference "ETSI EN 302 217-1";
> >
> > leaf maximum-nominal-power {
> > type power {
> > range "-99..40";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "Selected maximum output power. Minimum output
> > power is the same as the system
> > capability, available-min-output-power.";
> > reference "ETSI EN 302 217-1";
> > }
> >
> > leaf atpc-lower-threshold {
> > type power {
> > range "-99..-30";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "The lower threshold for the input power at far-end
> > used in the ATPC mode.";
> > reference "ETSI EN 302 217-1";
> > }
> > leaf atpc-upper-threshold {
> > type power {
> > range "-99..-30";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "The upper threshold for the input power at far-end
> > used in the ATPC mode.";
> > reference "ETSI EN 302 217-1";
> > }
> > }
> > mandatory true;
> > description
> > "A choice of Remote Transmit Power Control (RTPC)
> > or Automatic Transmit Power Control (ATPC)."; }
> >
> > [Amy] Choice is a better way. Is it possible to further refine your
> proposal?
> > Since maximum-nominal-power will be used by both RTPC and ATPC, how
> about to move it out of the choice, then use maximum-nominal-power in the
> choice? Like this:
> > leaf maximum-nominal-power {
> > type power {
> > range "-99..40";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "Selected maximum output power. Minimum output
> > power is the same as the system
> > capability, available-min-output-power.";
> > reference "ETSI EN 302 217-1";
> > }
> >
> > choice power-mode {
> > container rtpc {
> > description
> > "Remote Transmit Power Control (RTPC).";
> > reference "ETSI EN 302 217-1";
> >
> > use maximum-nominal-power;
> > }
> > container atpc {
> > description
> > "Automatic Transmit Power Control (ATPC).";
> > reference "ETSI EN 302 217-1";
> >
> > use maximum-nominal-power;
> >
> > leaf atpc-lower-threshold {
> > type power {
> > range "-99..-30";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "The lower threshold for the input power at far-end
> > used in the ATPC mode.";
> > reference "ETSI EN 302 217-1";
> > }
> > leaf atpc-upper-threshold {
> > type power {
> > range "-99..-30";
> > }
> > units "dBm";
> > mandatory true;
> > description
> > "The upper threshold for the input power at far-end
> > used in the ATPC mode.";
> > reference "ETSI EN 302 217-1";
> > }
> > }
> > mandatory true;
> > description
> > "A choice of Remote Transmit Power Control (RTPC)
> > or Automatic Transmit Power Control (ATPC).";
> > }
> >
> > [janl] Yes, this is certainly very reasonable. I placed the
> maximum-nominal-power inside the choice because the description string
> was giving the leaf different interpretations in each case. In that case I felt it
> better to have separate leaves with clear descriptions. If you feel the leaf can
> be shared between the options with a clear definition of its meaning, this is
> reasonable.
> > [Jonas] One purpose of the mode attribute was to show if the
> carrier-termination was configured for RTPC or ATPC, but I guess the same
> can be achieved by looking for which container, rtpc or atpc, has been
> instantiated.
> >
> > [janl2] Yes, that's the idea.
> > [Jonas2]. Good!
> >
> > I suggest that we go for Jan’s proposal since the attribute has
> different meaning in the two cases. One purpose of the
> >
> > #11) Choice more convenient (again)
> >
> > Same thing again for
> >
> > leaf coding-modulation-mode {
> > [Amy] will update it.
> > [Jonas] OK, see above.
> >
> > #12) Unusual exponential notation
> >
> > Do you really mean 10e-9 (=10*10^-9 =10^-8), or do you mean the more
> traditional notation 1e-9?
> >
> > leaf ber-alarm-threshold {
> > type enumeration {
> > enum "10e-9" {
> > [Amy] should be 1e-9. Need to confirm with the co-authors.
> > [Jonas] Yes, we mean 10^-9, 10^-8, …, 10^-1.
> >
> > [janl2] A perpahs simpler way to model this would be to only keep the
> part that contains information, i.e. as
> >
> > leaf ber-alarm-threshold {
> > type int8 {
> > range -9..-1;
> > }
> > default -6;
> > description "Specification of at which BER as a power of 10 an alarm
> should be raised. 10^-6 is the default.";
> > reference "ETSI EN 302 217-1";
> > }
> >
> > #13) Separate module with grouping, used a single time with refine
> >
> > Module ietf-interface-protection defines a grouping
> (protection-groups), which is used a single time, yet is refined when it is used
> below. As noted before comment #3, I find this way of laying out the YANG
> unnecessarily hard to read and understand, for no clear benefit.
> >
> > container radio-link-protection-groups { ....
> > uses ifprot:protection-groups {
> >
> > refine protection-group/members {
> > must "/if:interfaces/if:interface[if:name = current()]"
> > + "/if:type = 'mw-types:carrier-termination'" {
> >
> > Also, as noted in comment #7, the must statement is better written
> using derived-from-or-self. This applies regardless of the current refine
> statement is kept, or if the must statement moves to the actual leaf-list it
> applies to.
> > [Amy] Will update the must statement.
> > [Jonas] If I remember correctly, I think it was done with the ambition
> that it should be made generic so that other interface technologies could use
> the same protection module. In case, we revert back to one single module I
> suggest that we also simply the structure as Jan suggests.
> >
> > Appendix A.1 & A.2
> >
> > Besides the actual YANG modules, there are also a couple of examples
> in Appendix A.1 and A.2. I tried to use them and uncovered a couple of
> issues.
> >
> > #14) Config false leaf in config example
> >
> > Both examples list
> >
> > "tx-oper-status": on
> >
> > This is a config false item, which could never be part of a
> configuration message (and it also lacks comma at the end).
> > [Amy] will remove it.
> > [Jonas] OK
> >
> > #15) Wrong type in example
> >
> > Both examples list
> >
> > "coding-modulation-mode": 0,
> >
> > 0 not a legal value, should be "single" to match the rest of the
> example data.
> > [Amy] will fix it.
> > [Jonas] OK
> >
> > #16) Missing mandatory parameter
> >
> > Both examples lacks leaf maximum-nominal-power, which is mandatory
> according to the YANG, so the transaction fails to validate.
> > [Amy] will add it.
> > [Jonas] OK
> >
> > #17) Examples perhaps a tad basic
> >
> > The examples are demonstrating only a small part of the module
> functionality. A bigger example, e.g. including xpic with interface pointers
> might be useful.
> >
> > Feel free to reach out to me to discuss any of this. Thank you.
> > [Amy] will add additional example.
> > [Jonas] Good suggestion. Amy, let’s what configuration to show.
> >
> >
> > Best Regards,
> > /jan
> >
> >
