[Linuxptp-devel] [PATCH RFC 1/4] Allow the measured path delay to be zero.

[Richard Cochran]

The current code overloads the path delay, using zero as a special case to
flag whether or not a measurement has been made. However, when using a
short patch cable, the path delay may well work out to zero.

This patch fixes the issue by using a separate state variable to track
the presence of the path delay information.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/clock.c b/clock.c
index 3350b3d..b448b8a 100644
--- a/clock.c
+++ b/clock.c
@@ -105,6 +105,7 @@ struct clock {
tmv_t path_delay;
tmv_t ingress_ts;
struct tsproc *tsproc;
+ int have_pd;
struct freq_estimator fest;
struct time_status_np status;
double nrr;
@@ -1289,6 +1290,8 @@ void clock_path_delay(struct clock *c, tmv_t req, tmv_t rx)
if (c->stats.delay)
stats_add_value(c->stats.delay,
tmv_to_nanoseconds(c->path_delay));
+
+ c->have_pd = 1;
}

void clock_peer_delay(struct clock *c, tmv_t ppd, tmv_t req, tmv_t rx,
@@ -1302,6 +1305,8 @@ void clock_peer_delay(struct clock *c, tmv_t ppd, tmv_t req, tmv_t rx,

if (c->stats.delay)
stats_add_value(c->stats.delay, tmv_to_nanoseconds(ppd));
+
+ c->have_pd = 1;
}

int clock_slave_only(struct clock *c)
@@ -1361,6 +1366,9 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t origin)
if (tsproc_update_offset(c->tsproc, &c->master_offset, &weight))
return state;

+ if (!c->have_pd)
+ return state;
+
if (clock_utc_correct(c, ingress))
return c->servo_state;

@@ -1474,6 +1482,7 @@ static void handle_state_decision_event(struct clock *c)
tsproc_reset(c->tsproc, 1);
c->ingress_ts = tmv_zero();
c->path_delay = 0;
+ c->have_pd = 0;
c->nrr = 1.0;
fresh_best = 1;
}

[Richard Cochran]

When running with Synchronous Ethernet (SyncE), the correct clock
frequency is provided by the link partner. In this case, only the
offset needs correcting.

This patch provides SyncE nodes with an option for keeping the
frequency correction dialed to zero.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 6 ++++++
config.c | 6 ++++++
default.cfg | 1 +
ds.h | 1 +
gPTP.cfg | 1 +
ptp4l.c | 1 +
6 files changed, 16 insertions(+)

diff --git a/clock.c b/clock.c
index b448b8a..db0b0da 100644
--- a/clock.c
+++ b/clock.c
@@ -92,6 +92,7 @@ struct clock {
int last_port_number;
int free_running;
int freq_est_interval;
+ int freq_noadj;
int grand_master_capable; /* for 802.1AS only */
int utc_timescale;
int utc_offset_set;
@@ -804,6 +805,7 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,

c->free_running = dds->free_running;
c->freq_est_interval = dds->freq_est_interval;
+ c->freq_noadj = dds->freq_noadj;
c->grand_master_capable = dds->grand_master_capable;
c->kernel_leap = dds->kernel_leap;
c->utc_offset = CURRENT_UTC_OFFSET;
@@ -1393,6 +1395,10 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t origin)

tsproc_set_clock_rate_ratio(c->tsproc, clock_rate_ratio(c));

+ if (c->freq_noadj) {
+ adj = 0.0;
+ }
+
switch (state) {
case SERVO_UNLOCKED:
break;
diff --git a/config.c b/config.c
index ee0f302..c49b31a 100644
--- a/config.c
+++ b/config.c
@@ -347,6 +347,12 @@ static enum parser_result parse_global_setting(const char *option,
cfg->dds.freq_est_interval = val;
pod->freq_est_interval = val;

+ } else if (!strcmp(option, "freq_noadj")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ cfg->dds.freq_noadj = val;
+
} else if (!strcmp(option, "assume_two_step")) {
r = get_ranged_int(value, &val, 0, 1);
if (r != PARSED_OK)
diff --git a/default.cfg b/default.cfg
index b46c0f6..ade49db 100644
--- a/default.cfg
+++ b/default.cfg
@@ -12,6 +12,7 @@ clockAccuracy 0xFE
offsetScaledLogVariance 0xFFFF
free_running 0
freq_est_interval 1
+freq_noadj 0
#
# Port Data Set
#
diff --git a/ds.h b/ds.h
index 162687a..159288e 100644
--- a/ds.h
+++ b/ds.h
@@ -54,6 +54,7 @@ struct default_ds {
struct defaultDS dds;
int free_running;
int freq_est_interval; /*log seconds*/
+ int freq_noadj;
int grand_master_capable; /*802.1AS only*/
int stats_interval; /*log seconds*/
int kernel_leap;
diff --git a/gPTP.cfg b/gPTP.cfg
index 34fa238..c86db6d 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -12,6 +12,7 @@ clockAccuracy 0xFE
offsetScaledLogVariance 0xFFFF
free_running 0
freq_est_interval 1
+freq_noadj 0
#
# Port Data Set
#
diff --git a/ptp4l.c b/ptp4l.c
index 61c5854..ab79098 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -54,6 +54,7 @@ static struct config cfg_settings = {
},
.free_running = 0,
.freq_est_interval = 1,
+ .freq_noadj = 0,
.grand_master_capable = 1,
.stats_interval = 0,
.kernel_leap = 1,

[Miroslav Lichvar]

Post by Richard Cochran
When running with Synchronous Ethernet (SyncE), the correct clock
frequency is provided by the link partner. In this case, only the
offset needs correcting.

I'm not familiar with SyncE. What does this mean for the PHC device?
The frequency is automatically corrected by the driver or the device
itself and the correction is visible in the timex structure from
clock_adjtime() or that the apparent frequency offset is 0?

Post by Richard Cochran
@@ -1393,6 +1395,10 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t origin)
tsproc_set_clock_rate_ratio(c->tsproc, clock_rate_ratio(c));
+ if (c->freq_noadj) {
+ adj = 0.0;
+ }
+
switch (state) {
break;

Hm, so this effectively disables the servo and the only correction of
the clock is step when the threshold is reached?

I'm wondering if this could be done as a servo option or a completely
new servo.

[Richard Cochran]

Post by Miroslav Lichvar
I'm not familiar with SyncE. What does this mean for the PHC device?
The frequency is automatically corrected by the driver or the device
itself and the correction is visible in the timex structure from
clock_adjtime() or that the apparent frequency offset is 0?

The frequency is locked in hardware from the link partner's transmit
clock. It is not visible in timex, but you do measure zero frequency
offset using the PTP.

Post by Miroslav Lichvar
Hm, so this effectively disables the servo and the only correction of
the clock is step when the threshold is reached?

Yes.

Post by Miroslav Lichvar
I'm wondering if this could be done as a servo option or a completely
new servo.

Could be a new servo... I'll think about it.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
I'm not familiar with SyncE. What does this mean for the PHC device?
The frequency is automatically corrected by the driver or the device
itself and the correction is visible in the timex structure from
clock_adjtime() or that the apparent frequency offset is 0?

The frequency is locked in hardware from the link partner's transmit
clock. It is not visible in timex, but you do measure zero frequency
offset using the PTP.

Ok. So what is the new option supposed to do? Reduce the noise in the
offset? An easy way to do that I think would be to reduce the PI
constants.

[Richard Cochran]

Post by Miroslav Lichvar
Ok. So what is the new option supposed to do? Reduce the noise in the
offset? An easy way to do that I think would be to reduce the PI
constants.

Really, the frequence offset is zero (or at least less that 1 ppb!).
The very first Sync message gives the phase offset, and after that
first jump both offsets stay zero forever.

The option is supposed to tell ptp4l, just set the frequency offset to
zero and then leave it there. Actually, the reseting to zero is
important, just in case a previous run without SyncE had set some
offset.

Thanks,
Richard

------------------------------------------------------------------------------

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
Ok. So what is the new option supposed to do? Reduce the noise in the
offset? An easy way to do that I think would be to reduce the PI
constants.

Really, the frequence offset is zero (or at least less that 1 ppb!).
The very first Sync message gives the phase offset, and after that
first jump both offsets stay zero forever.

Is it not possible that the offset will change when there is some
restart or reconfiguration in the PTP network?

Post by Richard Cochran
The option is supposed to tell ptp4l, just set the frequency offset to
zero and then leave it there. Actually, the reseting to zero is
important, just in case a previous run without SyncE had set some
offset.

Would happen anything bad if the servo set the frequency to a non-zero
value?

The servos need to be able to adjust the frequency to correct offsets
smaller than the step threshold. If adjusting frequency is not
allowed, the only option would be to always step.

[Richard Cochran]

Post by Miroslav Lichvar
Is it not possible that the offset will change when there is some
restart or reconfiguration in the PTP network?

Yes, but any phase offset that suddenly appears is fixed by a jump.
Using SyncE only makes sense when you have a chain (or tree) of
devices. The "first" device (at the start of the chain) will adjust
its frequency normally to match the GM. All of the other devices in
the chain never adjust their frequencies, because this happens in
hardware automatically.

Of course, it is important for a SyncE device to know whether the
"previous" peer in the chain is SyncE capable or not, but PTP does not
tell you this. There is an ITU standard that covers simple SyncE
notifications, and you need some kind of SyncE management program to
restart ptp4l when the status changes.

Post by Miroslav Lichvar
Would happen anything bad if the servo set the frequency to a non-zero
value?

This would cause a growing phase offset.

Post by Miroslav Lichvar
The servos need to be able to adjust the frequency to correct offsets
smaller than the step threshold. If adjusting frequency is not
allowed, the only option would be to always step.

Yes. I have 'freq_noadj 1' together with 'step_threshold 0.000000001'
in the configuration.

Thanks,
Richard

------------------------------------------------------------------------------

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
Is it not possible that the offset will change when there is some
restart or reconfiguration in the PTP network?

Yes, but any phase offset that suddenly appears is fixed by a jump.

Is that always preferred over slew?

Post by Richard Cochran

Post by Miroslav Lichvar
Would happen anything bad if the servo set the frequency to a non-zero
value?

This would cause a growing phase offset.

Ok, but adjusting frequency to correct a phase offset is still
fine, right?

Post by Richard Cochran

Post by Miroslav Lichvar
The servos need to be able to adjust the frequency to correct offsets
smaller than the step threshold. If adjusting frequency is not
allowed, the only option would be to always step.

Yes. I have 'freq_noadj 1' together with 'step_threshold 0.000000001'
in the configuration.

Hm, so the offset normally stays at zero or is it stepping on each
update?

If there is no noise in the measurements, then one step on start and no
more clock updates would make sense. If there is some noise I think it would
be still useful to let the servos adjust the frequency similarly to a
case with a stabilized clock (e.g. OCXO), which has a fixed but
non-zero frequency offset.

In seems odd to me to call the servo function and then ignore its
result. In general this breaks the internal state of the servo since
the assumption is that it is always controlling the clock.

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran

Post by Miroslav Lichvar
Is it not possible that the offset will change when there is some
restart or reconfiguration in the PTP network?

Yes, but any phase offset that suddenly appears is fixed by a jump.

Is that always preferred over slew?

I wouldn't say "always", because you never know what people want.
Maybe it wouldn't hurt to allow the frequency adjustment in some
cases. But then, the user can just not set this option.

Post by Miroslav Lichvar
Ok, but adjusting frequency to correct a phase offset is still
fine, right?

To be honest, I didn't even try that with my hardware setup. I went
on the assumption that the frequency is already locked, and any
adjustment can only be a mistake.

Post by Miroslav Lichvar
Hm, so the offset normally stays at zero or is it stepping on each
update?

It always stays zero. Non-zero offsets only appear in error
conditions, like resetting the GM's time or breaking the SyncE chain.

Post by Miroslav Lichvar
If there is no noise in the measurements, then one step on start and no
more clock updates would make sense.

That is basically what I really want to do, but without rewriting ptp4l.

Post by Miroslav Lichvar
If there is some noise I think it would
be still useful to let the servos adjust the frequency similarly to a
case with a stabilized clock (e.g. OCXO), which has a fixed but
non-zero frequency offset.
In seems odd to me to call the servo function and then ignore its
result. In general this breaks the internal state of the servo since
the assumption is that it is always controlling the clock.

But a new servo type would make sense to you?

Thanks,
Richard

------------------------------------------------------------------------------

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
Hm, so the offset normally stays at zero or is it stepping on each
update?

It always stays zero. Non-zero offsets only appear in error
conditions, like resetting the GM's time or breaking the SyncE chain.

Interesting. Is the measured offset zero because it's a feature of the
SyncE design, or is it just so small that it's not visible in the 1ns
resolution?

Post by Richard Cochran

Post by Miroslav Lichvar
In seems odd to me to call the servo function and then ignore its
result. In general this breaks the internal state of the servo since
the assumption is that it is always controlling the clock.

But a new servo type would make sense to you?

I think it would. It could be a "dumb" servo that would step any
offset and return with locked state when the offset is below the value
set by the step_threshold option.

Or it could be a simple P controller, which would by design return
zero frequency offset when the phase offset is zero. Or the current PI
servo could be extended to support zero I constant.

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran

Post by Miroslav Lichvar
Hm, so the offset normally stays at zero or is it stepping on each
update?

It always stays zero. Non-zero offsets only appear in error
conditions, like resetting the GM's time or breaking the SyncE chain.

Interesting. Is the measured offset zero because it's a feature of the
SyncE design, or is it just so small that it's not visible in the 1ns
resolution?

Getting back to this question, there is an error, but it is simply too
small to be measured with the ns resolution.

Thanks,
Richard

------------------------------------------------------------------------------

[Richard Cochran]

This patch adds support for changing the priority1 and priority2
elements of the "default data set" at run time.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/clock.c b/clock.c
index db0b0da..0519839 100644
--- a/clock.c
+++ b/clock.c
@@ -484,12 +484,25 @@ static int clock_management_set(struct clock *c, struct port *p,
{
int respond = 0;
struct management_tlv *tlv;
+ struct management_tlv_datum *mtd;
struct grandmaster_settings_np *gsn;
struct subscribe_events_np *sen;

tlv = (struct management_tlv *) req->management.suffix;

switch (id) {
+ case TLV_PRIORITY1:
+ mtd = (struct management_tlv_datum *) tlv->data;
+ c->dds.priority1 = mtd->val;
+ *changed = 1;
+ respond = 1;
+ break;
+ case TLV_PRIORITY2:
+ mtd = (struct management_tlv_datum *) tlv->data;
+ c->dds.priority2 = mtd->val;
+ *changed = 1;
+ respond = 1;
+ break;
case TLV_GRANDMASTER_SETTINGS_NP:
gsn = (struct grandmaster_settings_np *) tlv->data;
c->dds.clockQuality = gsn->clockQuality;

[Richard Cochran]

This patch lets the pmc program change the priority1 and priority2
elements of the "default data set" at run time.

Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/pmc.c b/pmc.c
index d58e190..c21911c 100644
--- a/pmc.c
+++ b/pmc.c
@@ -66,8 +66,8 @@ struct management_id idtab[] = {
{ "CURRENT_DATA_SET", TLV_CURRENT_DATA_SET, do_get_action },
{ "PARENT_DATA_SET", TLV_PARENT_DATA_SET, do_get_action },
{ "TIME_PROPERTIES_DATA_SET", TLV_TIME_PROPERTIES_DATA_SET, do_get_action },
- { "PRIORITY1", TLV_PRIORITY1, do_get_action },
- { "PRIORITY2", TLV_PRIORITY2, do_get_action },
+ { "PRIORITY1", TLV_PRIORITY1, do_set_action },
+ { "PRIORITY2", TLV_PRIORITY2, do_set_action },
{ "DOMAIN", TLV_DOMAIN, do_get_action },
{ "SLAVE_ONLY", TLV_SLAVE_ONLY, do_get_action },
{ "TIME", TLV_TIME, not_supported },
@@ -493,6 +493,7 @@ static void do_get_action(int action, int index, char *str)
static void do_set_action(int action, int index, char *str)
{
struct grandmaster_settings_np gsn;
+ struct management_tlv_datum mtd;
struct port_ds_np pnp;
int cnt, code = idtab[index].code;
int leap_61, leap_59, utc_off_valid;
@@ -513,6 +514,24 @@ static void do_set_action(int action, int index, char *str)
return;
}
switch (code) {
+ case TLV_PRIORITY1:
+ cnt = sscanf(str, " %*s %*s %hhu", &mtd.val);
+ if (cnt != 1) {
+ fprintf(stderr, "%s SET needs 1 value\n",
+ idtab[index].name);
+ break;
+ }
+ pmc_send_set_action(pmc, code, &mtd, sizeof(mtd));
+ break;
+ case TLV_PRIORITY2:
+ cnt = sscanf(str, " %*s %*s %hhu", &mtd.val);
+ if (cnt != 1) {
+ fprintf(stderr, "%s SET needs 1 value\n",
+ idtab[index].name);
+ break;
+ }
+ pmc_send_set_action(pmc, code, &mtd, sizeof(mtd));
+ break;
case TLV_GRANDMASTER_SETTINGS_NP:
cnt = sscanf(str, " %*s %*s "
"clockClass %hhu "

[Richard Cochran]

Post by Richard Cochran
@@ -1361,6 +1366,9 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t origin)
if (tsproc_update_offset(c->tsproc, &c->master_offset, &weight))
return state;
+ if (!c->have_pd)
+ return state;

Hm, this code was developed out-of-tree, before the tsproc stuff, and
I just mindlessly rebased it on top. Looks like the test in
tsproc_update_offset() makes this patch obsolete.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Richard Cochran
@@ -1361,6 +1366,9 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t origin)
if (tsproc_update_offset(c->tsproc, &c->master_offset, &weight))
return state;
+ if (!c->have_pd)
+ return state;

Hm, this code was developed out-of-tree, before the tsproc stuff, and
I just mindlessly rebased it on top. Looks like the test in
tsproc_update_offset() makes this patch obsolete.

Yeah, I think with tsproc that bug is gone. I used to hit it in my
simulator occasionally, but didn't thought it was possible with real
hw.

[Richard Cochran]

Post by Miroslav Lichvar
Yeah, I think with tsproc that bug is gone. I used to hit it in my
simulator occasionally, but didn't thought it was possible with real
hw.

With SyncE over 0.5m cable, I get delay values of 0 or 1 ns. In fact,
using this setup I see 5 ns per meter of cable, and that matches the
expected physical signal delay over copper. So you can measure the
patch cable lengths using PTP/SyncE!

Useful? Maybe not, but it is still cool anyways.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran
With SyncE over 0.5m cable, I get delay values of 0 or 1 ns. In fact,
using this setup I see 5 ns per meter of cable, and that matches the
expected physical signal delay over copper. So you can measure the
patch cable lengths using PTP/SyncE!
Useful? Maybe not, but it is still cool anyways.

A similar feature with non-SyncE PTP could be measuring temperature of
the environment from the frequency offset of the clock :).