Re: [core] WG Last Call on draft-ietf-core-conditional-attributes

[Marco Tiloca <[e-mail]>]

Hi all,

Please find below my review comments.

Best,
/Marco


[Section 1]

* "describe a REST protocol [RFC7252]"

    Perhaps do you mean "describe the REST protocol CoAP [RFC7252]" ?

* s/machine to machine/machine-to-machine

* s/CoRE Observe/CoAP Observe


[Section 1.1]

* "This specification requires readers to be familiar with all the terms 
and concepts that are discussed in [RFC7641]."

    I would refer also to RFC 7252.


[Section 3]

* s/CoRE Observe/CoAP Observe

* s/simply a "name" without/simply as a "name" query parameter without

* s/"Note: In this draft, we"/Note: in this specification, we


[Section 3.1]

* "to be interesting"

    Do you mean "to trigger a notification" ?


[Section 3.1.3]

* s/the change step indicates/the Change Step attribute indicates

* s/the old state/the last reported value

* s/reported state by an amount/reported value by an amount

* s/on resource states represented with a scalar numeric value./on 
resources with a scalar numeric value.


[Section 3.1.4]

* In the section title, s/band/Band

* s/The notification band/The Notification Band

* When "c.band" is present with the "c.lt" attribute, it defines ...

    I think you mean:

      When the Notification Band and Less Than attributes are present 
while the Greater Than attribute is not, then the present attribute pair 
defines ...

* When "c.band" is present with the "c.gt" attribute, it defines ...

    I think you mean:

      When the Notification Band and Greater Than attributes are present 
while the Less Than attribute is not, then the present attribute pair 
defines ...

* s/in which the value/and the value  (2 occurrences)

* s/value not between/value is not between


[Section 3.1.5]

* s/the "c.edge" attribute/the Edge attribute


[Section 3.2]

* s/measurement of the conditions/evaluation of the conditions

* s/measurements of the conditions/evaluations of the conditions


[Section 3.2.1]

* s/the minimum period indicates/the Minimum Period attribute indicates


[Section 3.2.2]

* s/the maximum period indicates/the Maximum Period attribute indicates

* s/the minimum period parameter (if present)/the Minimum Period 
parameter (if present)


[Section 3.2.3]

* s/the minimum evaluation period indicates/the Minimum Evaluation 
Period attribute indicates

* s/measurements/evaluations  (2 instances)

* s/measurement/evaluation  (3 instances)


[Section 3.2.4]

* s/the maximum evaluation period indicates/the Maximum Evaluation 
Period attribute indicates

* s/measurements/evaluations

* s/measurement/evaluation  (2 instances)


[Section 3.2.5]

* s/the "c.con" attribute indicates/the Confirmable Notification 
attribute indicates


[Section 3.3]

* "and time to prepare for the next notification"

    I think you mean "and the time when to prepare the next notification".

* As to these two lines in the reference code:

    #define PMIN_EX ( r->last_sample_time - r->last_rep_time >= r->pmin )
    #define PMAX_EX ( r->last_sample_time - r->last_rep_time > r->pmax )

    Based on the definitions of pmin and pmax, shouldn't these lines 
rather be like the following?

    #define PMIN_EX ( current_time - r->last_rep_time >= r->pmin )
    #define PMAX_EX ( current_time - r->last_rep_time > r->pmax )

* As to these two lines in the reference code:

    #define LT_EX ( r->v < r->lt ^ r->last_rep_v < r->lt )
    #define GT_EX ( r->v > r->gt ^ r->last_rep_v > r->gt )

    Based on the definitions of lt and gt, shouldn't these lines rather 
be like the following?

    #define LT_EX ( ( r->v < r->lt && r->last_rep_v >= r->lt ) || \
                    ( r->v > r->lt && r->last_rep_v <= r->lt ) )
    #define GT_EX ( ( r->v > r->gt && r->last_rep_v <= r->gt ) || \
                    ( r->v < r->gt && r->last_rep_v >= r->gt ) )

* The name IN_BAND can be confusing; its second and third logical 
conditions relate to what is called out-of-band in the second from last 
paragraph of Section 3.1.4. Perhaps rename it to BAND_MATCH or BAND_SCOPE ?


[Section 4]

* "or the configuration of "c.epmin" and "c.epmax" values ..."

    I think you mean: "or as defined by the "c.epmin" and "c.epmax" values"

* s/CoRE Observe/CoAP Observe


[Section 6]

* In the first paragraph, s/This memo/This specification

* In the references column, you can replace "This memo" with "RFC XXXX" 
(or similar), while adding the usual note to IANA before the table about 
replacing it with the RFC number.


[Section 10.2]

* Shouldn't RFC 7252 and RFC 7641 also be normative references?


[Appendix A]

* "binding attribute / observe attributes"

    This should be Conditional Notification Attributes and Conditional 
Control Attributes


[Appendix A.2]

* The caption of Figure 2 should conclude as:

    "... when c.pmax time expires."


[Appendix A.4]

* The caption of Figure 4 should be:

    "Client registers and receives one notification of the current 
state, one of a new state (when, coincidentally, c.pmax time also 
expires), and one of a new state when it passes through the greater than 
threshold of 25."


[Nits]

* Section 3.1
--- s/here allow a/here allows a
--- s/how often a client/how often the client

* Section 3.1.3: s/Note: Due/Note: due

* Section 3.1.4, s/ different ranges results in differing behaviour/ 
different ranges result in differing behaviours

* Section 3.2.1
--- s/occured/occurred
--- s/whether or not/regardless of whether or not
--- s/Note: Due/Note: due

* Section 3.2.2, s/whether or not/regardless of whether or not

* Section 3.2.3, s/resource since the client/resource, since the client

* Section 4
--- s/maximum allow for/maximum allows for
--- s/1 or more/one or more
--- s/eg/e.g.

* Appendix A
--- s/brevity the only/brevity only


On 2022-05-11 17:53, Marco Tiloca wrote:
> Dear all,
>
> This mail starts a Working Group Last Call for the document:
>
> https://datatracker.ietf.org/doc/html/draft-ietf-core-conditional-attributes-04 
>
>
> Please provide your comments and feedback by Wednesday, the 25th of May.
>
> Best,
> Marco (for the CoRE Chairs)
>
>
> _______________________________________________
> core mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/core

-- 
Marco Tiloca
Ph.D., Senior Researcher

Phone: [phone]

RISE Research Institutes of Sweden AB
Box 1263
164 29 Kista (Sweden)

Division: Digital Systems
Department: Computer Science
Unit: Cybersecurity

https://www.ri.se