Discussion:
[Linuxptp-devel] [PATCH RFC 0/3] PI servo improvements
Richard Cochran
2013-06-24 18:44:07 UTC
Permalink
Here a WIP on improving the servo, inspired by the recent discussions
on the -devel list.

Warning: compile tested only!

Enjoy,
Richard


Richard Cochran (3):
Let the clock servo know the expected sync interval.
Use a dynamic frequency estimation interval.
Automatically adjust PI constants according to sync interval.

clock.c | 2 ++
pi.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++--------
servo.c | 5 ++++
servo.h | 7 +++++
servo_private.h | 2 ++
5 files changed, 83 insertions(+), 10 deletions(-)
--
1.7.10.4
Richard Cochran
2013-06-24 18:44:08 UTC
Permalink
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(+)

diff --git a/clock.c b/clock.c
index 0353b5a..a1fec55 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 89080c4..379b99f 100644
--- a/pi.c
+++ b/pi.c
@@ -132,6 +132,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;
@@ -142,6 +146,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->first_update = 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
--
1.7.10.4
Keller, Jacob E
2013-06-24 20:21:04 UTC
Permalink
Comment
-----Original Message-----
Sent: Monday, June 24, 2013 11:44 AM
Subject: [Linuxptp-devel] [PATCH RFC 1/3] 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(+)
diff --git a/clock.c b/clock.c
index 0353b5a..a1fec55 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 89080c4..379b99f 100644
--- a/pi.c
+++ b/pi.c
@@ -132,6 +132,10 @@ static double pi_sample(struct servo *servo,
return ppb;
}
+static void pi_sync_interval(struct servo *servo, int n)
+{
+}
+
Would it be cleaner to use a null pointer, and do a check before we call rather than using an empty function?
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
{
struct pi_servo *s;
@@ -142,6 +146,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->first_update = 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);
What if we did:

if (servo->sync_interval)
servo->sync_interval(servo, n);

that way we don't have to use an empty function that does nothing and it's clear this is an optional control function?

- Jake
Richard Cochran
2013-06-25 05:24:20 UTC
Permalink
Post by Keller, Jacob E
Post by Richard Cochran
+static void pi_sync_interval(struct servo *servo, int n)
+{
+}
+
Would it be cleaner to use a null pointer, and do a check before we call rather than using an empty function?
In this case, it is really only a chicken-egg problem. The code for
this function appears in the third patch.
Post by Keller, Jacob E
if (servo->sync_interval)
servo->sync_interval(servo, n);
that way we don't have to use an empty function that does nothing and it's clear this is an optional control function?
That would be okay, but this method is essential, not optional. It
only looks that way here because we are grafting a new servo interface
onto older code. I could combine patch #3 into this one, but I thought
it might be clearer to review if the changes come step by step.

Thanks,
Richard
Keller, Jacob E
2013-06-25 21:35:56 UTC
Permalink
-----Original Message-----
Sent: Monday, June 24, 2013 10:24 PM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] [PATCH RFC 1/3] Let the clock servo know
the expected sync interval.
Post by Keller, Jacob E
Post by Richard Cochran
+static void pi_sync_interval(struct servo *servo, int n)
+{
+}
+
Would it be cleaner to use a null pointer, and do a check before we
call rather than using an empty function?
In this case, it is really only a chicken-egg problem. The code for
this function appears in the third patch.
Post by Keller, Jacob E
if (servo->sync_interval)
servo->sync_interval(servo, n);
that way we don't have to use an empty function that does nothing
and it's clear this is an optional control function?
That would be okay, but this method is essential, not optional. It
only looks that way here because we are grafting a new servo interface
onto older code. I could combine patch #3 into this one, but I thought
it might be clearer to review if the changes come step by step.
Thanks,
Richard
Yep, I figured that out after reading the 3rd patch. Thanks Richard :)

- Jake
Richard Cochran
2013-06-24 18:44:09 UTC
Permalink
A slow servo (with smaller constants and lower sync rate) needs a
longer, better frequency estimation, but a higher sync rate hardly
needs any estimation at all, since it learns the frequency right away
in any case.

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

diff --git a/pi.c b/pi.c
index 379b99f..d5e6359 100644
--- a/pi.c
+++ b/pi.c
@@ -30,6 +30,7 @@
#define SWTS_KI 0.001

#define NSEC_PER_SEC 1000000000
+#define FREQ_EST_MARGIN 0.001

/* These take their values from the configuration file. (see ptp4l.c) */
double configured_pi_kp = 0.0;
@@ -64,6 +65,7 @@ static double pi_sample(struct servo *servo,
enum servo_state *state)
{
double ki_term, ppb = 0.0;
+ double freq_est_interval, localdiff;
struct pi_servo *s = container_of(servo, struct pi_servo, servo);

switch (s->count) {
@@ -84,6 +86,18 @@ static double pi_sample(struct servo *servo,
break;
}

+ /* Wait long enough before estimating the frequency offset. */
+ localdiff = (s->local[1] - s->local[0]) / 1e9;
+ localdiff += localdiff * FREQ_EST_MARGIN;
+ freq_est_interval = 0.016 / s->ki;
+ if (freq_est_interval > 1000.0) {
+ freq_est_interval = 1000.0;
+ }
+ if (localdiff < freq_est_interval) {
+ *state = SERVO_UNLOCKED;
+ break;
+ }
+
s->drift += (s->offset[1] - s->offset[0]) * 1e9 /
(s->local[1] - s->local[0]);
if (s->drift < -s->maxppb)
--
1.7.10.4
Miroslav Lichvar
2013-06-25 09:45:07 UTC
Permalink
Post by Richard Cochran
A slow servo (with smaller constants and lower sync rate) needs a
longer, better frequency estimation, but a higher sync rate hardly
needs any estimation at all, since it learns the frequency right away
in any case.
In the simulations with phc2sys and different update intervals it
works great. Please push it. I'll have some comments on the other two
patches.

Thanks,
--
Miroslav Lichvar
Richard Cochran
2013-06-24 18:44:10 UTC
Permalink
This patch makes the PI controller adapt its PI constants whenever the
sync interval changes. The constants are calculated using the formula

C = A * sync_interval ^ B

where the scaling factor A and exponent B will be user configurable (in
a subsequent patch). Credit for this idea goes to Miroslav Lichvar.

Signed-off-by: Richard Cochran <***@gmail.com>
---
pi.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/pi.c b/pi.c
index d5e6359..2048f12 100644
--- a/pi.c
+++ b/pi.c
@@ -23,11 +23,11 @@
#include "pi.h"
#include "servo_private.h"

-#define HWTS_KP 0.7
-#define HWTS_KI 0.3
-
-#define SWTS_KP 0.1
-#define SWTS_KI 0.001
+#define SWTS_KP_SCALE 0.1
+#define SWTS_KI_SCALE 0.001
+#define SWTS_KP_EXPONENT (-0.5)
+#define SWTS_KI_EXPONENT (+0.5)
+#define TT 1.0

#define NSEC_PER_SEC 1000000000
#define FREQ_EST_MARGIN 0.001
@@ -39,6 +39,13 @@ double configured_pi_offset = 0.0;
double configured_pi_f_offset = 0.0000001; /* 100 nanoseconds */
int configured_pi_max_freq = 900000000;

+double kp_scale = .8;
+double ki_scale = .2;
+double kp_exponent = (-0.5);
+double ki_exponent = (+0.5);
+static double kp_slow_const = .7;
+static double ki_slow_const = .38;
+
struct pi_servo {
struct servo servo;
int64_t offset[2];
@@ -53,6 +60,22 @@ struct pi_servo {
int first_update;
};

+static double kp(double t)
+{
+ if (t > TT) {
+ return kp_slow_const / t;
+ }
+ return kp_scale * pow(t, kp_exponent);
+}
+
+static double ki(double t)
+{
+ if (t > TT) {
+ return ki_slow_const / t;
+ }
+ return ki_scale * pow(t, ki_exponent);
+}
+
static void pi_destroy(struct servo *servo)
{
struct pi_servo *s = container_of(servo, struct pi_servo, servo);
@@ -148,6 +171,16 @@ static double pi_sample(struct servo *servo,

static void pi_sync_interval(struct servo *servo, int n)
{
+ double t;
+ struct pi_servo *s = container_of(servo, struct pi_servo, servo);
+
+ if (n < 0) {
+ t = 1.0 / (1 << -n);
+ } else {
+ t = (1 << n);
+ }
+ s->kp = kp(t);
+ s->ki = ki(t);
}

struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
@@ -165,15 +198,20 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->maxppb = max_ppb;
s->first_update = 1;

+ if (sw_ts) {
+ kp_scale = SWTS_KP_SCALE;
+ ki_scale = SWTS_KI_SCALE;
+ kp_exponent = SWTS_KP_EXPONENT;
+ ki_exponent = SWTS_KI_EXPONENT;
+ kp_slow_const = SWTS_KP_SCALE;
+ ki_slow_const = SWTS_KI_SCALE;
+ }
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;
+ s->kp = kp(1.0);
+ s->ki = ki(1.0);
}

if (configured_pi_offset > 0.0) {
--
1.7.10.4
Miroslav Lichvar
2013-06-25 13:23:17 UTC
Permalink
Post by Richard Cochran
This patch makes the PI controller adapt its PI constants whenever the
sync interval changes. The constants are calculated using the formula
C = A * sync_interval ^ B
where the scaling factor A and exponent B will be user configurable (in
a subsequent patch). Credit for this idea goes to Miroslav Lichvar.
I'm very happy to see it going in this direction instead of the table
implementation. I have two suggestions to further improve it.

Instead of using the interval announced by the PTP master, I think
it's better to calculate the constants from the difference between the
last two local time stamps. This would avoid instability of the servo
due to undersampling when:
- the actual sync interval is longer than logSyncInterval
(the spec requires only +/- 30% for 90% of messages)
- the sync messages are getting dropped in the network
- a filter which drops outliers is implemented

The second suggestion is to use the same formula for all intervals
instead of the split at 1 second. Drop the TT check and limit the
kp and ki constants by another (configurable?) constant as I suggested
before. Something like this:

kp = kp_scale * pow(t, kp_exponent);
if (kp > kp_norm_max / t)
kp = kp_norm_max / t;
return kp;

I think this is a cleaner solution and it will allow greater
flexibility in the servo configuration. I can prepare patches for it
if you want to push your patches as they are now and consider my
suggestions later.

Thanks,
--
Miroslav Lichvar
Richard Cochran
2013-06-26 13:22:37 UTC
Permalink
Post by Miroslav Lichvar
Instead of using the interval announced by the PTP master, I think
it's better to calculate the constants from the difference between the
last two local time stamps.
Well, it would have to be the transmit time, if anything. Still, I
don't like this, mostly because I have never seen anything like this
before. I doubt it would work better, but that is only my gut
feeling.
Post by Miroslav Lichvar
This would avoid instability of the servo
- the actual sync interval is longer than logSyncInterval
(the spec requires only +/- 30% for 90% of messages)
Probably the intent of the standard is that the mean is still correct,
just like for the delay requests. Otherwise, why should the clients
have stricter timing requirement than the master?
Post by Miroslav Lichvar
- the sync messages are getting dropped in the network
- a filter which drops outliers is implemented
I expect that the servo is already robust enough for these two.
Post by Miroslav Lichvar
The second suggestion is to use the same formula for all intervals
instead of the split at 1 second. Drop the TT check and limit the
kp and ki constants by another (configurable?) constant as I suggested
kp = kp_scale * pow(t, kp_exponent);
if (kp > kp_norm_max / t)
kp = kp_norm_max / t;
return kp;
I think this is a cleaner solution and it will allow greater
flexibility in the servo configuration. I can prepare patches for it
if you want to push your patches as they are now and consider my
suggestions later.
This second suggestion seems reasonable to me. Would you care to take
this patch #3 and re-write it?

As to the first suggestion, I think it goes too far. If you want to,
why not create a second servo implementing everything exactly as you
would like? I would gladly merge alternate servos, but I would like to
have one "traditional" PI controller as the default.

Thanks,
Richard
Miroslav Lichvar
2013-06-27 08:37:51 UTC
Permalink
Post by Richard Cochran
Post by Miroslav Lichvar
The second suggestion is to use the same formula for all intervals
instead of the split at 1 second. Drop the TT check and limit the
kp and ki constants by another (configurable?) constant as I suggested
kp = kp_scale * pow(t, kp_exponent);
if (kp > kp_norm_max / t)
kp = kp_norm_max / t;
return kp;
I think this is a cleaner solution and it will allow greater
flexibility in the servo configuration. I can prepare patches for it
if you want to push your patches as they are now and consider my
suggestions later.
This second suggestion seems reasonable to me. Would you care to take
this patch #3 and re-write it?
Ok, I'll do that.
Post by Richard Cochran
As to the first suggestion, I think it goes too far. If you want to,
why not create a second servo implementing everything exactly as you
would like? I would gladly merge alternate servos, but I would like to
have one "traditional" PI controller as the default.
Fair enough.

Thanks,
--
Miroslav Lichvar
Loading...