[Linuxptp-devel] [PATCHv2 2/4] Change label of frequency offset.

[Miroslav Lichvar]

Change the label of the frequency offset in the clock messages printed
by ptp4l and phc2sys from "adj" to "freq" to indicate it's a frequency
value.

Also, modify clock_no_adjust() to print the frequency offset instead of
the ratio and use PRId64 instead of lld to print int64_t values.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 11 +++++++----
phc2sys.c | 2 +-
2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/clock.c b/clock.c
index d9d44df..281adff 100644
--- a/clock.c
+++ b/clock.c
@@ -235,7 +235,7 @@ static int clock_master_lost(struct clock *c)
static enum servo_state clock_no_adjust(struct clock *c)
{
double fui;
- double ratio;
+ double ratio, freq;
tmv_t origin2;
struct freq_estimator *f = &c->fest;
enum servo_state state = SERVO_UNLOCKED;
@@ -276,9 +276,11 @@ static enum servo_state clock_no_adjust(struct clock *c)

ratio = tmv_dbl(tmv_sub(origin2, f->origin1)) /
tmv_dbl(tmv_sub(c->t2, f->ingress1));
+ freq = (1.0 - ratio) * 1e9;

- pr_info("master offset %10lld s%d ratio %.9f path delay %10lld",
- c->master_offset, state, ratio, c->path_delay);
+ pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
+ "path delay %9" PRId64,
+ c->master_offset, state, freq, c->path_delay);

fui = 1.0 + (c->status.cumulativeScaledRateOffset + 0.0) / POW2_41;

@@ -901,7 +903,8 @@ enum servo_state clock_synchronize(struct clock *c,

adj = servo_sample(c->servo, c->master_offset, ingress, &state);

- pr_info("master offset %10lld s%d adj %+7.0f path delay %10lld",
+ pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
+ "path delay %9" PRId64,
c->master_offset, state, adj, c->path_delay);

switch (state) {
diff --git a/phc2sys.c b/phc2sys.c
index 058feaa..b6be26d 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -174,7 +174,7 @@ static void update_clock(struct clock *clock, int64_t offset, uint64_t ts)
break;
}

- pr_info("%s %9" PRId64 " s%d %lld.%09llu adj %.2f",
+ pr_info("%s %9" PRId64 " s%d %lld.%09llu freq %+7.0f",
clock->source_label, offset, state,
ts / NS_PER_SEC, ts % NS_PER_SEC, ppb);
}

[Miroslav Lichvar]

If the delay is known, print it together with the offset and frequency.
Remove the time stamp from the output to fit it into 80 chars.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.c | 36 ++++++++++++++++++++++--------------
sysoff.c | 16 ++++++++++------
sysoff.h | 4 +++-
3 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index b6be26d..2b2f42a 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -121,7 +121,7 @@ static void clock_step(clockid_t clkid, int64_t ns)
}

static int read_phc(clockid_t clkid, clockid_t sysclk, int readings,
- int64_t *offset, uint64_t *ts)
+ int64_t *offset, uint64_t *ts, int64_t *delay)
{
struct timespec tdst1, tdst2, tsrc;
int i;
@@ -146,6 +146,7 @@ static int read_phc(clockid_t clkid, clockid_t sysclk, int readings,
*ts = tdst2.tv_sec * NS_PER_SEC + tdst2.tv_nsec;
}
}
+ *delay = best_interval;

return 1;
}
@@ -156,7 +157,8 @@ struct clock {
const char *source_label;
};

-static void update_clock(struct clock *clock, int64_t offset, uint64_t ts)
+static void update_clock(struct clock *clock,
+ int64_t offset, uint64_t ts, int64_t delay)
{
enum servo_state state;
double ppb;
@@ -174,9 +176,14 @@ static void update_clock(struct clock *clock, int64_t offset, uint64_t ts)
break;
}

- pr_info("%s %9" PRId64 " s%d %lld.%09llu freq %+7.0f",
- clock->source_label, offset, state,
- ts / NS_PER_SEC, ts % NS_PER_SEC, ppb);
+ if (delay >= 0) {
+ pr_info("%s offset %9" PRId64 " s%d freq %+7.0f "
+ "delay %6" PRId64,
+ clock->source_label, offset, state, ppb, delay);
+ } else {
+ pr_info("%s offset %9" PRId64 " s%d freq %+7.0f",
+ clock->source_label, offset, state, ppb);
+ }
}

static int read_pps(int fd, int64_t *offset, uint64_t *ts)
@@ -204,7 +211,7 @@ static int read_pps(int fd, int64_t *offset, uint64_t *ts)
static int do_pps_loop(struct clock *clock, int fd,
clockid_t src, int n_readings, int sync_offset)
{
- int64_t pps_offset, phc_offset;
+ int64_t pps_offset, phc_offset, phc_delay;
uint64_t pps_ts, phc_ts;

clock->source_label = "pps";
@@ -218,7 +225,7 @@ static int do_pps_loop(struct clock *clock, int fd,
of seconds in the offset and PPS for the rest. */
if (src != CLOCK_INVALID) {
if (!read_phc(src, clock->clkid, n_readings,
- &phc_offset, &phc_ts))
+ &phc_offset, &phc_ts, &phc_delay))
return -1;

/* Convert the time stamp to the PHC time. */
@@ -236,7 +243,7 @@ static int do_pps_loop(struct clock *clock, int fd,
pps_offset -= sync_offset * NS_PER_SEC;
}

- update_clock(clock, pps_offset, pps_ts);
+ update_clock(clock, pps_offset, pps_ts, -1);
}
close(fd);
return 0;
@@ -246,19 +253,19 @@ static int do_sysoff_loop(struct clock *clock, clockid_t src,
int rate, int n_readings, int sync_offset)
{
uint64_t ts;
- int64_t offset;
+ int64_t offset, delay;
int err = 0, fd = CLOCKID_TO_FD(src);

clock->source_label = "sys";

while (1) {
usleep(1000000 / rate);
- if (sysoff_measure(fd, n_readings, &offset, &ts)) {
+ if (sysoff_measure(fd, n_readings, &offset, &ts, &delay)) {
err = -1;
break;
}
offset -= sync_offset * NS_PER_SEC;
- update_clock(clock, offset, ts);
+ update_clock(clock, offset, ts, delay);
}
return err;
}
@@ -267,17 +274,18 @@ static int do_phc_loop(struct clock *clock, clockid_t src,
int rate, int n_readings, int sync_offset)
{
uint64_t ts;
- int64_t offset;
+ int64_t offset, delay;

clock->source_label = "phc";

while (1) {
usleep(1000000 / rate);
- if (!read_phc(src, clock->clkid, n_readings, &offset, &ts)) {
+ if (!read_phc(src, clock->clkid, n_readings,
+ &offset, &ts, &delay)) {
continue;
}
offset -= sync_offset * NS_PER_SEC;
- update_clock(clock, offset, ts);
+ update_clock(clock, offset, ts, delay);
}
return 0;
}
diff --git a/sysoff.c b/sysoff.c
index 37a9664..f7b6240 100644
--- a/sysoff.c
+++ b/sysoff.c
@@ -52,7 +52,8 @@ static void insertion_sort(int length, int64_t interval, int64_t offset, uint64_
samples[i+1].timestamp = ts;
}

-static int64_t sysoff_estimate(struct ptp_clock_time *pct, int n_samples, uint64_t *ts)
+static int64_t sysoff_estimate(struct ptp_clock_time *pct, int n_samples,
+ uint64_t *ts, int64_t *delay)
{
int64_t t1, t2, tp;
int64_t interval, offset;
@@ -67,10 +68,12 @@ static int64_t sysoff_estimate(struct ptp_clock_time *pct, int n_samples, uint64
insertion_sort(i, interval, offset, (t2 + t1) / 2);
}
*ts = samples[0].timestamp;
+ *delay = samples[0].interval;
return samples[0].offset;
}

-int sysoff_measure(int fd, int n_samples, int64_t *result, uint64_t *ts)
+int sysoff_measure(int fd, int n_samples,
+ int64_t *result, uint64_t *ts, int64_t *delay)
{
struct ptp_sys_offset pso;
pso.n_samples = n_samples;
@@ -78,13 +81,13 @@ int sysoff_measure(int fd, int n_samples, int64_t *result, uint64_t *ts)
perror("ioctl PTP_SYS_OFFSET");
return SYSOFF_RUN_TIME_MISSING;
}
- *result = sysoff_estimate(pso.ts, n_samples, ts);
+ *result = sysoff_estimate(pso.ts, n_samples, ts, delay);
return SYSOFF_SUPPORTED;
}

int sysoff_probe(int fd, int n_samples)
{
- int64_t junk;
+ int64_t junk, delay;
uint64_t ts;

if (n_samples > PTP_MAX_SAMPLES) {
@@ -94,12 +97,13 @@ int sysoff_probe(int fd, int n_samples)
return SYSOFF_RUN_TIME_MISSING;
}

- return sysoff_measure(fd, n_samples, &junk, &ts);
+ return sysoff_measure(fd, n_samples, &junk, &ts, &delay);
}

#else /* !PTP_SYS_OFFSET */

-int sysoff_measure(int fd, int n_samples, int64_t *result, uint64_t *ts)
+int sysoff_measure(int fd, int n_samples,
+ int64_t *result, uint64_t *ts, int64_t *delay)
{
return SYSOFF_COMPILE_TIME_MISSING;
}
diff --git a/sysoff.h b/sysoff.h
index f2e481e..cb70265 100644
--- a/sysoff.h
+++ b/sysoff.h
@@ -39,6 +39,8 @@ int sysoff_probe(int fd, int n_samples);
* @param n_samples The number of consecutive readings to make.
* @param result The estimated offset in nanoseconds.
* @param ts The system time corresponding to the 'result'.
+ * @param delay The delay in reading of the clock in nanoseconds.
* @return One of the SYSOFF_ enumeration values.
*/
-int sysoff_measure(int fd, int n_samples, int64_t *result, uint64_t *ts);
+int sysoff_measure(int fd, int n_samples,
+ int64_t *result, uint64_t *ts, int64_t *delay);

[Richard Cochran]

Post by Miroslav Lichvar
If the delay is known, print it together with the offset and frequency.
Remove the time stamp from the output to fit it into 80 chars.
---

Applied.

Thanks,
Richard

[Miroslav Lichvar]

The clock_sync_interval() function is called when logSyncInterval
changes from zero. Call it also when the clock is created to have
fest.max_count set accordingly to freq_est_interval even with zero
logSyncInterval.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index dc93bdc..d9d44df 100644
--- a/clock.c
+++ b/clock.c
@@ -499,7 +499,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
c->pollfd[i].events = 0;
}

- c->fest.max_count = 2;
+ clock_sync_interval(c, 0);

for (i = 0; i < count; i++) {
c->fault_timeout[i] = iface[i].pod.fault_reset_interval;

[Richard Cochran]

Post by Miroslav Lichvar
The clock_sync_interval() function is called when logSyncInterval
changes from zero. Call it also when the clock is created to have
fest.max_count set accordingly to freq_est_interval even with zero
logSyncInterval.
---

Applied.

Thanks,
Richard

[Miroslav Lichvar]

Add new options to ptp4l and phc2sys to print summary statistics of the
clock instead of the individual samples.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
clock.h | 3 +-
config.c | 5 ++++
config.h | 1 +
default.cfg | 1 +
gPTP.cfg | 1 +
makefile | 8 +++---
phc2sys.8 | 11 ++++++++
phc2sys.c | 75 ++++++++++++++++++++++++++++++++++++++++++++-----
ptp4l.8 | 10 +++++++
ptp4l.c | 4 ++-
stats.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
stats.h | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
13 files changed, 353 insertions(+), 22 deletions(-)
create mode 100644 stats.c
create mode 100644 stats.h

diff --git a/clock.c b/clock.c
index 281adff..ccb1ece 100644
--- a/clock.c
+++ b/clock.c
@@ -31,6 +31,7 @@
#include "phc.h"
#include "port.h"
#include "servo.h"
+#include "stats.h"
#include "print.h"
#include "tlv.h"
#include "uds.h"
@@ -50,6 +51,13 @@ struct freq_estimator {
int count;
};

+struct clock_stats {
+ struct stats *offset;
+ struct stats *freq;
+ struct stats *delay;
+ int max_count;
+};
+
struct clock {
clockid_t clkid;
struct servo *servo;
@@ -80,6 +88,8 @@ struct clock {
tmv_t t1;
tmv_t t2;
struct clock_description desc;
+ struct clock_stats stats;
+ int stats_interval;
};

struct clock the_clock;
@@ -104,6 +114,9 @@ void clock_destroy(struct clock *c)
}
servo_destroy(c->servo);
mave_destroy(c->avg_delay);
+ stats_destroy(c->stats.offset);
+ stats_destroy(c->stats.freq);
+ stats_destroy(c->stats.delay);
memset(c, 0, sizeof(*c));
msg_cleanup();
}
@@ -232,6 +245,40 @@ static int clock_master_lost(struct clock *c)
return 1;
}

+static void clock_stats_update(struct clock_stats *s,
+ int64_t offset, double freq)
+{
+ struct stats_result offset_stats, freq_stats, delay_stats;
+
+ stats_add_value(s->offset, offset);
+ stats_add_value(s->freq, freq);
+
+ if (stats_get_num_values(s->offset) < s->max_count)
+ return;
+
+ stats_get_result(s->offset, &offset_stats);
+ stats_get_result(s->freq, &freq_stats);
+
+ /* Path delay stats are updated separately, they may be empty. */
+ if (!stats_get_result(s->delay, &delay_stats)) {
+ pr_info("rms %4.0f max %4.0f "
+ "freq %+6.0f +/- %3.0f "
+ "delay %5.0f +/- %3.0f",
+ offset_stats.rms, offset_stats.max_abs,
+ freq_stats.mean, freq_stats.stddev,
+ delay_stats.mean, delay_stats.stddev);
+ } else {
+ pr_info("rms %4.0f max %4.0f "
+ "freq %+6.0f +/- %3.0f",
+ offset_stats.rms, offset_stats.max_abs,
+ freq_stats.mean, freq_stats.stddev);
+ }
+
+ stats_reset(s->offset);
+ stats_reset(s->freq);
+ stats_reset(s->delay);
+}
+
static enum servo_state clock_no_adjust(struct clock *c)
{
double fui;
@@ -278,9 +325,13 @@ static enum servo_state clock_no_adjust(struct clock *c)
tmv_dbl(tmv_sub(c->t2, f->ingress1));
freq = (1.0 - ratio) * 1e9;

- pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
- "path delay %9" PRId64,
- c->master_offset, state, freq, c->path_delay);
+ if (c->stats.max_count > 1) {
+ clock_stats_update(&c->stats, c->master_offset, freq);
+ } else {
+ pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
+ "path delay %9" PRId64,
+ c->master_offset, state, freq, c->path_delay);
+ }

fui = 1.0 + (c->status.cumulativeScaledRateOffset + 0.0) / POW2_41;

@@ -433,7 +484,7 @@ UInteger8 clock_class(struct clock *c)

struct clock *clock_create(int phc_index, struct interface *iface, int count,
enum timestamp_type timestamping, struct default_ds *dds,
- enum servo_type servo)
+ enum servo_type servo, int stats_interval)
{
int i, fadj = 0, max_adj = 0.0, sw_ts = timestamping == TS_SOFTWARE ? 1 : 0;
struct clock *c = &the_clock;
@@ -486,6 +537,14 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
pr_err("Failed to create moving average");
return NULL;
}
+ c->stats_interval = stats_interval;
+ c->stats.offset = stats_create();
+ c->stats.freq = stats_create();
+ c->stats.delay = stats_create();
+ if (!c->stats.offset || !c->stats.freq || !c->stats.delay) {
+ pr_err("failed to create stats");
+ return NULL;
+ }

c->dds = dds->dds;

@@ -834,12 +893,18 @@ void clock_path_delay(struct clock *c, struct timespec req, struct timestamp rx,
c->cur.meanPathDelay = tmv_to_TimeInterval(c->path_delay);

pr_debug("path delay %10lld %10lld", c->path_delay, pd);
+
+ if (c->stats.delay)
+ stats_add_value(c->stats.delay, pd);
}

void clock_peer_delay(struct clock *c, tmv_t ppd, double nrr)
{
c->path_delay = ppd;
c->nrr = nrr;
+
+ if (c->stats.delay)
+ stats_add_value(c->stats.delay, ppd);
}

void clock_remove_fda(struct clock *c, struct port *p, struct fdarray fda)
@@ -903,9 +968,13 @@ enum servo_state clock_synchronize(struct clock *c,

adj = servo_sample(c->servo, c->master_offset, ingress, &state);

- pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
- "path delay %9" PRId64,
- c->master_offset, state, adj, c->path_delay);
+ if (c->stats.max_count > 1) {
+ clock_stats_update(&c->stats, c->master_offset, adj);
+ } else {
+ pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
+ "path delay %9" PRId64,
+ c->master_offset, state, adj, c->path_delay);
+ }

switch (state) {
case SERVO_UNLOCKED:
@@ -925,12 +994,17 @@ enum servo_state clock_synchronize(struct clock *c,

void clock_sync_interval(struct clock *c, int n)
{
- int shift = c->freq_est_interval - n;
+ int shift;

+ shift = c->freq_est_interval - n;
if (shift < 0)
shift = 0;
-
c->fest.max_count = (1 << shift);
+
+ shift = c->stats_interval - n;
+ if (shift < 0)
+ shift = 0;
+ c->stats.max_count = (1 << shift);
}

struct timePropertiesDS *clock_time_properties(struct clock *c)
diff --git a/clock.h b/clock.h
index 581fa14..6bc98cd 100644
--- a/clock.h
+++ b/clock.h
@@ -67,11 +67,12 @@ UInteger8 clock_class(struct clock *c);
* @param timestamping The timestamping mode for this clock.
* @param dds A pointer to a default data set for the clock.
* @param servo The servo that this clock will use.
+ * @param stats_interval Interval in which are printed clock statistics.
* @return A pointer to the single global clock instance.
*/
struct clock *clock_create(int phc_index, struct interface *iface, int count,
enum timestamp_type timestamping, struct default_ds *dds,
- enum servo_type servo);
+ enum servo_type servo, int stats_interval);

/**
* Obtains a clock's default data set.
diff --git a/config.c b/config.c
index e4b9c22..d0bbd63 100644
--- a/config.c
+++ b/config.c
@@ -378,6 +378,11 @@ static enum parser_result parse_global_setting(const char *option,
for (i = 0; i < OUI_LEN; i++)
cfg->dds.clock_desc.manufacturerIdentity[i] = oui[i];

+ } else if (!strcmp(option, "summary_interval")) {
+ if (1 != sscanf(value, "%d", &val))
+ return BAD_VALUE;
+ cfg->summary_interval = val;
+
} else
return NOT_PARSED;

diff --git a/config.h b/config.h
index 497d683..3af193c 100644
--- a/config.h
+++ b/config.h
@@ -76,6 +76,7 @@ struct config {
int print_level;
int use_syslog;
int verbose;
+ int summary_interval;
};

int config_read(char *name, struct config *cfg);
diff --git a/default.cfg b/default.cfg
index 10401ac..7b3e2f7 100644
--- a/default.cfg
+++ b/default.cfg
@@ -32,6 +32,7 @@ follow_up_info 0
tx_timestamp_retries 100
use_syslog 1
verbose 0
+summary_interval 0
#
# Servo Options
#
diff --git a/gPTP.cfg b/gPTP.cfg
index 3c47e28..ecd5f71 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -31,6 +31,7 @@ follow_up_info 1
tx_timestamp_retries 100
use_syslog 1
verbose 0
+summary_interval 0
#
# Servo options
#
diff --git a/makefile b/makefile
index 9f2b587..671de48 100644
--- a/makefile
+++ b/makefile
@@ -30,8 +30,8 @@ CFLAGS = -Wall $(VER) $(INC) $(DEBUG) $(FEAT_CFLAGS) $(EXTRA_CFLAGS)
LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
PRG = ptp4l pmc phc2sys hwstamp_ctl
OBJ = bmc.o clock.o config.o fsm.o ptp4l.o mave.o msg.o phc.o pi.o port.o \
- print.o raw.o servo.o sk.o tlv.o tmtab.o transport.o udp.o udp6.o uds.o util.o \
- version.o
+ print.o raw.o servo.o sk.o stats.o tlv.o tmtab.o transport.o udp.o udp6.o \
+ uds.o util.o version.o

OBJECTS = $(OBJ) hwstamp_ctl.o phc2sys.o pmc.o pmc_common.o sysoff.o
SRC = $(OBJECTS:.o=.c)
@@ -52,8 +52,8 @@ ptp4l: $(OBJ)
pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
udp6.o uds.o util.o version.o

-phc2sys: msg.o phc2sys.o pmc_common.o print.o pi.o servo.o raw.o sk.o sysoff.o \
- tlv.o transport.o udp.o udp6.o uds.o util.o version.o
+phc2sys: msg.o phc2sys.o pmc_common.o print.o pi.o servo.o raw.o sk.o stats.o \
+ sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o version.o

hwstamp_ctl: hwstamp_ctl.o version.o

diff --git a/phc2sys.8 b/phc2sys.8
index 578d4ab..0542ba7 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -27,6 +27,8 @@ phc2sys \- synchronize two clocks
] [
.BI \-O " offset"
] [
+.BI \-u " summary-updates"
+] [
.B \-w
] [
.BI \-l " print-level"
@@ -112,6 +114,15 @@ Without
.B \-w
the default is 0.
.TP
+.BI \-u " summary-updates"
+Specify the number of clock updates included in summary statistics. The
+statistics include offset root mean square (RMS), maximum absolute offset,
+frequency offset mean and standard deviation, and mean of the delay in clock
+readings and standard deviation. The units are nanoseconds and parts per
+billion (ppb). If zero, the individual samples are printed instead of the
+statistics. The messages are printed at the LOG_INFO level.
+The default is 0 (disabled).
+.TP
.B \-w
Wait until ptp4l is in a synchronized state.
.TP
diff --git a/phc2sys.c b/phc2sys.c
index 2b2f42a..479bb83 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -42,6 +42,7 @@
#include "print.h"
#include "servo.h"
#include "sk.h"
+#include "stats.h"
#include "sysoff.h"
#include "tlv.h"
#include "version.h"
@@ -155,8 +156,47 @@ struct clock {
clockid_t clkid;
struct servo *servo;
const char *source_label;
+ struct stats *offset_stats;
+ struct stats *freq_stats;
+ struct stats *delay_stats;
+ int stats_max_count;
};

+static void update_clock_stats(struct clock *clock,
+ int64_t offset, double freq, int64_t delay)
+{
+ struct stats_result offset_stats, freq_stats, delay_stats;
+
+ stats_add_value(clock->offset_stats, offset);
+ stats_add_value(clock->freq_stats, freq);
+ if (delay >= 0)
+ stats_add_value(clock->delay_stats, delay);
+
+ if (stats_get_num_values(clock->offset_stats) < clock->stats_max_count)
+ return;
+
+ stats_get_result(clock->offset_stats, &offset_stats);
+ stats_get_result(clock->freq_stats, &freq_stats);
+
+ if (!stats_get_result(clock->delay_stats, &delay_stats)) {
+ pr_info("rms %4.0f max %4.0f "
+ "freq %+6.0f +/- %3.0f "
+ "delay %5.0f +/- %3.0f",
+ offset_stats.rms, offset_stats.max_abs,
+ freq_stats.mean, freq_stats.stddev,
+ delay_stats.mean, delay_stats.stddev);
+ } else {
+ pr_info("rms %4.0f max %4.0f "
+ "freq %+6.0f +/- %3.0f",
+ offset_stats.rms, offset_stats.max_abs,
+ freq_stats.mean, freq_stats.stddev);
+ }
+
+ stats_reset(clock->offset_stats);
+ stats_reset(clock->freq_stats);
+ stats_reset(clock->delay_stats);
+}
+
static void update_clock(struct clock *clock,
int64_t offset, uint64_t ts, int64_t delay)
{
@@ -176,13 +216,17 @@ static void update_clock(struct clock *clock,
break;
}

- if (delay >= 0) {
- pr_info("%s offset %9" PRId64 " s%d freq %+7.0f "
- "delay %6" PRId64,
- clock->source_label, offset, state, ppb, delay);
+ if (clock->offset_stats) {
+ update_clock_stats(clock, offset, ppb, delay);
} else {
- pr_info("%s offset %9" PRId64 " s%d freq %+7.0f",
- clock->source_label, offset, state, ppb);
+ if (delay >= 0) {
+ pr_info("%s offset %9" PRId64 " s%d freq %+7.0f "
+ "delay %6" PRId64,
+ clock->source_label, offset, state, ppb, delay);
+ } else {
+ pr_info("%s offset %9" PRId64 " s%d freq %+7.0f",
+ clock->source_label, offset, state, ppb);
+ }
}
}

@@ -423,6 +467,7 @@ static void usage(char *progname)
" -R [rate] slave clock update rate in HZ (1)\n"
" -N [num] number of master clock readings per update (5)\n"
" -O [offset] slave-master time offset (0)\n"
+ " -u [num] number of clock updates in summary stats (0)\n"
" -w wait for ptp4l\n"
" -h prints this message and exits\n"
" -v prints the software version and exits\n"
@@ -446,7 +491,8 @@ int main(int argc, char *argv[])
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
- while (EOF != (c = getopt(argc, argv, "c:d:hs:P:I:S:R:N:O:i:wl:mqv"))) {
+ while (EOF != (c = getopt(argc, argv,
+ "c:d:hs:P:I:S:R:N:O:i:u:wl:mqv"))) {
switch (c) {
case 'c':
dst_clock.clkid = clock_open(optarg);
@@ -484,6 +530,9 @@ int main(int argc, char *argv[])
case 'i':
ethdev = optarg;
break;
+ case 'u':
+ dst_clock.stats_max_count = atoi(optarg);
+ break;
case 'w':
wait_sync = 1;
break;
@@ -529,6 +578,18 @@ int main(int argc, char *argv[])
return -1;
}

+ if (dst_clock.stats_max_count > 0) {
+ dst_clock.offset_stats = stats_create();
+ dst_clock.freq_stats = stats_create();
+ dst_clock.delay_stats = stats_create();
+ if (!dst_clock.offset_stats ||
+ !dst_clock.freq_stats ||
+ !dst_clock.delay_stats) {
+ fprintf(stderr, "failed to create stats");
+ return -1;
+ }
+ }
+
print_set_progname(progname);
print_set_verbose(verbose);
print_set_syslog(use_syslog);
diff --git a/ptp4l.8 b/ptp4l.8
index 370b7cf..3ee222d 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -307,6 +307,16 @@ The default is 0 (disabled).
Print messages to the system log if enabled.
The default is 1 (enabled).
.TP
+.B summary_interval
+The time interval in which are printed summary statistics of the clock. It is
+specified as a power of two in seconds. The statistics include offset root mean
+square (RMS), maximum absolute offset, frequency offset mean and standard
+deviation, and path delay mean and standard deviation. The units are
+nanoseconds and parts per billion (ppb). If there is only one clock update in
+the interval, the sample will be printed instead of the statistics. The
+messages are printed at the LOG_INFO level.
+The default is 0 (1 second).
+.TP
.B time_stamping
The time stamping method. The allowed values are hardware, software and legacy.
The default is hardware.
diff --git a/ptp4l.c b/ptp4l.c
index 3da3388..2a8eeb0 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -99,6 +99,7 @@ static struct config cfg_settings = {
.print_level = LOG_INFO,
.use_syslog = 1,
.verbose = 0,
+ .summary_interval = 0,

.cfg_ignore = 0,
};
@@ -351,7 +352,8 @@ int main(int argc, char *argv[])

clock = clock_create(phc_index, iface, cfg_settings.nports,
*timestamping, &cfg_settings.dds,
- cfg_settings.clock_servo);
+ cfg_settings.clock_servo,
+ cfg_settings.summary_interval);
if (!clock) {
fprintf(stderr, "failed to create a clock\n");
return -1;
diff --git a/stats.c b/stats.c
new file mode 100644
index 0000000..cc8f72d
--- /dev/null
+++ b/stats.c
@@ -0,0 +1,87 @@
+/**
+ * @file stats.c
+ * @note Copyright (C) 2013 Miroslav Lichvar <***@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#include <math.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "stats.h"
+
+struct stats {
+ int num;
+ double min;
+ double max;
+ double mean;
+ double sum_sqr;
+ double sum_diff_sqr;
+};
+
+struct stats *stats_create()
+{
+ struct stats *stats;
+
+ stats = calloc(1, sizeof *stats);
+ return stats;
+}
+
+void stats_destroy(struct stats *stats)
+{
+ free(stats);
+}
+
+void stats_add_value(struct stats *stats,
+ double value)
+{
+ double old_mean = stats->mean;
+
+ if (!stats->num || stats->max < value)
+ stats->max = value;
+ if (!stats->num || stats->min > value)
+ stats->min = value;
+
+ stats->num++;
+ stats->mean = old_mean + (value - old_mean) / stats->num;
+ stats->sum_sqr += value * value;
+ stats->sum_diff_sqr += (value - old_mean) * (value - stats->mean);
+}
+
+int stats_get_num_values(struct stats *stats)
+{
+ return stats->num;
+}
+
+int stats_get_result(struct stats *stats,
+ struct stats_result *result)
+{
+ if (!stats->num)
+ return -1;
+
+ result->min = stats->min;
+ result->max = stats->max;
+ result->max_abs = stats->max > -stats->min ? stats->max : -stats->min;
+ result->mean = stats->mean;
+ result->rms = sqrt(stats->sum_sqr / stats->num);
+ result->stddev = sqrt(stats->sum_diff_sqr / stats->num);
+
+ return 0;
+}
+
+void stats_reset(struct stats *stats)
+{
+ memset(stats, 0, sizeof *stats);
+}
diff --git a/stats.h b/stats.h
new file mode 100644
index 0000000..2e70f8f
--- /dev/null
+++ b/stats.h
@@ -0,0 +1,77 @@
+/**
+ * @file stats.h
+ * @brief Implements various statistics.
+ * @note Copyright (C) 2013 Miroslav Lichvar <***@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#ifndef HAVE_STATS_H
+#define HAVE_STATS_H
+
+/** Opaque type */
+struct stats;
+
+/**
+ * Create a new instance of statistics.
+ * @return A pointer to a new stats on success, NULL otherwise.
+ */
+struct stats *stats_create();
+
+/**
+ * Destroy an instance of stats.
+ * @param servo Pointer to stats obtained via @ref stats_create().
+ */
+void stats_destroy(struct stats *stats);
+
+/**
+ * Add a new value to the stats.
+ * @param stats Pointer to stats obtained via @ref stats_create().
+ * @param value The measured value.
+ */
+void stats_add_value(struct stats *stats,
+ double value);
+
+/**
+ * Get the number of values collected in the stats so far.
+ * @param stats Pointer to stats obtained via @ref stats_create().
+ * @return The number of values.
+ */
+int stats_get_num_values(struct stats *stats);
+
+struct stats_result {
+ double min;
+ double max;
+ double max_abs;
+ double mean;
+ double rms;
+ double stddev;
+};
+
+/**
+ * Obtain the results of the calculated statistics.
+ * @param stats Pointer to stats obtained via @ref stats_create().
+ * @param stats_result Pointer to stats_result to store the results.
+ * @return Zero on success, non-zero if no values were added.
+ */
+int stats_get_result(struct stats *stats,
+ struct stats_result *result);
+
+/**
+ * Reset all statistics.
+ * @param stats Pointer to stats obtained via @ref stats_create().
+ */
+void stats_reset(struct stats *stats);
+
+#endif

[Richard Cochran]

I have a few nits to pick, below.

Post by Miroslav Lichvar
diff --git a/config.h b/config.h
index 497d683..3af193c 100644
--- a/config.h
+++ b/config.h
@@ -76,6 +76,7 @@ struct config {
int print_level;
int use_syslog;
int verbose;
+ int summary_interval;

Put this new field into 'struct default_ds' and then there will be no
need to change the signature of clock_create().

Post by Miroslav Lichvar
};
int config_read(char *name, struct config *cfg);

...

Post by Miroslav Lichvar
diff --git a/stats.c b/stats.c
new file mode 100644
index 0000000..cc8f72d
--- /dev/null
+++ b/stats.c
@@ -0,0 +1,87 @@
+/**
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#include <math.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "stats.h"
+
+struct stats {
+ int num;

Maybe use unsigned, or better yet uint64_t?

Post by Miroslav Lichvar
+ double min;
+ double max;
+ double mean;
+ double sum_sqr;
+ double sum_diff_sqr;
+};
+
+struct stats *stats_create()
+{
+ struct stats *stats;
+
+ stats = calloc(1, sizeof *stats);
+ return stats;
+}
+
+void stats_destroy(struct stats *stats)
+{
+ free(stats);
+}
+
+void stats_add_value(struct stats *stats,
+ double value)
+{
+ double old_mean = stats->mean;
+
+ if (!stats->num || stats->max < value)
+ stats->max = value;
+ if (!stats->num || stats->min > value)
+ stats->min = value;
+
+ stats->num++;

Should we check for overflow?

Post by Miroslav Lichvar
+ stats->mean = old_mean + (value - old_mean) / stats->num;
+ stats->sum_sqr += value * value;
+ stats->sum_diff_sqr += (value - old_mean) * (value - stats->mean);

Incremental, nice.

Post by Miroslav Lichvar
+}
+
+int stats_get_num_values(struct stats *stats)
+{
+ return stats->num;
+}
+
+int stats_get_result(struct stats *stats,
+ struct stats_result *result)
+{
+ if (!stats->num)
+ return -1;
+
+ result->min = stats->min;
+ result->max = stats->max;
+ result->max_abs = stats->max > -stats->min ? stats->max : -stats->min;
+ result->mean = stats->mean;
+ result->rms = sqrt(stats->sum_sqr / stats->num);
+ result->stddev = sqrt(stats->sum_diff_sqr / stats->num);
+
+ return 0;
+}
+
+void stats_reset(struct stats *stats)
+{
+ memset(stats, 0, sizeof *stats);
+}
diff --git a/stats.h b/stats.h
new file mode 100644
index 0000000..2e70f8f
--- /dev/null
+++ b/stats.h
@@ -0,0 +1,77 @@
+/**
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#ifndef HAVE_STATS_H
+#define HAVE_STATS_H
+
+/** Opaque type */
+struct stats;
+
+/**
+ * Create a new instance of statistics.
+ */
+struct stats *stats_create();

This prototype, with empty () means "do not check the arguments" which
is surely not what you meant. This is different in the C language than
in C++. Writing f(void) means that the function takes no arguments.

Post by Miroslav Lichvar
+
+/**
+ * Destroy an instance of stats.
+ */
+void stats_destroy(struct stats *stats);
+
+/**
+ * Add a new value to the stats.
+ */
+void stats_add_value(struct stats *stats,
+ double value);

Over-zealous line break.

Post by Miroslav Lichvar
+
+/**
+ * Get the number of values collected in the stats so far.
+ */
+int stats_get_num_values(struct stats *stats);
+
+struct stats_result {
+ double min;
+ double max;
+ double max_abs;
+ double mean;
+ double rms;
+ double stddev;
+};
+
+/**
+ * Obtain the results of the calculated statistics.
+ */
+int stats_get_result(struct stats *stats,
+ struct stats_result *result);

Here, too.

Post by Miroslav Lichvar
+
+/**
+ * Reset all statistics.
+ */
+void stats_reset(struct stats *stats);
+
+#endif
--
1.8.1

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran

Post by Miroslav Lichvar
+struct stats {
+ int num;

Maybe use unsigned, or better yet uint64_t?

Okay, now that I am running the code and seeing how to use the option,
I understand that 'num' only counts up to the number of sync periods
in the averaging interval.

But I still would say, use unsigned int.

Post by Richard Cochran

Post by Miroslav Lichvar
+ double min;
+ double max;
+ double mean;
+ double sum_sqr;
+ double sum_diff_sqr;
+};
+void stats_add_value(struct stats *stats,
+ double value)
+{
+ double old_mean = stats->mean;
+
+ if (!stats->num || stats->max < value)
+ stats->max = value;
+ if (!stats->num || stats->min > value)
+ stats->min = value;
+
+ stats->num++;

Should we check for overflow?

So, no need to check for overflow, but we should do a sanity check on
the 'summary_interval' value from the config file, to make sure it
fits.

Overall, I really like the summary option, as it makes way more sense
than spamming the syslog with the instantaneous offset every second.

Thanks,
Richard

[Keller, Jacob E]

-----Original Message-----
Sent: Friday, February 08, 2013 11:10 AM
To: Miroslav Lichvar
Subject: Re: [Linuxptp-devel] [PATCHv2 4/4] Add summary statistics.
Overall, I really like the summary option, as it makes way more sense
than spamming the syslog with the instantaneous offset every second.
Thanks,
Richard

Agreed!!

- Jake

[Richard Cochran]

Post by Richard Cochran

Post by Richard Cochran

Post by Miroslav Lichvar
+struct stats {
+ int num;

Maybe use unsigned, or better yet uint64_t?

Okay, now that I am running the code and seeing how to use the option,
I understand that 'num' only counts up to the number of sync periods
in the averaging interval.
But I still would say, use unsigned int.

And to be consistent, I will fix up the max_count fields to be
unsigned.

Post by Richard Cochran
So, no need to check for overflow, but we should do a sanity check on
the 'summary_interval' value from the config file, to make sure it
fits.

I think we can just add bounds checking into clock_sync_interval(),
and issue a warning if the shift value is out of range.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Richard Cochran
Okay, now that I am running the code and seeing how to use the option,
I understand that 'num' only counts up to the number of sync periods
in the averaging interval.
But I still would say, use unsigned int.

And to be consistent, I will fix up the max_count fields to be
unsigned.

Should that include max_count in the clock_stats struct in clock.c and
stats_max_count in phc2sys.c?

Post by Richard Cochran

Post by Richard Cochran
So, no need to check for overflow, but we should do a sanity check on
the 'summary_interval' value from the config file, to make sure it
fits.

I think we can just add bounds checking into clock_sync_interval(),
and issue a warning if the shift value is out of range.

Check that max_count after the shift doesn't overflow? I think
enforcing a reasonable maximum for summary_interval (and perhaps all
other log based options) in the config parser would work well too.

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran

Post by Richard Cochran
Okay, now that I am running the code and seeing how to use the option,
I understand that 'num' only counts up to the number of sync periods
in the averaging interval.
But I still would say, use unsigned int.

And to be consistent, I will fix up the max_count fields to be
unsigned.

Should that include max_count in the clock_stats struct in clock.c and
stats_max_count in phc2sys.c?

Yes, it should.

Post by Miroslav Lichvar

Post by Richard Cochran

Post by Richard Cochran
So, no need to check for overflow, but we should do a sanity check on
the 'summary_interval' value from the config file, to make sure it
fits.

I think we can just add bounds checking into clock_sync_interval(),
and issue a warning if the shift value is out of range.

Check that max_count after the shift doesn't overflow? I think
enforcing a reasonable maximum for summary_interval (and perhaps all
other log based options) in the config parser would work well too.

We set, for example,

c->fest.max_count = (1 << shift);

so we need to check that (shift < sizeof(int) * 8).

Thanks,
Richard

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 2 +-
phc2sys.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/clock.c b/clock.c
index f5836fd..974b37c 100644
--- a/clock.c
+++ b/clock.c
@@ -55,7 +55,7 @@ struct clock_stats {
struct stats *offset;
struct stats *freq;
struct stats *delay;
- int max_count;
+ unsigned int max_count;
};

struct clock {
diff --git a/phc2sys.c b/phc2sys.c
index 479bb83..582492d 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -159,7 +159,7 @@ struct clock {
struct stats *offset_stats;
struct stats *freq_stats;
struct stats *delay_stats;
- int stats_max_count;
+ unsigned int stats_max_count;
};

static void update_clock_stats(struct clock *clock,

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 8 ++++++++
port.c | 4 ++++
2 files changed, 12 insertions(+)

diff --git a/clock.c b/clock.c
index 974b37c..e4e4f30 100644
--- a/clock.c
+++ b/clock.c
@@ -999,11 +999,19 @@ void clock_sync_interval(struct clock *c, int n)
shift = c->freq_est_interval - n;
if (shift < 0)
shift = 0;
+ else if (shift >= sizeof(int) * 8) {
+ shift = sizeof(int) * 8 - 1;
+ pr_warning("freq_est_interval is too long");
+ }
c->fest.max_count = (1 << shift);

shift = c->stats_interval - n;
if (shift < 0)
shift = 0;
+ else if (shift >= sizeof(int) * 8) {
+ shift = sizeof(int) * 8 - 1;
+ pr_warning("summary_interval is too long");
+ }
c->stats.max_count = (1 << shift);
}

diff --git a/port.c b/port.c
index 1438808..9899687 100644
--- a/port.c
+++ b/port.c
@@ -551,6 +551,10 @@ static void port_nrate_initialize(struct port *p)

if (shift < 0)
shift = 0;
+ else if (shift >= sizeof(int) * 8) {
+ shift = sizeof(int) * 8 - 1;
+ pr_warning("freq_est_interval is too long");
+ }

/* We start in the 'incapable' state. */
p->pdr_missing = ALLOWED_LOST_RESPONSES + 1;

[Richard Cochran]

Post by Miroslav Lichvar
Add new options to ptp4l and phc2sys to print summary statistics of the
clock instead of the individual samples.

I went ahead and applied this, fixing up a few of things that I
comment on.

Thanks,
Richard

[Richard Cochran]

Post by Miroslav Lichvar
Change the label of the frequency offset in the clock messages printed
by ptp4l and phc2sys from "adj" to "freq" to indicate it's a frequency
value.
Also, modify clock_no_adjust() to print the frequency offset instead of
the ratio and use PRId64 instead of lld to print int64_t values.
---

Applied.

Thanks,
Richard