Hello, Laurent. See below.
From: Laurent Toutain <laurent.toutain@xxxxxxxxxxxxxxxxx>
Date: Wednesday, July 20, 2022 at 11:20 AM
To: Joe Clarke (jclarke) <jclarke@xxxxxxxxx>
Cc: yang-doctors@xxxxxxxx <yang-doctors@xxxxxxxx>, draft-ietf-lpwan-schc-yang-data-model.all@xxxxxxxx <draft-ietf-lpwan-schc-yang-data-model.all@xxxxxxxx>, last-call@xxxxxxxx <last-call@xxxxxxxx>, lp-wan@xxxxxxxx <lp-wan@xxxxxxxx>
Subject: Re: [lp-wan] [yang-doctors] Yangdoctors last call review of draft-ietf-lpwan-schc-yang-data-model-14Hi Joe,
Thanks a lot for your comments, it really improve the document.
On Fri, Jul 15, 2022 at 10:32 PM Joe Clarke (jclarke) <jclarke=40cisco.com@xxxxxxxxxxxxxx> wrote:
Ugh, this didn’t get formatted well at all. Resending here:
- YANG tree has subtle differences from the YANG module; regenerate
done and included in -15. We compress a little bit the yang tree to avoid to have too long lines
which are creating errors with idnits. For instance this one:
+--:(fragmentation) {fragmentation}?
| +--rw fragmentation-mode schc:fragmentation-mode-type
which exceed 1 character.
[JMC] Have a look at RFC 8792 for wrapping text with long lines. For the tree have a look at Section 5.2.
- Run the module through `pyang -f yang`
same thing, the reformatting is creating some too long line errors, so we edited the document
to remove them. For instance the rule figure has been shifted to the left, and some carriage
return has been added. Is it correct ?
[JMC] I generally find that pyang in yang format does the right thing.
- No linting errors, though the copyright needs to be updated
I suppose it is the year in the yang module, we moved from 2021 to 2022.
The funny thing is that pyang --ietf complains with 2022 and not 2021.
[JMC] Yes, you’ll have to change some of the boilerplate language now that the year is 2022. In particular, the “Simplified BSD” license should now be the “Revised BSD” license. You also need https for the trustee web site.
- appiid and deviid should be AppIID and DevIID as stated in RFC8724
Do you mean in the identity ? We lowercased all acronyms in the module
- msb
-lsb
-no-ack
...
if we do it for appiid, we should do it everywhere.
[JMC] I was referring to descriptions only where you can do things like AppIID to make the descriptive text more in line with the base reference.
more clearly state what the leaf is/does and leave out that fact that it
uses an identityref
done, it becomes:
leaf field-length {
type schc:fl-type;
mandatory true;
description
"Field Length, expressed in number of bits if the length is
known when the Rule is created or through a specific function
if the length is variable.";
}
leaf direction-indicator {
type schc:di-type;
mandatory true;
description
"Direction Indicator, indicate if this field must be
consider for both directions, or only uplink or
downlink.";
}
[JMC] I might be a bit clearer on the direction of what in the description, but the rest reads fine to me.
- For something like di-type, does it make sense to be an identity? Seems like this could be an enumeration
as I don't think you'll have directions other than up, down, bi? But maybe you think
these types may be extended? Just curious.
We don't know, the problem with enumeration is that it is difficult to extend. We are currently investigate the mesh topology,
so may be another "direction" will appear. As we are not sure that there is only up and down, identity reference is more generic.
[JMC] Right, if you think extension will happen, use identities. Like I said, I was just curious. No action is required on your part. I am satisfied with your explanation.
- For w-size, why not use derived-from-or-self here (same in other places where fragmentation-mode is referenced)?
In fact, I must admit that these function are still a mystery for me. I didn't find a good tutorial on that and I tested them empirically with yangson.
I changed to derived-from-or-self. If I understand derive-from-and-self allows to test an equality and derived-from allows more to test what derives
from this identity but not the identify itself.
[JMC] Correct. And it seems like derived..self is what you want.
- See Section 3.4 of https://datatracker.ietf.org/doc/html/rfc8407 on how to reference YANG tree diagrams
I don't understand the comment
[JMC] Sorry. Prior to including your YANG tree mention that it conforms to RFC8349 and include the informative reference.
- Why do you have empty cases for the mode choice (e.g., no-ack, ack-always)? It's not clear you need a choice here.
And if you need it, you could just have one "when" clause on the case itself.
That's something we had difficulties to handle. We wanted originally to put parameters specific to the 3 frag mode in these choices,
but ACK-Always and ACK-on-Error got some common values, so we moved them in the generic part with some when/must conditions.
Since ACK-on-Error have some specific parameters, they are on the case. The question for us was if we have in the future an augmentation
for one mode, it is better in our views to define the name that will be used to do this augmentation.
We have already an augmentation for ACK-on-Error to support compound-ack.
[JMC] When I read Section 4.7 of RFC 8407 I tend to think that you should target to augment the choice rather than add empty case statements that _might_ be used in the future. The name of the case is less important than the nodes that will come under it anyway.
- There is another empty case for the no-compression case in the nature choice.
The description there says that a rule is required.
- Can one have both
features for compression and fragmentation? The choice seems to imply no, but
I am curious. I didn't get the impression that they were mutually exclusive
from 8724.
you are right, that's a big problem, a rule can be of only one nature, and right now we can carry both frag and comp and
no-frag is transparent since nature does not appear in the data model.
one solution is to add a leaf which will define the nature of the rule before the choice and test it in each case to see if we allow it.
[JMC] If you can have both, the current choice approach will not work since the server cannot realize both cases at the same time. Since you already have features defined for each, why not let if-feature do the work here and have containers for compression and fragmentation?
Joe
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
