[Linuxptp-devel] [PATCH RFC 1/6] Use a consistent frequency estimation interval in the PI controller.

[Richard Cochran]

As it now stands, the frequency offset from the master clock is guessed
by using one sync interval. When the interval is shorter than one second,
this does not work any more when using hardware time stamping, and
software time stamping requires an even longer interval. This patch
fixes the issue by always using an interval at least one second long
(and 16 seconds in the case of SW time stamping).

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

diff --git a/pi.c b/pi.c
index 427cb3e..4671b11 100644
--- a/pi.c
+++ b/pi.c
@@ -47,6 +47,7 @@ struct pi_servo {
double ki;
double max_offset;
int count;
+ int hwts;
};

static void pi_destroy(struct servo *servo)
@@ -60,7 +61,7 @@ static double pi_sample(struct servo *servo,
uint64_t local_ts,
enum servo_state *state)
{
- double ki_term, ppb = 0.0;
+ double fest_interval, ki_term, ppb = 0.0;
struct pi_servo *s = container_of(servo, struct pi_servo, servo);

switch (s->count) {
@@ -79,6 +80,11 @@ static double pi_sample(struct servo *servo,
s->count = 0;
break;
}
+ /* Wait long enough before estimating the frequency offset. */
+ fest_interval = s->hwts ? 1.0 : 16.0;
+ if ((s->local[1] - s->local[0]) / 1e9 - fest_interval > 0.001) {
+ break;
+ }

s->drift += (s->offset[1] - s->offset[0]) * 1e9 /
(s->local[1] - s->local[0]);
@@ -133,6 +139,7 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->servo.sample = pi_sample;
s->drift = fadj;
s->maxppb = max_ppb;
+ s->hwts = sw_ts ? 0 : 1;

if (configured_pi_kp && configured_pi_ki) {
s->kp = configured_pi_kp;

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
pi.c | 15 +++++++++++++++
pi.h | 9 +++++++++
2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/pi.c b/pi.c
index 931592e..c8943aa 100644
--- a/pi.c
+++ b/pi.c
@@ -244,3 +244,18 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)

return &s->servo;
}
+
+int pi_user_constant(int sync, double kp, double ki)
+{
+ struct pi_const *k;
+ if (sync < PI_MIN_SYNC_INTERVAL || sync > PI_MAX_SYNC_INTERVAL) {
+ return -1;
+ }
+ if (kp < 0.0 || ki < 0.0) {
+ return -1;
+ }
+ k = &pi_table[ sync2index(sync) ];
+ k->p = kp;
+ k->i = ki;
+ return 0;
+}
diff --git a/pi.h b/pi.h
index 954f495..ce6843c 100644
--- a/pi.h
+++ b/pi.h
@@ -50,4 +50,13 @@ extern int configured_pi_max_freq;

struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts);

+/**
+ * Install a user specified pair of constants for a particular sync interval.
+ * @param sync The logarithm base two of the sync interval.
+ * @param kp The proportional constant for the PI controller.
+ * @param ki The integral constant for the PI controller.
+ * @return Zero on success, or non-zero if the sync value is out of range.
+ */
+int pi_user_constant(int sync, double kp, double ki);
+
#endif

[Keller, Jacob E]

Richard,

-----Original Message-----
Sent: Thursday, May 16, 2013 10:40 PM
Subject: [Linuxptp-devel] [PATCH RFC 4/6] Provide a method to install a
user constant into the table.
---
pi.c | 15 +++++++++++++++
pi.h | 9 +++++++++
2 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/pi.c b/pi.c
index 931592e..c8943aa 100644
--- a/pi.c
+++ b/pi.c
@@ -244,3 +244,18 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
return &s->servo;
}
+
+int pi_user_constant(int sync, double kp, double ki)
+{
+ struct pi_const *k;
+ if (sync < PI_MIN_SYNC_INTERVAL || sync >
PI_MAX_SYNC_INTERVAL) {
+ return -1;
+ }
+ if (kp < 0.0 || ki < 0.0) {
+ return -1;
+ }
+ k = &pi_table[ sync2index(sync) ];
+ k->p = kp;
+ k->i = ki;
+ return 0;
+}
diff --git a/pi.h b/pi.h
index 954f495..ce6843c 100644
--- a/pi.h
+++ b/pi.h
@@ -50,4 +50,13 @@ extern int configured_pi_max_freq;
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts);
+/**
+ * Install a user specified pair of constants for a particular sync interval.
+ */
+int pi_user_constant(int sync, double kp, double ki);
+
#endif
--
1.7.2.5

You should make it clear this function has the ability to overwrite a standard calculated value..

- Jake

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 8 +++++++-
default.cfg | 5 +++++
2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index 3103ecd..5464d20 100644
--- a/config.c
+++ b/config.c
@@ -21,6 +21,7 @@
#include <ctype.h>
#include "config.h"
#include "ether.h"
+#include "pi.h"
#include "print.h"
#include "util.h"

@@ -189,7 +190,7 @@ static enum parser_result parse_global_setting(const char *option,
const char *value,
struct config *cfg)
{
- double df;
+ double df, df2;
int i, val, cfg_ignore = cfg->cfg_ignore;
UInteger16 u16;
UInteger8 u8;
@@ -275,6 +276,11 @@ static enum parser_result parse_global_setting(const char *option,
return BAD_VALUE;
*cfg->tx_timestamp_timeout = val;

+ } else if (!strcmp(option, "pi_const")) {
+ if (3 != sscanf(value, "%d %lf %lf", &val, &df, &df2) ||
+ pi_user_constant(val, df, df2))
+ return BAD_VALUE;
+
} else if (!strcmp(option, "pi_proportional_const")) {
if (1 != sscanf(value, "%lf", &df) || df < 0.0)
return BAD_VALUE;
diff --git a/default.cfg b/default.cfg
index f9007b1..1fdbbef 100644
--- a/default.cfg
+++ b/default.cfg
@@ -42,6 +42,11 @@ pi_proportional_const 0.0
pi_integral_const 0.0
pi_offset_const 0.0
pi_max_frequency 900000000
+# Custom PI constants
+# keyword Sync KP KI
+# pi_const 0 0.800000 0.200000
+# pi_const -1 1.131371 0.141421
+# pi_const -2 1.600000 0.100000
clock_servo pi
#
# Transport options

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 10 ----------
config.h | 2 --
default.cfg | 2 --
gPTP.cfg | 2 --
phc2sys.c | 12 ++++++------
pi.c | 13 +++----------
pi.h | 12 ------------
ptp4l.c | 2 --
8 files changed, 9 insertions(+), 46 deletions(-)

diff --git a/config.c b/config.c
index 5464d20..1163fa8 100644
--- a/config.c
+++ b/config.c
@@ -281,16 +281,6 @@ static enum parser_result parse_global_setting(const char *option,
pi_user_constant(val, df, df2))
return BAD_VALUE;

- } else if (!strcmp(option, "pi_proportional_const")) {
- if (1 != sscanf(value, "%lf", &df) || df < 0.0)
- return BAD_VALUE;
- *cfg->pi_proportional_const = df;
-
- } else if (!strcmp(option, "pi_integral_const")) {
- if (1 != sscanf(value, "%lf", &df) || df < 0.0)
- return BAD_VALUE;
- *cfg->pi_integral_const = df;
-
} else if (!strcmp(option, "pi_offset_const")) {
if (1 != sscanf(value, "%lf", &df) || df < 0.0)
return BAD_VALUE;
diff --git a/config.h b/config.h
index 901b50a..c78dbd1 100644
--- a/config.h
+++ b/config.h
@@ -65,8 +65,6 @@ struct config {

enum servo_type clock_servo;

- double *pi_proportional_const;
- double *pi_integral_const;
double *pi_offset_const;
int *pi_max_frequency;

diff --git a/default.cfg b/default.cfg
index 1fdbbef..31a7632 100644
--- a/default.cfg
+++ b/default.cfg
@@ -38,8 +38,6 @@ kernel_leap 1
#
# Servo Options
#
-pi_proportional_const 0.0
-pi_integral_const 0.0
pi_offset_const 0.0
pi_max_frequency 900000000
# Custom PI constants
diff --git a/gPTP.cfg b/gPTP.cfg
index ed134ff..be32247 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -37,8 +37,6 @@ kernel_leap 1
#
# Servo options
#
-pi_proportional_const 0.0
-pi_integral_const 0.0
pi_offset_const 0.0
pi_max_frequency 900000000
clock_servo pi
diff --git a/phc2sys.c b/phc2sys.c
index 1dbe2bc..20c85b6 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -576,7 +576,7 @@ int main(int argc, char *argv[])
int c, domain_number = 0, phc_readings = 5, pps_fd = -1;
int max_ppb, r, wait_sync = 0, forced_sync_offset = 0;
int print_level = LOG_INFO, use_syslog = 1, verbose = 0;
- double ppb, phc_interval = 1.0;
+ double kp = 0.0, ki = 0.0, ppb, phc_interval = 1.0;
struct timespec phc_interval_tp;
struct clock dst_clock = {
.clkid = CLOCK_REALTIME,
@@ -584,9 +584,6 @@ int main(int argc, char *argv[])
.kernel_leap = 1,
};

- configured_pi_kp = KP;
- configured_pi_ki = KI;
-
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
@@ -611,10 +608,10 @@ int main(int argc, char *argv[])
src = clock_open(optarg);
break;
case 'P':
- configured_pi_kp = atof(optarg);
+ kp = atof(optarg);
break;
case 'I':
- configured_pi_ki = atof(optarg);
+ ki = atof(optarg);
break;
case 'S':
configured_pi_offset = atof(optarg);
@@ -746,6 +743,9 @@ int main(int argc, char *argv[])
}
}

+ if (kp || ki) {
+ pi_user_constant(0, kp, ki);
+ }
dst_clock.servo = servo_create(CLOCK_SERVO_PI, -ppb, max_ppb, 0);

if (pps_fd >= 0)
diff --git a/pi.c b/pi.c
index c8943aa..14a5f06 100644
--- a/pi.c
+++ b/pi.c
@@ -81,8 +81,6 @@ static double sw_ki(double t)
#define NSEC_PER_SEC 1000000000

/* These take their values from the configuration file. (see ptp4l.c) */
-double configured_pi_kp = 0.0;
-double configured_pi_ki = 0.0;
double configured_pi_offset = 0.0;
int configured_pi_max_freq = 900000000;

@@ -223,14 +221,9 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)

pi_init_table(sw_ts);

- if (configured_pi_kp && configured_pi_ki) {
- s->kp = configured_pi_kp;
- s->ki = configured_pi_ki;
- } else {
- k = &pi_table[ sync2index(0) ];
- s->kp = k->p;
- s->ki = k->i;
- }
+ k = &pi_table[ sync2index(0) ];
+ s->kp = k->p;
+ s->ki = k->i;

if (configured_pi_offset > 0.0) {
s->max_offset = configured_pi_offset * NSEC_PER_SEC;
diff --git a/pi.h b/pi.h
index ce6843c..4158ae7 100644
--- a/pi.h
+++ b/pi.h
@@ -22,18 +22,6 @@
#include "servo.h"

/**
- * When set to a non-zero value, this variable determines the
- * proportional constant for the PI controller.
- */
-extern double configured_pi_kp;
-
-/**
- * When set to a non-zero value, this variable determines the
- * integral constant for the PI controller.
- */
-extern double configured_pi_ki;
-
-/**
* When set to a non-zero value, this variable controls the maximum allowed
* offset before a clock jump occurs instead of the default clock-slewing
* mechanism
diff --git a/ptp4l.c b/ptp4l.c
index ecaf9ed..a4420f1 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -93,8 +93,6 @@ static struct config cfg_settings = {

.clock_servo = CLOCK_SERVO_PI,

- .pi_proportional_const = &configured_pi_kp,
- .pi_integral_const = &configured_pi_ki,
.pi_offset_const = &configured_pi_offset,
.pi_max_frequency = &configured_pi_max_freq,

[Richard Cochran]

This patch adds a new servo method to let the algorithm know about the
master clock's reported sync message interval. This information can be
used by the servo to adapt its synchronization parameters.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 2 ++
pi.c | 5 +++++
servo.c | 5 +++++
servo.h | 7 +++++++
servo_private.h | 2 ++
5 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/clock.c b/clock.c
index eceedc5..d6e8bef 100644
--- a/clock.c
+++ b/clock.c
@@ -1098,6 +1098,8 @@ void clock_sync_interval(struct clock *c, int n)
pr_warning("summary_interval is too long");
}
c->stats.max_count = (1 << shift);
+
+ servo_sync_interval(c->servo, n);
}

struct timePropertiesDS *clock_time_properties(struct clock *c)
diff --git a/pi.c b/pi.c
index 4671b11..4a57bab 100644
--- a/pi.c
+++ b/pi.c
@@ -127,6 +127,10 @@ static double pi_sample(struct servo *servo,
return ppb;
}

+static void pi_sync_interval(struct servo *servo, int n)
+{
+}
+
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
{
struct pi_servo *s;
@@ -137,6 +141,7 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)

s->servo.destroy = pi_destroy;
s->servo.sample = pi_sample;
+ s->servo.sync_interval = pi_sync_interval;
s->drift = fadj;
s->maxppb = max_ppb;
s->hwts = sw_ts ? 0 : 1;
diff --git a/servo.c b/servo.c
index a312ad1..a6fd299 100644
--- a/servo.c
+++ b/servo.c
@@ -41,3 +41,8 @@ double servo_sample(struct servo *servo,
{
return servo->sample(servo, offset, local_ts, state);
}
+
+void servo_sync_interval(struct servo *servo, int n)
+{
+ servo->sync_interval(servo, n);
+}
diff --git a/servo.h b/servo.h
index 4d4a8e7..4efa50e 100644
--- a/servo.h
+++ b/servo.h
@@ -86,4 +86,11 @@ double servo_sample(struct servo *servo,
uint64_t local_ts,
enum servo_state *state);

+/**
+ * Inform a clock servo about the master's sync interval.
+ * @param servo Pointer to a servo obtained via @ref servo_create().
+ * @param n The logarithm base two of the sync interval.
+ */
+void servo_sync_interval(struct servo *servo, int n);
+
#endif
diff --git a/servo_private.h b/servo_private.h
index f9c151f..2df49d2 100644
--- a/servo_private.h
+++ b/servo_private.h
@@ -28,6 +28,8 @@ struct servo {
double (*sample)(struct servo *servo,
int64_t offset, uint64_t local_ts,
enum servo_state *state);
+
+ void (*sync_interval)(struct servo *servo, int n);
};

#endif

[Keller, Jacob E]

-----Original Message-----
Sent: Thursday, May 16, 2013 10:40 PM
Subject: [Linuxptp-devel] [PATCH RFC 2/6] Let the clock servo know the
expected sync interval.
This patch adds a new servo method to let the algorithm know about the
master clock's reported sync message interval. This information can be
used by the servo to adapt its synchronization parameters.
---
clock.c | 2 ++
pi.c | 5 +++++
servo.c | 5 +++++
servo.h | 7 +++++++
servo_private.h | 2 ++
5 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/clock.c b/clock.c
index eceedc5..d6e8bef 100644
--- a/clock.c
+++ b/clock.c
@@ -1098,6 +1098,8 @@ void clock_sync_interval(struct clock *c, int n)
pr_warning("summary_interval is too long");
}
c->stats.max_count = (1 << shift);
+
+ servo_sync_interval(c->servo, n);
}
struct timePropertiesDS *clock_time_properties(struct clock *c)
diff --git a/pi.c b/pi.c
index 4671b11..4a57bab 100644
--- a/pi.c
+++ b/pi.c
@@ -127,6 +127,10 @@ static double pi_sample(struct servo *servo,
return ppb;
}
+static void pi_sync_interval(struct servo *servo, int n)
+{
+}
+
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
{
struct pi_servo *s;
@@ -137,6 +141,7 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->servo.destroy = pi_destroy;
s->servo.sample = pi_sample;
+ s->servo.sync_interval = pi_sync_interval;
s->drift = fadj;
s->maxppb = max_ppb;
s->hwts = sw_ts ? 0 : 1;
diff --git a/servo.c b/servo.c
index a312ad1..a6fd299 100644
--- a/servo.c
+++ b/servo.c
@@ -41,3 +41,8 @@ double servo_sample(struct servo *servo,
{
return servo->sample(servo, offset, local_ts, state);
}
+
+void servo_sync_interval(struct servo *servo, int n)
+{
+ servo->sync_interval(servo, n);
+}
diff --git a/servo.h b/servo.h
index 4d4a8e7..4efa50e 100644
--- a/servo.h
+++ b/servo.h
@@ -86,4 +86,11 @@ double servo_sample(struct servo *servo,
uint64_t local_ts,
enum servo_state *state);
+/**
+ * Inform a clock servo about the master's sync interval.
+ */
+void servo_sync_interval(struct servo *servo, int n);
+
#endif
diff --git a/servo_private.h b/servo_private.h
index f9c151f..2df49d2 100644
--- a/servo_private.h
+++ b/servo_private.h
@@ -28,6 +28,8 @@ struct servo {
double (*sample)(struct servo *servo,
int64_t offset, uint64_t local_ts,
enum servo_state *state);
+
+ void (*sync_interval)(struct servo *servo, int n);
};
#endif
--
1.7.2.5

This patch looks great.

- Jake

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
pi.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/pi.c b/pi.c
index 4a57bab..931592e 100644
--- a/pi.c
+++ b/pi.c
@@ -23,11 +23,60 @@
#include "pi.h"
#include "servo_private.h"

-#define HWTS_KP 0.7
-#define HWTS_KI 0.3
+#define PI_MIN_SYNC_INTERVAL -7
+#define PI_MAX_SYNC_INTERVAL 7

-#define SWTS_KP 0.1
-#define SWTS_KI 0.001
+#define PI_TABLE_SIZE (PI_MAX_SYNC_INTERVAL - PI_MIN_SYNC_INTERVAL + 1)
+
+struct pi_const {
+ double p;
+ double i;
+};
+
+static struct pi_const pi_table[PI_TABLE_SIZE];
+
+static int sync2index(int n)
+{
+ return n - PI_MIN_SYNC_INTERVAL;
+}
+
+#define AP .8
+#define AI .2
+#define BP (-0.5)
+#define BI ( 0.5)
+#define TT 1.0
+
+static double calc_kp(double t)
+{
+ if (t > TT) {
+ return .7 / t;
+ }
+ return AP * pow(t, BP);
+}
+
+static double calc_ki(double t)
+{
+ if (t > TT) {
+ return .38 / t;
+ }
+ return AI * pow(t, BI);
+}
+
+static double sw_kp(double t)
+{
+ if (t > TT) {
+ return .1 / t;
+ }
+ return .1 * pow(t, BP);
+}
+
+static double sw_ki(double t)
+{
+ if (t > TT) {
+ return .001 / t;
+ }
+ return .001 * pow(t, BI);
+}

#define NSEC_PER_SEC 1000000000

@@ -56,6 +105,31 @@ static void pi_destroy(struct servo *servo)
free(s);
}

+static void pi_init_table(int sw_ts)
+{
+ struct pi_const *k;
+ double t;
+ int s;
+
+ for (s = PI_MIN_SYNC_INTERVAL; s <= PI_MAX_SYNC_INTERVAL; s++) {
+ if (s < 0) {
+ t = 1.0 / (1 << -s);
+ } else {
+ t = (1 << s);
+ }
+ k = &pi_table[ sync2index(s) ];
+ if (!k->p && !k->i) {
+ if (sw_ts) {
+ k->p = sw_kp(t);
+ k->i = sw_ki(t);
+ } else {
+ k->p = calc_kp(t);
+ k->i = calc_ki(t);
+ }
+ }
+ }
+}
+
static double pi_sample(struct servo *servo,
int64_t offset,
uint64_t local_ts,
@@ -133,6 +207,7 @@ static void pi_sync_interval(struct servo *servo, int n)

struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
{
+ struct pi_const *k;
struct pi_servo *s;

s = calloc(1, sizeof(*s));
@@ -146,15 +221,15 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->maxppb = max_ppb;
s->hwts = sw_ts ? 0 : 1;

+ pi_init_table(sw_ts);
+
if (configured_pi_kp && configured_pi_ki) {
s->kp = configured_pi_kp;
s->ki = configured_pi_ki;
- } else if (sw_ts) {
- s->kp = SWTS_KP;
- s->ki = SWTS_KI;
} else {
- s->kp = HWTS_KP;
- s->ki = HWTS_KI;
+ k = &pi_table[ sync2index(0) ];
+ s->kp = k->p;
+ s->ki = k->i;
}

if (configured_pi_offset > 0.0) {

[Keller, Jacob E]

Comments inline.

-----Original Message-----
Sent: Thursday, May 16, 2013 10:40 PM
Subject: [Linuxptp-devel] [PATCH RFC 3/6] Add a table of PI constants,
with one entry per sync interval.
---
pi.c | 93
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 84 insertions(+), 9 deletions(-)
diff --git a/pi.c b/pi.c
index 4a57bab..931592e 100644
--- a/pi.c
+++ b/pi.c
@@ -23,11 +23,60 @@
#include "pi.h"
#include "servo_private.h"
-#define HWTS_KP 0.7
-#define HWTS_KI 0.3
+#define PI_MIN_SYNC_INTERVAL -7
+#define PI_MAX_SYNC_INTERVAL 7
-#define SWTS_KP 0.1
-#define SWTS_KI 0.001
+#define PI_TABLE_SIZE (PI_MAX_SYNC_INTERVAL -
PI_MIN_SYNC_INTERVAL + 1)
+
+struct pi_const {
+ double p;
+ double i;
+};
+
+static struct pi_const pi_table[PI_TABLE_SIZE];
+
+static int sync2index(int n)
+{
+ return n - PI_MIN_SYNC_INTERVAL;
+}
+
+#define AP .8
+#define AI .2
+#define BP (-0.5)
+#define BI ( 0.5)
+#define TT 1.0
+
+static double calc_kp(double t)
+{
+ if (t > TT) {
+ return .7 / t;
+ }
+ return AP * pow(t, BP);
+}
+
+static double calc_ki(double t)
+{
+ if (t > TT) {
+ return .38 / t;
+ }
+ return AI * pow(t, BI);
+}
+

For consistency since those are clearly the hw calc ones, the names for the hw and sw ki/kp functions should be the same..

+static double sw_kp(double t)
+{
+ if (t > TT) {
+ return .1 / t;
+ }
+ return .1 * pow(t, BP);
+}
+
+static double sw_ki(double t)
+{
+ if (t > TT) {
+ return .001 / t;
+ }
+ return .001 * pow(t, BI);
+}
#define NSEC_PER_SEC 1000000000
@@ -56,6 +105,31 @@ static void pi_destroy(struct servo *servo)
free(s);
}
+static void pi_init_table(int sw_ts)
+{
+ struct pi_const *k;
+ double t;
+ int s;
+
+ for (s = PI_MIN_SYNC_INTERVAL; s <= PI_MAX_SYNC_INTERVAL;
s++) {
+ if (s < 0) {
+ t = 1.0 / (1 << -s);
+ } else {
+ t = (1 << s);
+ }
+ k = &pi_table[ sync2index(s) ];
+ if (!k->p && !k->i) {
+ if (sw_ts) {
+ k->p = sw_kp(t);
+ k->i = sw_ki(t);
+ } else {
+ k->p = calc_kp(t);
+ k->i = calc_ki(t);
+ }
+ }
+ }
+}
+
static double pi_sample(struct servo *servo,
int64_t offset,
uint64_t local_ts,
@@ -133,6 +207,7 @@ static void pi_sync_interval(struct servo *servo, int n)
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
{
+ struct pi_const *k;
struct pi_servo *s;
s = calloc(1, sizeof(*s));
@@ -146,15 +221,15 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->maxppb = max_ppb;
s->hwts = sw_ts ? 0 : 1;
+ pi_init_table(sw_ts);
+
if (configured_pi_kp && configured_pi_ki) {
s->kp = configured_pi_kp;
s->ki = configured_pi_ki;
- } else if (sw_ts) {
- s->kp = SWTS_KP;
- s->ki = SWTS_KI;
} else {
- s->kp = HWTS_KP;
- s->ki = HWTS_KI;
+ k = &pi_table[ sync2index(0) ];
+ s->kp = k->p;
+ s->ki = k->i;
}
if (configured_pi_offset > 0.0) {
--
1.7.2.5

Otherwise this patch looks good, but I don't get the math, so I'll let Miroslav comment on that.

Thanks

- Jake

[Miroslav Lichvar]

Here is what I came up with after the recent discussion about adapting
the servo to the sync interval. I gave this some light testing,
including changing the sync on the fly. Comments are welcome.

Please wait a bit before including this set. I will have some comments
and I'd still like to reply in the other thread about default
constants, but I'm currently busy with other things. Hopefully next
week.

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar
Please wait a bit before including this set. I will have some comments
and I'd still like to reply in the other thread about default
constants, but I'm currently busy with other things. Hopefully next
week.

Okay, fine, there is no rush. I look forward to your feedback.

Thanks,
Richard

[Keller, Jacob E]

Richard,

Comments inline.

-----Original Message-----
Sent: Thursday, May 16, 2013 10:40 PM
Subject: [Linuxptp-devel] [PATCH RFC 1/6] Use a consistent frequency
estimation interval in the PI controller.
As it now stands, the frequency offset from the master clock is guessed
by using one sync interval. When the interval is shorter than one second,
this does not work any more when using hardware time stamping, and
software time stamping requires an even longer interval. This patch
fixes the issue by always using an interval at least one second long
(and 16 seconds in the case of SW time stamping).
---
pi.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/pi.c b/pi.c
index 427cb3e..4671b11 100644
--- a/pi.c
+++ b/pi.c
@@ -47,6 +47,7 @@ struct pi_servo {
double ki;
double max_offset;
int count;
+ int hwts;
};
static void pi_destroy(struct servo *servo)
@@ -60,7 +61,7 @@ static double pi_sample(struct servo *servo,
uint64_t local_ts,
enum servo_state *state)
{
- double ki_term, ppb = 0.0;
+ double fest_interval, ki_term, ppb = 0.0;
struct pi_servo *s = container_of(servo, struct pi_servo, servo);
switch (s->count) {
@@ -79,6 +80,11 @@ static double pi_sample(struct servo *servo,
s->count = 0;
break;
}
+ /* Wait long enough before estimating the frequency
offset. */
+ fest_interval = s->hwts ? 1.0 : 16.0;
+ if ((s->local[1] - s->local[0]) / 1e9 - fest_interval > 0.001) {
+ break;
+ }

I am confused by this check. Let's say that T0 is 10seconds, and T1 is 13seconds, we are doing software timestamping..

Then the result would be (13 - 10) - 16 = -13 which fails this condition so it quits. Unless I am missing something this will break only when the interval is more than fest_interval, and will continue otherwise. I don't see how this buys us anything? Maybe I don't understand the intent, or have the numbers messed up? Could you at least explain this conditional to me?

s->drift += (s->offset[1] - s->offset[0]) * 1e9 /
(s->local[1] - s->local[0]);
@@ -133,6 +139,7 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->servo.sample = pi_sample;
s->drift = fadj;
s->maxppb = max_ppb;
+ s->hwts = sw_ts ? 0 : 1;
if (configured_pi_kp && configured_pi_ki) {
s->kp = configured_pi_kp;
--
1.7.2.5
------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers
complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Richard Cochran]

Post by Keller, Jacob E

Post by Richard Cochran
+ /* Wait long enough before estimating the frequency
offset. */
+ fest_interval = s->hwts ? 1.0 : 16.0;
+ if ((s->local[1] - s->local[0]) / 1e9 - fest_interval > 0.001) {
+ break;
+ }

I am confused by this check. Let's say that T0 is 10seconds, and T1 is 13seconds, we are doing software timestamping..
Then the result would be (13 - 10) - 16 = -13 which fails this condition so it quits. Unless I am missing something this will break only when the interval is more than fest_interval, and will continue otherwise. I don't see how this buys us anything? Maybe I don't understand the intent, or have the numbers messed up? Could you at least explain this conditional to me?

Yes, the test in the expression is backwards. It should be:

fest_interval - (s->local[1] - s->local[0]) / 1e9 > 0.001

Thanks,
Richard

[Miroslav Lichvar]

Here is what I came up with after the recent discussion about adapting
the servo to the sync interval. I gave this some light testing,
including changing the sync on the fly. Comments are welcome.

The code looks good to me. The only issue I noticed is missing update
of the constants in pi_sync_interval().

But I'm still thinking about using the fully functional approach. One
advantage would be that we could use the measured update interval
instead of the expected interval and push the constants further to the
limits without having to worry about instability. Another advantage
would be easier configuration. The table allows any setting, but I
think users who have the knowledge needed to configure the constants
well, will always need to use a function of the update interval to
derive the constants.

What if the A*, B* constants from your patch implementing the table
calculation were configurable?

kp = Ap * u^Bp
ki = Ai * u^Bi

And then limit the constants by
kp = min(kp, Mp / u)
ki = min(ki, Mi / u)

With only six configurable constants I think we would cover any needs
the user might have and enforce the stability of the servo. It will be
easier to configure than filling the table and we could have some
recommendations on how to tweak the constants. If we receive reports
from users that it's not enough, we can extend the function and
provide more configurable constants.

What do you think?

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar
With only six configurable constants I think we would cover any needs
the user might have and enforce the stability of the servo. It will be
easier to configure than filling the table and we could have some
recommendations on how to tweak the constants. If we receive reports
from users that it's not enough, we can extend the function and
provide more configurable constants.
What do you think?

I also thought about a purely functional implementation, but I am not
sure that would be best. Overall, I would like the program to "just
work" for most people. I think very few people actually worry about
the exact synchronization performance, and from this point of view,
the functional way is fine.

Originally I wrote a simple PI controller, and I had hoped that people
would contribute other, more creative the servo alternatives (hence
the modular approach). So far, only you, Miroslav, have shown any
interest in this area at all (which I do appreciate).

So how does ntpd handle this issue? I got the impression that most of
the servo magic is hard coded, but I don't know it that well.

Getting back to the question, one use case that the functional
approach does not cover is when you have a carefully engineered
environment, with one, set sync interval, and particular expectations
for the servo behavior. But maybe this is not so important, as one
could always just choose the formula's weights to exactly yield the
desired PI constants.

Thanks,
Richard

[Keller, Jacob E]

-----Original Message-----
Sent: Wednesday, May 22, 2013 11:38 AM
To: Miroslav Lichvar
Subject: Re: [Linuxptp-devel] [PATCH RFC 0/6] calculate PI constants for
different sync intervals

Post by Miroslav Lichvar
With only six configurable constants I think we would cover any needs
the user might have and enforce the stability of the servo. It will be
easier to configure than filling the table and we could have some
recommendations on how to tweak the constants. If we receive

reports

Post by Miroslav Lichvar
from users that it's not enough, we can extend the function and
provide more configurable constants.
What do you think?

I also thought about a purely functional implementation, but I am not
sure that would be best. Overall, I would like the program to "just
work" for most people. I think very few people actually worry about
the exact synchronization performance, and from this point of view,
the functional way is fine.
Originally I wrote a simple PI controller, and I had hoped that people
would contribute other, more creative the servo alternatives (hence
the modular approach). So far, only you, Miroslav, have shown any
interest in this area at all (which I do appreciate).
So how does ntpd handle this issue? I got the impression that most of
the servo magic is hard coded, but I don't know it that well.
Getting back to the question, one use case that the functional
approach does not cover is when you have a carefully engineered
environment, with one, set sync interval, and particular expectations
for the servo behavior. But maybe this is not so important, as one
could always just choose the formula's weights to exactly yield the
desired PI constants.
Thanks,
Richard

Part of the reason few people have attempted to create different styles of servo, is the overall complexity of doing this well. Some of the people I work with have trouble understanding PTP protocol let alone understanding how the servo impacts this.

- Jake

[Dale Smith]

On Wed, May 22, 2013 at 4:42 PM, Keller, Jacob E

Post by Keller, Jacob E

-----Original Message-----
Originally I wrote a simple PI controller, and I had hoped that people
would contribute other, more creative the servo alternatives (hence
the modular approach). So far, only you, Miroslav, have shown any
interest in this area at all (which I do appreciate).

Part of the reason few people have attempted to create different styles of
servo, is the overall complexity of doing this well. Some of the people I
work with have trouble understanding PTP protocol let alone understanding
how the servo impacts this.

So what other kinds of servos could possibly be useful? Adding D for a
full PID would probably not work that well. I know *my* vcxo is way too
jittery for that.

-Dale

[Richard Cochran]

Post by Dale Smith
So what other kinds of servos could possibly be useful?

Maybe an optimal controller, or something more dynamic, like using a
different strategy for large and small offsets.

Post by Dale Smith
Adding D for a
full PID would probably not work that well. I know *my* vcxo is way too
jittery for that.

A colleague of mine tried once to improve ptpd's servo by adding 'D',
but it did not improve things, IIRC.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Dale Smith
So what other kinds of servos could possibly be useful?

Maybe an optimal controller, or something more dynamic, like using a
different strategy for large and small offsets.

It should be able to adapt to the observed jitter and wander, that
would be a great improvement over the fixed PI servo.

I like the algoritm used by chrony, but I may be biased. It keeps past
measurements and uses weighted linear regression to find the offset
and frequency. The number of samples used in the regression is
controlled by a statistical test, this is what provides the adaptivity
to jitter/wander. Unlike PI or PLL, it doesn't suffer from
undersampling.

I believe it could be adapted for PTP clocks, but it would have to be
implemented from scratch as the licensing is not compatible with
linuxptp (GPLv2 vs GPLv2+).

Thanks,

[Miroslav Lichvar]

Post by Richard Cochran
So how does ntpd handle this issue? I got the impression that most of
the servo magic is hard coded, but I don't know it that well.

The PLL/FLL clock discipline used by ntpd has two adjustable
parameters. One is the time constant, which is set to the current
polling interval. By adjusting the polling interval, ntpd controls the
response of the loop.

The other parameter is the update interval at which the FLL part
becomes active, this should be the Allan intercept. The kernel PLL/FLL
implementation has only a flag which switches between two hardcoded
values, but ntpd doesn't ever change it. With the daemon loop it is
fully configurable.

Post by Richard Cochran
Getting back to the question, one use case that the functional
approach does not cover is when you have a carefully engineered
environment, with one, set sync interval, and particular expectations
for the servo behavior. But maybe this is not so important, as one
could always just choose the formula's weights to exactly yield the
desired PI constants.

Yes, by setting the Bp, Bi constants to zero the user can make the
function constant and set the P, I constants directly.

Thanks,