WGLC Review Comment [Section 3.3]

From https://mailarchive.ietf.org/arch/msg/core/WOe7F8-y3TNbrqnLkYQshZB1flk/

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

I rewrote this part of the draft, and plan to instead move the following pseudocode into an Appendix:


// struct Resource {
//
//  bool band;
//  int pmin;
//  int pmax;
//  int epmin;
//  int epmax;
//  int st;
//  
//  time_t last_sampled_time;
//  time_t last_rep_time;

//  int curr_state;
//  int prev_state;   
//
//  ...
//
// };

boolean is_notifiable( Resource * r ) {

    #define BAND_EXISTS ( r->band )
    #define LT_EXISTS ( r->lt )
    #define GT_EXISTS ( r->gt )

    #define EPMIN_TRUE ( curr_time - r->last_sampled_time >= r->epmin )
    #define EPMAX_TRUE ( curr_time - r->last_sampled_time > r->epmax )

    #define PMIN_TRUE ( curr_time - r->last_reported_time >= r->pmin )
    #define PMAX_TRUE ( curr_time - r->last_reported_time > r->pmax )

    #define LT_TRUE ( r->curr_state < r->lt ^ r->prev_state < r->lt )
    #define GT_TRUE ( r->curr_state > r->gt ^ r->prev_state > r->gt )

    #define ST_TRUE ( abs( r->curr_state - r->prev_state ) >= r->st )

    #define IN-BAND_TRUE ( r->gt < r->lt && r->gt <= r->curr_state && r->curr_state <= r->lt )
    #define OUT-OF-BAND_TRUE ( r->lt < r-gt && r->gt < r->curr_state && r->curr_state < r->lt )
    #define BAND-MIN_TRUE ( r->lt <= r->curr_state)
    #define BAND-MAX_TRUE (r->curr_state <= r->gt)

    if PMAX_TRUE {
        return true;
    }

    if PMIN_TRUE {
        if !BAND_EXISTS {
            if LT_TRUE || GT_TRUE || ST_TRUE {
                return true;
            }
        }
        else {
         if ( ( BAND-MIN_TRUE && !GT_EXISTS) || (BAND-MAX_TRUE && !LT_EXISTS) || IN-BAND_TRUE || OUT-OF-BAND_TRUE ) {
             return true;
         }
        }
    }

    return false;

}

Explanation:

Moving the code logic away from the main text does not affect the legibility of the draft
My intention for the re-write is to improve readability .
In the Appendix, I'll explicitly state this is pseudocode even if I'm writing with C-language constructs, and it is for reference and is not intended to be compile-safe.
I simplified the state. Right now it's just a scalar (int) type, and not a Boolean or String as was in the original code.
The code logic is predicated on checking pmin and pmax, and not epmin and epmax. Please note that pmin and pmax check on time between state updates sent to the client, while epmin and epmax check on time between sampled values.

@marco-tiloca-sics @dnav please review

In the definition of the Resource structure:

Add also lt and gt, before st
Rename last_rep_time to last_reported_time

In is_notifiable():

Even though the name of curr_time is intuitive, it can be separately defined upfront right at the beginning of is_notifiable(), making it clear that its value is the current time. For instance, curr_time can take what is returned by a function get_current_time(), not to be detailed in the example.
#define LT_TRUE ( r->curr_state < r->lt ^ r->prev_state < r->lt )

Shouldn't this be like this instead?

#define LT_TRUE ( ( r->curr_state < r->lt && r->prev_state > r->lt ) ^ ( r->curr_state > r->lt && r->prev_state < r->lt ) )
#define GT_TRUE ( r->curr_state > r->gt ^ r->prev_state > r->gt )

Shouldn't this be like this instead?

#define GT_TRUE ( (r->curr_state > r->gt && r->prev_state < r->gt ) ^ (r->curr_state < r->gt && r->prev_state > r->gt ) )
#define IN-BAND_TRUE ( r->gt < r->lt && r->gt <= r->curr_state && r->curr_state <= r->lt )

Shouldn't this be like this instead?

#define IN-BAND_TRUE ( LT_EXISTS && GT_EXISTS && r->gt < r->lt && r->gt <= r->curr_state && r->curr_state <= r->lt )
#define OUT-OF-BAND_TRUE ( r->lt < r-gt && r->gt < r->curr_state && r->curr_state < r->lt )

Shouldn't this be like this instead?

#define OUT-OF-BAND_TRUE ( LT_EXISTS && GT_EXISTS && r->lt < r-gt && ( r->gt < r->curr_state ^ r->curr_state < r->lt ) )
#define BAND-MIN_TRUE ( r->lt <= r->curr_state)

Shouldn't this be like this instead?

#define BAND-MIN_TRUE ( LT_EXISTS && r->lt <= r->curr_state)
#define BAND-MAX_TRUE (r->curr_state <= r->gt)

Shouldn't this be like this instead?

#define BAND-MAX_TRUE (GT_EXISTS && r->curr_state <= r->gt)

In the definition of the Resource structure:

Add also lt and gt, before st

Rename last_rep_time to last_reported_time

In is_notifiable():

Even though the name of curr_time is intuitive, it can be separately defined upfront right at the beginning of is_notifiable(), making it clear that its value is the current time. For instance, curr_time can take what is returned by a function get_current_time(), not to be detailed in the example.

Would it be alright just to cut corner a little and add just a single line, such as:

time_t curr_time = get_current_time();

#define LT_TRUE ( r->curr_state < r->lt ^ r->prev_state < r->lt ) Shouldn't this be like this instead? #define LT_TRUE ( ( r->curr_state < r->lt && r->prev_state > r->lt ) ^ ( r->curr_state > r->lt && r->prev_state < r->lt ) )

#define GT_TRUE ( r->curr_state > r->gt ^ r->prev_state > r->gt ) Shouldn't this be like this instead? #define GT_TRUE ( (r->curr_state > r->gt && r->prev_state < r->gt ) ^ (r->curr_state < r->gt && r->prev_state > r->gt ) )

I would prefer to keep the macros I proposed, based on Michael's code for 2 reasons:

They are far easier to read and comprehend
They also handle corner cases correctly. For example, if lt=5, gt =10: if the state changes from 9 to 11, a notification would be sent both by both versions of the GT_TRUE macros, but if the state changes from 10 to 11 (or 11 to 10), only the current GT_TRUE will indicate a notifiable condition and not the one you proposed. This is also the same with the LT_TRUE macros (eg from 5 to 4 or 4 to 5).
In all other cases, both versions appear to function the same.

#define IN-BAND_TRUE ( r->gt < r->lt && r->gt <= r->curr_state && r->curr_state <= r->lt ) Shouldn't this be like this instead? #define IN-BAND_TRUE ( LT_EXISTS && GT_EXISTS && r->gt < r->lt && r->gt <= r->curr_state && r->curr_state <= r->lt )

#define OUT-OF-BAND_TRUE ( r->lt < r-gt && r->gt < r->curr_state && r->curr_state < r->lt ) Shouldn't this be like this instead? #define OUT-OF-BAND_TRUE ( LT_EXISTS && GT_EXISTS && r->lt < r-gt && ( r->gt < r->curr_state ^ r->curr_state < r->lt ) )

#define BAND-MIN_TRUE ( r->lt <= r->curr_state) Shouldn't this be like this instead? #define BAND-MIN_TRUE ( LT_EXISTS && r->lt <= r->curr_state)

#define BAND-MAX_TRUE (r->curr_state <= r->gt) Shouldn't this be like this instead? #define BAND-MAX_TRUE (GT_EXISTS && r->curr_state <= r->gt)

I can add the LT_EXISTS and GT_EXISTS macros to these.

Pseudocode is now rectified and added as an appendix to draft -06

core-wg / conditional-attributes

WGLC Review Comment [Section 3.3] #23