Re: [Linuxptp-users] Interesting (very wrong) PTP output

[Christian R. <chr...@om...>]

Hi Jake and Richard,

>On Tue, Jun 05, 2012 at 11:31:24PM +0000, Keller, Jacob E wrote:
>> It seems like somehow the p/i servo code is incorrect because once
>> we hit max adjust it never seems to move away from it.
>
> The servo clamps the adjustment to the maximum. It looks like it the
> servo output is always outside of the limit. You can inspect s->drift
> to get a better idea what is going on.

I agree, looks like something is going wrong with s->drift, it seems to get
set to a very high value and does not recover from that.

Since the value of s->drift is clamped to the maximum adjustment in pi.c when
s->count == 4, it must be the initial calculation for s->count == 2. This
calculation tries to get a good estimate for s->drift to allow a faster
settling of the control loop. However, there is no clamping of the value
of s->drift in this case.

What are your scripts doing before ptp4l is started? Do they do some clock
adjustments? Note that the initial calculation of s->drift only works
if the clock is initially set to 0 ppb, otherwise some correction
of the calculated s->drift must be done.

Finally, if we calculate an estimate for s->drift, we should also
apply it to the clock.

Please find below a patch (it's only an quick hack and only compile-tested)
that
a) sets the clock to 0 ppb before doing anything else,
b) clamps s->drift to the maximum adjustment value at the first calculation
   (estimation),
c) applies the estimate to the clock.

Regards,
Christian

---
 clock.c |    3 +--
 pi.c    |   25 +++++++++++++++++--------
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/clock.c b/clock.c
index e3797bf..c8216c3 100644
--- a/clock.c
+++ b/clock.c
@@ -475,13 +475,12 @@ enum servo_state clock_synchronize(struct clock *c,
 		c->master_offset, state, adj, c->path_delay);
 
 	switch (state) {
-	case SERVO_UNLOCKED:
-		break;
 	case SERVO_JUMP:
 		clock_step(c->clkid, -c->master_offset);
 		c->t1 = tmv_zero();
 		c->t2 = tmv_zero();
 		break;
+	case SERVO_UNLOCKED:
 	case SERVO_LOCKED:
 		clock_ppb(c->clkid, -adj);
 		break;
diff --git a/pi.c b/pi.c
index 33766b1..743ae71 100644
--- a/pi.c
+++ b/pi.c
@@ -59,28 +59,37 @@ static double pi_sample(struct servo *servo,
 
 	switch (s->count) {
 	case 0:
-		s->offset[0] = offset;
-		s->local[0] = local_ts;
 		*state = SERVO_UNLOCKED;
-		s->count = 1;
 		break;
 	case 1:
-		s->offset[1] = offset;
-		s->local[1] = local_ts;
+		s->offset[0] = offset;
+		s->local[0] = local_ts;
 		*state = SERVO_UNLOCKED;
 		s->count = 2;
 		break;
 	case 2:
-		s->drift = (s->offset[1] - s->offset[0]) /
-			(s->local[1] - s->local[0]);
+		s->offset[1] = offset;
+		s->local[1] = local_ts;
 		*state = SERVO_UNLOCKED;
 		s->count = 3;
 		break;
 	case 3:
-		*state = SERVO_JUMP;
+		s->drift = (s->offset[1] - s->offset[0]) /
+			(s->local[1] - s->local[0]);
+		if (s->drift < -s->maxppb) {
+			s->drift = -s->maxppb;
+		} else if (s->drift > s->maxppb) {
+			s->drift = s->maxppb;
+		}
+		ppb = s->drift;
+		*state = SERVO_UNLOCKED;
 		s->count = 4;
 		break;
 	case 4:
+		*state = SERVO_JUMP;
+		s->count = 5;
+		break;
+	case 5:
 		ki_term = s->ki * offset;
 		ppb = s->kp * offset + s->drift + ki_term;
 		if (ppb < -s->maxppb) {
-- 
1.7.4.1