> On Mar 25, 2019, at 7:12 AM, Richard Cochran <[email protected]> wrote:
>
> On Mon, Mar 18, 2019 at 03:51:30PM -0700, Vedang Patel wrote:
>> @@ -1123,10 +1123,16 @@ static void port_synchronize(struct port *p,
>> c2 = correction_to_tmv(correction2);
>> t1c = tmv_add(t1, tmv_add(c1, c2));
>>
>> + last_state = clock_servo_state(p->clock);
>> state = clock_synchronize(p->clock, t2, t1c);
>> switch (state) {
>> case SERVO_UNLOCKED:
>> port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
>> + p->logPdelayReqInterval = p->logMinPdelayReqInterval;
>> + p->logSyncInterval = p->initialLogSyncInterval;
>> + port_tx_interval_request(p, SIGNAL_NO_CHANGE,
>> + p->logSyncInterval,
>> + SIGNAL_NO_CHANGE);
>
> This sends a message with every Sync. That isn't what we want.
> Instead, we should send the request only when entering SERVO_UNLOCKED,
> and only if the relevant options are configured.
>
Yes. You are right. We should check whether last state was SERVO_UNLOCKED and
the servo_offset_threshold config option is not 0.
Also, p->logSyncInterval here should be replaced by SIGNAL_SET_INITIAL so slave
does not enforce it’s own config options on the master. This is the reason why
the 11-syncinterval and 23-basedelay were failing.
>> break;
>> case SERVO_JUMP:
>> port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
>> @@ -1139,6 +1145,16 @@ static void port_synchronize(struct port *p,
>> case SERVO_LOCKED:
>> port_dispatch(p, EV_MASTER_CLOCK_SELECTED, 0);
>> break;
>> + case SERVO_LOCKED_STABLE:
>> + if (last_state == SERVO_LOCKED) {
>> + p->logPdelayReqInterval = p->operLogPdelayReqInterval;
>> + p->logSyncInterval = p->operLogSyncInterval;
>> + port_tx_interval_request(p, SIGNAL_NO_CHANGE,
>> + p->logSyncInterval,
>> + SIGNAL_NO_CHANGE);
>
> In contrast, this one is okay, since it only sends once, and only if
> the options are set (otherwise you can't enter SERVO_LOCKED_STABLE).
>
>> + }
>> + port_dispatch(p, EV_MASTER_CLOCK_SELECTED, 0);
>> + break;
>> }
>> }
>>
>> @@ -1613,8 +1629,10 @@ int port_initialize(struct port *p)
>> p->localPriority = config_get_int(cfg, p->name,
>> "G.8275.portDS.localPriority");
>> p->initialLogSyncInterval = config_get_int(cfg, p->name,
>> "logSyncInterval");
>> p->logSyncInterval = p->initialLogSyncInterval;
>> + p->operLogSyncInterval = config_get_int(cfg, p->name,
>> "operLogSyncInterval");
>> p->logMinPdelayReqInterval = config_get_int(cfg, p->name,
>> "logMinPdelayReqInterval");
>> p->logPdelayReqInterval = p->logMinPdelayReqInterval;
>> + p->operLogPdelayReqInterval = config_get_int(cfg, p->name,
>> "operLogPdelayReqInterval");
>> p->neighborPropDelayThresh = config_get_int(cfg, p->name,
>> "neighborPropDelayThresh");
>> p->min_neighbor_prop_delay = config_get_int(cfg, p->name,
>> "min_neighbor_prop_delay");
>>
>> diff --git a/port_private.h b/port_private.h
>> index d28ab0712798..bb1d86e08fb5 100644
>> --- a/port_private.h
>> +++ b/port_private.h
>> @@ -115,9 +115,11 @@ struct port {
>> UInteger8 transportSpecific;
>> UInteger8 localPriority;
>> Integer8 initialLogSyncInterval;
>> + Integer8 operLogSyncInterval;
>> Integer8 logSyncInterval;
>> Enumeration8 delayMechanism;
>> Integer8 logMinPdelayReqInterval;
>> + Integer8 operLogPdelayReqInterval;
>> Integer8 logPdelayReqInterval;
>> UInteger32 neighborPropDelayThresh;
>> int follow_up_info;
>> diff --git a/ptp4l.8 b/ptp4l.8
>> index 99b085c148c9..fad21bf57724 100644
>> --- a/ptp4l.8
>> +++ b/ptp4l.8
>> @@ -161,6 +161,13 @@ The mean time interval between Sync messages. A shorter
>> interval may improve
>> accuracy of the local clock. It's specified as a power of two in seconds.
>> The default is 0 (1 second).
>> .TP
>> +.B operLogSyncInterval
>> +The mean time interval between Sync messages. This value is only used by the
>> +slave device when interval_update_timer is enabled. Slave will send this
>> +interval to the master to switch to. This is done via a signaling message
>> after
>> +interval_update_timer expires. It's specified as a power of two in seconds.
>> The
>> +default value is 0 (1 second).
>> +.TP
>> .B logMinDelayReqInterval
>> The minimum permitted mean time interval between Delay_Req messages. A
>> shorter
>> interval makes ptp4l react faster to the changes in the path delay. It's
>> @@ -172,6 +179,13 @@ The minimum permitted mean time interval between
>> Pdelay_Req messages. It's
>> specified as a power of two in seconds.
>> The default is 0 (1 second).
>> .TP
>> +.B operLogPdelayReqInterval
>> +The mean time interval between Pdelay Request messages. This value is only
>> used
>> +by the slave device when interval_update_timer is enabled. Slave will
>> switch to
>> +the interval specified by this config option after the interval_update_timer
>> +expires. It's specified as a power of two in seconds. The default value is
>> 0 (1
>> +second).
>> +.TP
>> .B announceReceiptTimeout
>> The number of missed Announce messages before the last Announce messages
>> expires.
>> @@ -715,6 +729,22 @@ This will disable source port identity checking for
>> Sync and Follow_Up
>> messages. This is useful when the announce messages are disabled in the
>> master
>> and the slave does not have any way to know it's identity. The default is 0
>> (disabled).
>> +.TP
>> +.B servo_num_offset_values
>> +The number of offset values calculated in previously received Sync messages
>> to
>> +consider when adjusting the Sync and Pdelay request intervals. More
>> information
>> +provided in the description of 'offset_threshold'. The default value is 10.
>> +.TP
>> +.B servo_offset_threshold
>> +This value is used by the slave for adjusting the intervals for Sync and
>> Pdelay
>> +request messages. The slave will check the last 'num_offset_values' offsets
>> and
>> +if all those offsets are less than the offset_threshold, it will adjust both
>> +the intervals. The Sync interval is adjusted via the signaling mechanism and
>> +the pdelay request interval is just adjusted locally. The new values to use
>> for
>> +sync message intervals and pdelay request intervals can be indicated by
>> +operLogSyncInterval and operLogPdelayReqInterval respectively. This
>> mechanism
>> +is currently only supported when BMCA == 'noop'. The default
>> +value of offset_threshold is 0 (disabled).
>>
>> .SH UNICAST DISCOVERY OPTIONS
>>
>> diff --git a/servo.c b/servo.c
>> index 8be4b92875de..483ad31487f1 100644
>> --- a/servo.c
>> +++ b/servo.c
>> @@ -17,6 +17,7 @@
>> * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> */
>> #include <string.h>
>> +#include <stdlib.h>
>>
>> #include "config.h"
>> #include "linreg.h"
>> @@ -79,6 +80,8 @@ struct servo *servo_create(struct config *cfg, enum
>> servo_type type,
>> }
>>
>> servo->first_update = 1;
>> + servo->offset_threshold = config_get_int(cfg, NULL,
>> "servo_offset_threshold");
>> + servo->num_offset_values = config_get_int(cfg, NULL,
>> "servo_num_offset_values");
>>
>> return servo;
>> }
>> @@ -88,6 +91,19 @@ void servo_destroy(struct servo *servo)
>> servo->destroy(servo);
>> }
>>
>> +static int check_offset_threshold(struct servo *s, int64_t offset)
>> +{
>> + uint64_t abs_offset = abs(offset);
>
> abs() returns an int...
>
>> + if (s->offset_threshold) {
>> + if (abs_offset < INT64_MAX && abs_offset < s->offset_threshold
> ^^^^^^^^^^^^^^^^^^^^^^
> ... so this test is always true
>
Sorry I missed this. Will change it to INT_MAX.
Thanks,
Vedang
>> + && s->curr_offset_values)
>> + s->curr_offset_values--;
>> + return s->curr_offset_values ? 0 : 1;
>> + }
>> + return 0;
>> +}
>> +
>> double servo_sample(struct servo *servo,
>> int64_t offset,
>> uint64_t local_ts,
>
> Thanks,
> Richard
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
