Subject: Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

From: Jan Lindblad <[email-masked]>
Date: Thu, 07 Jun 2018 03:18:29 -0700

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.

#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 {

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.

#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 {

#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.

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.

#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 "";

#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')" {

#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.

#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  {

#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).";
    }

#11) Choice more convenient (again)

Same thing again for

    leaf coding-modulation-mode {

#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" {

#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.

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).

#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.

#16) Missing mandatory parameter

Both examples lacks leaf maximum-nominal-power, which is mandatory according to
the YANG, so the transaction fails to validate.

#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.
/jan

---

From: "Yemin (Amy)" <[email-masked]>
Date: Fri, 8 Jun 2018 09:36:46 +0000

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:[email-masked]] On Behalf Of Jan Lindblad
Sent: Thursday, June 07, 2018 6:18 PM
To: [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Reviewer: Jan Lindblad

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.

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

  }

    base qam-4096;

    description

  }

    base qam-4096;

    description

  }

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.

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.

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.

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.

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.

It's more future-proof and more likely to be interoperable if you use proper XPATH functions to determine kinship than using plain equality

is better written as

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.

#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.

      type if:interface-ref;

is better written as

      current()]"

It is possible to write this in a more compact way, but there's nothing wrong with the above.

[Amy] will adopt this.

    leaf tx-frequency {

      type uint32;

      units "kHz";

      mandatory true;

      description

    }

   leaf rx-frequency {

      type uint32;

      units "kHz";

      description

    }

    leaf duplex-distance {

      type uint32;

      units "kHz";

      description

         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.

    leaf tx-frequency {

      type uint32;

      units "kHz";

      mandatory true;

      description

    }

      leaf rx-frequency {

        type uint32;

        units "kHz";

        description

      }

      leaf duplex-distance {

        type uint32;

        units "kHz";

        description

      }

    }

If you would like to have read-only computed values accessible in the model, maybe you could add:

      type uint32;

      units "kHz";

      description

      config false;

    }

      type uint32;

      units "kHz";

      description

      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.

Would it make sense to add a must statement checking that the lower threshold is less than (or equal?) to the upper threshold?

      type power {

        range "-99..-30";

      }

      units "dBm";

      mandatory true;

      description

         used in the ATPC mode.";

    }

    leaf atpc-upper-threshold  {
[Amy] can add the must statement.

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

        }

        enum atpc {

          description

        }

      }

      mandatory true;

      description

    }

      type power {

        range "-99..40";

      }

      units "dBm";

      mandatory true;

      description

    }

      type power {

        range "-99..-30";

      }

      units "dBm";

      mandatory true;

      description

         used in the ATPC mode.";

    }

      type power {

        range "-99..-30";

      }

      units "dBm";

      mandatory true;

      description

         used in the ATPC mode.";

    }

could be modeled as:

    choice power-mode {

      container rtpc {

        description

          type power {

            range "-99..40";

          }

          units "dBm";

          mandatory true;

          description

        }

      }

      container atpc {

        description

          type power {

            range "-99..40";

          }

          units "dBm";

          mandatory true;

          description

        }

          type power {

            range "-99..-30";

          }

          units "dBm";

          mandatory true;

          description

             used in the ATPC mode.";

        }

          type power {

            range "-99..-30";

          }

          units "dBm";

          mandatory true;

          description

             used in the ATPC mode.";

        }

      }

      mandatory true;

      description

}

[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

        use maximum-nominal-power;
      }
      container atpc {
        description
          "Automatic Transmit Power Control (ATPC).";
        reference "ETSI EN 302 217-1";

        use maximum-nominal-power;

Same thing again for

[Amy] will update it.

Do you really mean 10e-9 (=10*10^-9 =10^-8), or do you mean the more traditional notation 1e-9?

        type enumeration {

          enum "10e-9" {

[Amy] should be 1e-9. Need to confirm with the co-authors.

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 { ....

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.

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.

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.

Both examples list

[Amy] will fix it.

Both examples lacks leaf maximum-nominal-power, which is mandatory according to the YANG, so the transaction fails to validate.

[Amy] will add it.

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.

[Amy] will add additional example.

/jan

_______________________________________________

CCAMP mailing list

[email-masked]<mailto:[email-masked]>

https://www.ietf.org/mailman/listinfo/ccamp

---

From: Jan Lindblad <[email-masked]>
Date: Mon, 11 Jun 2018 10:55:04 +0200

Yemin,

> The draft co-authors may add more reply to your comments.

Very good. I added some replies marked [janl] below.

> 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. 

> 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.

> That’s the real value of YANG doctors! Thanks.

[janl] ;-)
 
...
> 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?

[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.

Best Regards,
/jan

---

From: Jonas Ahlberg <[email-masked]>
Date: Mon, 11 Jun 2018 12:37:20 +0000

Hi Jan,

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].

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) <[email-masked]>
Sent: den 8 juni 2018 11:37
To: Jan Lindblad <[email-masked]>; [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: RE: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Hi Jan,

BR,

Amy

-----Original Message-----
From: CCAMP [mailto:[email-masked]] On Behalf Of Jan Lindblad
Sent: Thursday, June 07, 2018 6:18 PM
To: [email-masked]<mailto:[email-masked]>
Cc: [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>
Subject: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Reviewer: Jan Lindblad

  identity qam-4096 {

    base coding-modulation;

    description

  }

    base qam-4096;

    description

  }

    base qam-4096;

    description

  }

[Jonas] I agree.

  identity E1 {

  identity STM-1 {
[Amy] will fix it.
[Jonas] OK.

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).

      leaf status {

[Amy] status should be config false.
[Jonas] Correct. Our mistake.

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.

is better written as

[Amy] will adopt this.

[Jonas] Agree.

#6) Blank id reasonable?

    leaf id {

      type string;

      default "";

[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.

      type if:interface-ref;

is better written as

      current()]"

[Amy] will adopt this.

[Jonas] Agree.

    leaf tx-frequency {

      type uint32;

      units "kHz";

      mandatory true;

      description

    }

   leaf rx-frequency {

      type uint32;

      units "kHz";

      description

    }

    leaf duplex-distance {

      type uint32;

      units "kHz";

      description

         in a single operation.";

    }

    leaf tx-frequency {

      type uint32;

      units "kHz";

      mandatory true;

      description

    }

      leaf rx-frequency {

        type uint32;

        units "kHz";

        description

      }

      leaf duplex-distance {

        type uint32;

        units "kHz";

        description

      }

    }

      type uint32;

      units "kHz";

      description

      config false;

    }

      type uint32;

      units "kHz";

      description

      config false;

    }

[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

      type power {

        range "-99..-30";

      }

      units "dBm";

      mandatory true;

      description

         used in the ATPC mode.";

    }

[Jonas] Good!

    leaf power-mode {

      type enumeration {

        enum rtpc {

          description

        }

        enum atpc {

          description

        }

      }

      mandatory true;

      description

    }

      type power {

        range "-99..40";

      }

      units "dBm";

      mandatory true;

      description

    }

      type power {

        range "-99..-30";

      }

      units "dBm";

      mandatory true;

      description

         used in the ATPC mode.";

    }

      type power {

        range "-99..-30";

      }

      units "dBm";

      mandatory true;

      description

         used in the ATPC mode.";

    }

could be modeled as:

    choice power-mode {

      container rtpc {

        description

          type power {

            range "-99..40";

          }

          units "dBm";

          mandatory true;

          description

        }

      }

      container atpc {

        description

          type power {

            range "-99..40";

          }

          units "dBm";

          mandatory true;

          description

        }

          type power {

            range "-99..-30";

          }

          units "dBm";

          mandatory true;

          description

             used in the ATPC mode.";

        }

          type power {

            range "-99..-30";

          }

          units "dBm";

          mandatory true;

          description

             used in the ATPC mode.";

        }

      }

      mandatory true;

      description

}

    choice power-mode {
      container rtpc {
        description

[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.
I suggest that we go for Jan's proposal since the attribute has different meaning in the two cases. One purpose of the

Same thing again for

[Amy] will update it.

[Jonas] OK, see above.

        type enumeration {

          enum "10e-9" {

[Jonas] Yes, we mean 10^-9, 10^-8, ..., 10^-1.

[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

Both examples list

        "tx-oper-status": on

[Amy] will remove it.

[Jonas] OK

Both examples list

[Amy] will fix it.

[Jonas] OK

[Amy] will add it.

[Jonas] OK

[Jonas] Good suggestion. Amy, let's what configuration to show.

/jan

CCAMP mailing list

---

From: Jan Lindblad <[email-masked]>
Date: Mon, 11 Jun 2018 15:29:20 +0200

Jonas,

> 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 ;-)
 
> From: Yemin (Amy) <[email-masked] <mailto:[email-masked]>> 
> To: Jan Lindblad <[email-masked] <mailto:[email-masked]>>; [email-masked] <mailto:[email-masked]>
> Cc: [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>
> From: CCAMP [mailto:[email-masked] <mailto:[email-masked]>] On Behalf Of Jan Lindblad
> To: [email-masked] <mailto:[email-masked]>
> Cc: [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>
> [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).
>  
>  
> [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:
> …

[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.
 
> [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.
>  

[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.
 
> [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,…

[janl2] Yes, that's the idea.

> I suggest that we go for Jan’s proposal since the attribute has different meaning in the two cases. One purpose of the
>  
> [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";
}
 
> [Jonas] Good suggestion. Amy, let’s what configuration to show.
>  

Best Regards,
/jan

---

From: Jonas Ahlberg <[email-masked]>
Date: Tue, 12 Jun 2018 07:15:33 +0000

Hi Jan & Amy,
See comments & questions in green [Jonas2].
Regards
JonasA

From: Jan Lindblad <[email-masked]>
Sent: den 11 juni 2018 15:29
To: Jonas Ahlberg <[email-masked]>
Cc: Yemin (Amy) <[email-masked]>; [email-masked]; [email-masked]; [email-masked]; [email-masked]
Subject: Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Jonas,

Regards
JonasA

From: Yemin (Amy) <[email-masked]<mailto:[email-masked]>>
Sent: den 8 juni 2018 11:37
To: Jan Lindblad <[email-masked]<mailto:[email-masked]>>; [email-masked]<mailto:[email-masked]>
Cc: [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>
Subject: RE: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Hi Jan,

BR,
Amy
-----Original Message-----
From: CCAMP [mailto:[email-masked]] On Behalf Of Jan Lindblad
Sent: Thursday, June 07, 2018 6:18 PM
To: [email-masked]<mailto:[email-masked]>
Cc: [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>
Subject: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Reviewer: Jan Lindblad

  identity qam-4096 {
    base coding-modulation;
    description

  identity E1 {
  identity STM-1 {
[Amy] will fix it.
[Jonas] OK.

      leaf status {

[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?

is better written as

#6) Blank id reasonable?

    leaf id {
      type string;
      default "";

is better written as

    leaf tx-frequency {
      type uint32;
      units "kHz";
      mandatory true;
      description

    leaf duplex-distance {
      type uint32;
      units "kHz";
      description

    leaf tx-frequency {
      type uint32;
      units "kHz";
      mandatory true;
      description

leaf tx { ... }
choice freq-or-dist {
  container by-frequency {
    leaf rx { ... }

[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?

    leaf power-mode {
      type enumeration {
        enum rtpc {
          description

        enum atpc {
          description

could be modeled as:

    choice power-mode {
      container rtpc {
        description

    choice power-mode {
      container rtpc {
        description

[Jonas2]. Good!

Same thing again for

Appendix A.1 & A.2

Both examples list

        "tx-oper-status": on

Both examples list

Best Regards,
/jan

---

From: Jan Lindblad <[email-masked]>
Date: Tue, 12 Jun 2018 09:34:10 +0200

Jonas,

Comments below marked [janl3].

> From: Jan Lindblad <[email-masked] <mailto:[email-masked]>> 
> To: Jonas Ahlberg <[email-masked] <mailto:[email-masked]>>
> Cc: Yemin (Amy) <[email-masked] <mailto:[email-masked]>>; [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>

[janl3] Yes, I agree. I find this easier to read and use.

> 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 direct…

[janl3] Well, yes. The operator/manager can choose whichever method he likes and switch from one to the other at any time. If a value for distance is configured, the rx-frequency is implicitly deleted, and vice versa. That's how a choice works, something like radio buttons (but there could be several elements inside each "button").

One question there: the distance is currently unsigned, so how will the system know if the rx freq is dist kHz above or below the tx freq? Or is this obvious, the tx is always lower or something? There's no validation for any such rule in the YANG for sure.

Maybe we should also make the choice mandatory. I think I left it optional in my suggestion, and I'm not sure that makes sense. So:

---

From: "t.petch" <[email-masked]>
Date: Tue, 12 Jun 2018 12:55:35 +0100

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.
> [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" <[email-masked]>
To: "Jan Lindblad" <[email-masked]>

> 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.
> 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.
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.
> for qam-4096 and strong. This would allow future applications to
filter the identities on such criteria. Just a thought.
> There are a couple of identities with capitals. Consider changing to
all lowercase; that is the YANG convention.
> 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.
> [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.
> [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).
> 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?
> 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.
> [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:
> 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.
> [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?
> It's more future-proof and more likely to be interoperable if you use
proper XPATH functions to determine kinship than using plain equality
> 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.
> [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.
> 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.
> [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?
> 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?
> Would it make sense to add a must statement checking that the lower
threshold is less than (or equal?) to the upper threshold?
> 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:
> 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:
> [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.
> I suggest that we go for Jan’s proposal since the attribute has
different meaning in the two cases. One purpose of the
>   description "Specification of at which BER as a power of 10 an alarm
should be raised. 10^-6 is the default.";
> 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.
> 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.
> [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.
> This is a config false item, which could never be part of a
configuration message (and it also lacks comma at the end).
> Both examples lacks leaf maximum-nominal-power, which is mandatory
according to the YANG, so the transaction fails to validate.
> 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.

---

From: Daniele Ceccarelli <[email-masked]>
Date: Tue, 12 Jun 2018 15:54:24 +0000

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  

> From: t.petch <[email-masked]>
> Sent: martedì 12 giugno 2018 06:56
> To: Jonas Ahlberg <[email-masked]>; Jan Lindblad <janl@tail-
> f.com>
> Cc: [email-masked]; [email-masked]; Yemin (Amy)
> <[email-masked]>; [email-masked]; ietf
> <[email-masked]>
> 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
> 
> 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
> protection group is likely to be reused elsewhere or if it can be merged with
> the microwave module again.
> >
> > Regards
> > JonasA
> >
> > From: Yemin (Amy)
> <[email-masked]<mailto:[email-masked]>>
> 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.
> >
> 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
> 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.
> raised that the interface protection function could be generic and be used by
> other technologies in future, so we split the models.
> else, but if it indeed is usable elsewhere, having it in a separate module isn't
> a bad idea.
> 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).
> >
> >
> around the fact that this is indeed meant to be config true.
> 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.
> extend the module with additional, potentially vendor specific, commands by
> augmentation.
> that they can take (different) input and output parameters, and that they
> can be access controlled separately using NACM. This way different
> standard or vendor modules could still augment whatever additional actions
> they like to the same location in the tree.
> Amy, should we consider change to this or are there other benefits with the
> original approach?
> >
> 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?
> >
> ’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.
> >          + "/if:type, 'mw-types:carrier-termination')" { [Amy] will
> > adopt this.
> > [Jonas] Agree.
> >
> 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
> attribute used for configuration or the attribute showing the resulting be
> called rx-frequency?
> 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.
> >
> 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?
> >
> 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
> >          or Automatic Transmit Power Control (ATPC)."; }
> >
> about to move it out of the choice, then use maximum-nominal-power in the
> choice? Like this:
> 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.
> 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.
> >
> (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.
> >
> 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.
> 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
> >
> functionality. A bigger example, e.g. including xpic with interface pointers
> might be useful.
> >

---

From: Daniele Ceccarelli <[email-masked]>
Date: Tue, 12 Jun 2018 16:05:35 +0000

Hi CCAMP,

Regarding this one:

“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.”

Protection groups already exist in various drafts, e.g. the following in TE-tunnel.

leaf protection-group-ingress-node-id {
      type te-types:te-node-id;
      description
        "Indicates the te-node-id of the protection group
        ingress node when the APS state represents an extenal
        command (LoP, SF, MS) applied to it or a WTR timer
        running on it. If the external command is not applied to
        the ingress node or the WTR timer is not running on it,
        this attribute is not specified. If value 0.0.0.0 is used
        when the te-node-id of the protection group ingress node is
        unknown (e.g., because the ingress node is outside the scope
        of control of the server)";

i’m not sure if we already have a generalized protection group or we have a specific one already there and hence this one would be a specific one for microwave.
If the former I’d remove the definition of a protection group in the MW draft and point and extend the existing one, if the latter it’s ok to merge the two modules since everything is microwave specific.

Can you help me understand what’s the case?

BR
Daniele

Sent: martedì 12 giugno 2018 02:34

Jonas,

Hi Jan & Amy,

From: Jan Lindblad <[email-masked]<mailto:[email-masked]>>
Sent: den 11 juni 2018 15:29
To: Jonas Ahlberg <[email-masked]<mailto:[email-masked]>>
Cc: Yemin (Amy) <[email-masked]<mailto:[email-masked]>>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>
Subject: Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Jonas,

Regards
JonasA

Hi Jan,

BR,
Amy
-----Original Message-----
From: CCAMP [mailto:[email-masked]] On Behalf Of Jan Lindblad
Sent: Thursday, June 07, 2018 6:18 PM
To: [email-masked]<mailto:[email-masked]>
Cc: [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>
Subject: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Reviewer: Jan Lindblad

  identity qam-4096 {
    base coding-modulation;
    description

  identity E1 {
  identity STM-1 {
[Amy] will fix it.
[Jonas] OK.

      leaf status {

is better written as

#6) Blank id reasonable?

    leaf id {
      type string;
      default "";

is better written as

    leaf tx-frequency {
      type uint32;
      units "kHz";
      mandatory true;
      description

    leaf duplex-distance {
      type uint32;
      units "kHz";
      description

    leaf tx-frequency {
      type uint32;
      units "kHz";
      mandatory true;
      description

leaf tx { ... }
choice freq-or-dist {
  container by-frequency {
    leaf rx { ... }

    leaf power-mode {
      type enumeration {
        enum rtpc {
          description

        enum atpc {
          description

could be modeled as:

    choice power-mode {
      container rtpc {
        description

    choice power-mode {
      container rtpc {
        description

Same thing again for

Appendix A.1 & A.2

Both examples list

        "tx-oper-status": on

Both examples list

Best Regards,
/jan

---

From: "Yemin (Amy)" <[email-masked]>
Date: Wed, 13 Jun 2018 02:02:07 +0000

Hi Tom, 

Thanks for pointing out the YANG guideline. I also think that E1/STM-1 fall within the MAY case. 
E1/STM-1 is quiet meaningful in transport area. 
If it's possible to use E1, I certainly prefer E1/STM-1 which is straight forward. 

Regarding the STM-1/STM1, I think STM-1 is right way. 

BR,
Amy

-----Original Message-----
From: t.petch [mailto:[email-masked]] 
Sent: Tuesday, June 12, 2018 7:56 PM
To: Jonas Ahlberg <[email-masked]>; Jan Lindblad <[email-masked]>
Cc: [email-masked]; [email-masked]; Yemin (Amy) <[email-masked]>; [email-masked]; ietf <[email-masked]>
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

> [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

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" <[email-masked]>
To: "Jan Lindblad" <[email-masked]>

> 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.
> 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.
> 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.
> 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.
> [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.
> [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).
> 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?
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.
> [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.
> 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.
> [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?
> 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.
> 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.
> [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.
> 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.
> 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?
> 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.
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?
> 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
> [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.
> 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.
> 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.
> [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.
> 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.
> 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.

---

From: Jonas Ahlberg <[email-masked]>
Date: Fri, 22 Jun 2018 08:26:22 +0000

Hi all,

A new version of I-D, draft-ietf-ccamp-mw-yang-06.txt has been submitted to address the review comments from the Yangdoctors. See details below.

Name:                  draft-ietf-ccamp-mw-yang

Revision:              06

Title:                      A YANG Data Model for Microwave Radio Link

Document date:               2018-06-19

Group:                  ccamp

Pages:                   50

URL:            https://www.ietf.org/internet-drafts/draft-ietf-ccamp-mw-yang-06.txt

Status:         https://datatracker.ietf.org/doc/draft-ietf-ccamp-mw-yang/

Htmlized:       https://tools.ietf.org/html/draft-ietf-ccamp-mw-yang-06

Htmlized:       https://datatracker.ietf.org/doc/html/draft-ietf-ccamp-mw-yang

Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-ccamp-mw-yang-06

Abstract:

   This document defines a YANG data model for control and management of

   the radio link interfaces, and their connectivity to packet

   (typically Ethernet) interfaces in a microwave/millimeter wave node.

   The data nodes for management of the interface protection

   functionality is broken out into a separate and generic YANG data

   model in order to make it available also for other interface types.

We have addressed the comments from the review in the following way.

#1) Consider adding structure to related identities
      Updated according to the review comment, but without multiple inheritances.

#2) Convention to use all lowercase in YANG symbols
       The original uppercase symbols for E1 and STM-1 will be kept as is.

#x) Keep the interface-protection-module separate or not.
       After a discussion with the chairs and the group the preference is to keep them separate.
      No change.

#3) Config true leaf name status
       Changed to “config false”

#4) Action external-commands
      Updated according to the review comment, where each action is a separate action statement.

#5) Use derived-from when comparing identities
      Updated according to the review comment

#6) Blank id reasonable?
       The default has been removed and the description has been updated to clarify the consequence of not configuring this leaf.

#7) Use derived-from when comparing identities (again)
      Updated according to the review comment

#8) Badly broken frequency duplex config
       Updated according to the review comment. Duplex-distance changed to signed on duplex and make the choice statement mandatory.

#9) Check that lower threshold is less than upper threshold
       Check added according to review comment.

#10) Choice more convenient
         Choice used to replace power-mode.

#11) Choice more convenient (again)
         Choice used to replace coding-modulation-mode

#12) Unusual exponential notation
         Corrected according to review comment.

#13) Separate module with grouping, used a single time with refine
         Related to the decision to keep the modules separate with the purpose to make it useable for other interface types as well.
         Refinement required since the validation of the type of interface cannot be generic.

#14) Config false leaf in config example
         Removed according to review comment.

#15) Wrong type in example
         Updated according to review comment.

#16) Missing mandatory parameter
         Added according to review comment.

#17) Examples perhaps a tad basic
         A third example has been added.

Regards
JonasA
on behalf of the microwave team

Sent: den 12 juni 2018 09:34

Jonas,

Hi Jan & Amy,

Jonas,

Regards
JonasA

Hi Jan,

BR,
Amy
-----Original Message-----
From: CCAMP [mailto:[email-masked]] On Behalf Of Jan Lindblad
Sent: Thursday, June 07, 2018 6:18 PM
To: [email-masked]<mailto:[email-masked]>
Cc: [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>
Subject: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05

Reviewer: Jan Lindblad

  identity qam-4096 {
    base coding-modulation;
    description

  identity E1 {
  identity STM-1 {
[Amy] will fix it.
[Jonas] OK.

      leaf status {

is better written as

#6) Blank id reasonable?

    leaf id {
      type string;
      default "";

is better written as

    leaf tx-frequency {
      type uint32;
      units "kHz";
      mandatory true;
      description

    leaf duplex-distance {
      type uint32;
      units "kHz";
      description

    leaf tx-frequency {
      type uint32;
      units "kHz";
      mandatory true;
      description

leaf tx { ... }
choice freq-or-dist {
  container by-frequency {
    leaf rx { ... }

    leaf power-mode {
      type enumeration {
        enum rtpc {
          description

        enum atpc {
          description

could be modeled as:

    choice power-mode {
      container rtpc {
        description

    choice power-mode {
      container rtpc {
        description

Same thing again for

Appendix A.1 & A.2

Both examples list

        "tx-oper-status": on

Both examples list

Best Regards,
/jan