Re: [Linuxptp-devel] [PATCH RFC 4/6] clock: set sample weight.

[Richard C. <ric...@gm...>]

On Fri, Feb 13, 2015 at 01:56:16PM +0100, Miroslav Lichvar wrote:
> If there is (on average) at least one delay message for every sync
> message and the servo prefers weighted samples, use the sample delay
> directly instead of the long-term filtered delay and set the weight of
> the sample to the ratio of the two delays.
> 
> Signed-off-by: Miroslav Lichvar <mli...@re...>
> ---
>  clock.c | 38 +++++++++++++++++++++++++++++++-------
>  clock.h |  5 ++++-
>  port.c  |  7 ++++++-
>  3 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 64ce562..acf159b 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1418,10 +1418,11 @@ enum servo_state clock_synchronize(struct clock *c,
>  				   struct timespec ingress_ts,
>  				   struct timestamp origin_ts,
>  				   Integer64 correction1,
> -				   Integer64 correction2)
> +				   Integer64 correction2,
> +				   int sync_delay_rate)
>  {
> -	double adj;
> -	tmv_t ingress, origin;
> +	double adj, weight;
> +	tmv_t ingress, origin, sample_delay;
>  	enum servo_state state = SERVO_UNLOCKED;
>  
>  	ingress = timespec_to_tmv(ingress_ts);
> @@ -1433,15 +1434,38 @@ enum servo_state clock_synchronize(struct clock *c,
>  	c->c1 = correction_to_tmv(correction1);
>  	c->c2 = correction_to_tmv(correction2);
>  
> +	if (!c->path_delay)
> +		return state;
> +
> +	weight = 1.0;
> +
> +	/*
> +	 * If there is (on average) at least one delay message for every sync
> +	 * message and the servo prefers weighted samples, use the sample delay
> +	 * directly instead of the long-term filtered delay and set the weight
> +	 * of the sample to the ratio of the two delays.
> +	 */
> +	if (sync_delay_rate <= 0 && servo_weight_samples(c->servo)) {
> +		/* Get new sample delay with updated t1 and t2 */
> +		sample_delay = clock_get_sample_delay(c);
> +
> +		if (sample_delay > 0 && c->path_delay > 0) {
> +			weight = (double)c->path_delay / sample_delay;
> +			if (weight > 1.0)
> +				weight = 1.0;
> +			pr_debug("delay sample %9" PRId64 " filtered %9" PRId64
> +				 " weight %f",
> +				 sample_delay, c->path_delay, weight);
> +			c->path_delay = sample_delay;

So now, c->path_delay is set once in clock_path_delay() using
averaging, and here the averaged value is used for the ratio, but then
you clobber c->path_delay with the new value?

Your intention would be more clear if you would not set c->path_delay
here, but rather use a local variable.

Thanks
Richard

> +		}
> +	}
> +
>  	/*
>  	 * c->master_offset = ingress - origin - c->path_delay - c->c1 - c->c2;
>  	 */
>  	c->master_offset = tmv_sub(ingress,
>  		tmv_add(origin, tmv_add(c->path_delay, tmv_add(c->c1, c->c2))));
>  
> -	if (!c->path_delay)
> -		return state;
> -
>  	if (clock_utc_correct(c, ingress))
>  		return c->servo_state;
>  
> @@ -1451,7 +1475,7 @@ enum servo_state clock_synchronize(struct clock *c,
>  		return clock_no_adjust(c);
>  
>  	adj = servo_sample(c->servo, tmv_to_nanoseconds(c->master_offset),
> -			   tmv_to_nanoseconds(ingress), 1.0, &state);
> +			   tmv_to_nanoseconds(ingress), weight, &state);
>  	c->servo_state = state;
>  
>  	if (c->stats.max_count > 1) {
> diff --git a/clock.h b/clock.h
> index b3a5c9f..a5a70d3 100644
> --- a/clock.h
> +++ b/clock.h
> @@ -223,13 +223,16 @@ int clock_switch_phc(struct clock *c, int phc_index);
>   * @param correction1  The correction field of the sync message.
>   * @param correction2  The correction field of the follow up message.
>   *                     Pass zero in the case of one step operation.
> + * @param sync_delay_rate The number of expected sync messages per delay
> + *                     message, as a power of two.
>   * @return             The state of the clock's servo.
>   */
>  enum servo_state clock_synchronize(struct clock *c,
>  				   struct timespec ingress_ts,
>  				   struct timestamp origin_ts,
>  				   Integer64 correction1,
> -				   Integer64 correction2);
> +				   Integer64 correction2,
> +				   int sync_delay_rate);
>  
>  /**
>   * Inform a slaved clock about the master's sync interval.
> diff --git a/port.c b/port.c
> index 7f9aa3d..8cf52f7 100644
> --- a/port.c
> +++ b/port.c
> @@ -1012,11 +1012,16 @@ static void port_synchronize(struct port *p,
>  			     Integer64 correction1, Integer64 correction2)
>  {
>  	enum servo_state state;
> +	int sync_delay_rate;
>  
>  	port_set_sync_rx_tmo(p);
>  
> +	sync_delay_rate = (p->delayMechanism == DM_P2P ?
> +			   p->logMinPdelayReqInterval :
> +			   p->logMinDelayReqInterval) - p->logSyncInterval;
> +
>  	state = clock_synchronize(p->clock, ingress_ts, origin_ts,
> -				  correction1, correction2);
> +				  correction1, correction2, sync_delay_rate);
>  	switch (state) {
>  	case SERVO_UNLOCKED:
>  		port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
> -- 
> 2.1.0
> 
> 
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming. The Go Parallel Website,
> sponsored by Intel and developed in partnership with Slashdot Media, is your
> hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials and more. Take a
> look and join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> Linuxptp-devel mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel