[Linuxptp-devel] [PATCHv2] Add a new servo option which specifies a first step threshold

Discussion:

Ken ICHIKAWA

2013-06-18 07:46:46 UTC

Current pi servo steps clock without any condition on start.
This patch adds a new servo option "configured_pi_f_offset". The option is similar
to configured_pi_offset but only affects in the first clock update. Therefore,
if this option is set as 0.0, we can prevent clock step on start.
The new servo option can be specified from phc2sys by using -F option.

This feature is usefull when we need to restart phc2sys without system
clock jump. Restarting phc2sys is needed to change its configuration.

change from v1:(http://sourceforge.net/mailarchive/message.php?msg_id=31039874)
- remake as a new servo option.

Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
phc2sys.8 | 14 ++++++++++++--
phc2sys.c | 8 +++++++-
pi.c | 18 +++++++++++++++++-
pi.h | 11 ++++++++++-
4 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/phc2sys.8 b/phc2sys.8
index 92081cf..0e55610 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -15,6 +15,10 @@ phc2sys \- synchronize two clocks
] [
.BI \-I " ki"
] [
+.BI \-S " step"
+] [
+.BI \-F " step"
+] [
.BI \-R " update-rate"
] [
.BI \-N " clock-readings"
@@ -95,8 +99,14 @@ Specify the integral constant of the PI controller. The default is 0.3.
.BI \-S " step"
Specify the step threshold of the PI controller. It is the maximum offset that
the controller corrects by changing the clock frequency instead of stepping the
-clock. The clock is always stepped on start. The value of 0.0 disables stepping
-after the start. The default is 0.0.
+clock. The clock is always stepped on start (unless the
+.BI \-F " step"
+is 0.0). The value of 0.0 disables stepping after the start. The default is 0.0.
+.TP
+.BI \-F " step"
+Specify the first step threshold of the PI controller. It is the maximum offset
+that the controller corrects by stepping the clock on start. The value of 0.0
+disables stepping on start. The default is 0.0000001 (100 nano seconds).
.TP
.BI \-R " update-rate"
Specify the slave clock update rate when running in the direct synchronization
diff --git a/phc2sys.c b/phc2sys.c
index d4c9b65..25a6977 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -555,6 +555,7 @@ static void usage(char *progname)
" -P [kp] proportional constant (0.7)\n"
" -I [ki] integration constant (0.3)\n"
" -S [step] step threshold (disabled)\n"
+ " -F [step] first step threshold on start (0.0000001)\n"
" -R [rate] slave clock update rate in HZ (1.0)\n"
" -N [num] number of master clock readings per update (5)\n"
" -O [offset] slave-master time offset (0)\n"
@@ -593,7 +594,7 @@ int main(int argc, char *argv[])
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:u:wn:xl:mqv"))) {
+ "c:d:s:P:I:S:F:R:N:O:i:u:wn:xl:mqvh"))) {
switch (c) {
case 'c':
dst_clock.clkid = clock_open(optarg);
@@ -627,6 +628,11 @@ int main(int argc, char *argv[])
0.0, DBL_MAX))
return -1;
break;
+ case 'F':
+ if (get_arg_val_d(c, optarg, &configured_pi_f_offset,
+ 0.0, DBL_MAX))
+ return -1;
+ break;
case 'R':
if (get_arg_val_d(c, optarg, &phc_rate, 0.0, DBL_MAX))
return -1;
diff --git a/pi.c b/pi.c
index 427cb3e..0bf65ec 100644
--- a/pi.c
+++ b/pi.c
@@ -18,6 +18,7 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <stdlib.h>
+#include <float.h>
#include <math.h>

#include "pi.h"
@@ -35,6 +36,7 @@
double configured_pi_kp = 0.0;
double configured_pi_ki = 0.0;
double configured_pi_offset = 0.0;
+double configured_pi_f_offset = 0.0000001; /* 100 nano seconds */
int configured_pi_max_freq = 900000000;

struct pi_servo {
@@ -46,6 +48,7 @@ struct pi_servo {
double kp;
double ki;
double max_offset;
+ double max_f_offset;
int count;
};

@@ -87,7 +90,14 @@ static double pi_sample(struct servo *servo,
else if (s->drift > s->maxppb)
s->drift = s->maxppb;

- *state = SERVO_JUMP;
+ if (s->max_f_offset && (s->max_f_offset < fabs(offset)))
+ *state = SERVO_JUMP;
+ else
+ *state = SERVO_LOCKED;
+ /* Assign a value which always steps clock. This enables
+ clock jump by configured_pi_offset(aka s->max_offset). */
+ s->max_f_offset = DBL_MIN;
+
ppb = s->drift;
s->count = 2;
break;
@@ -151,6 +161,12 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->max_offset = 0.0;
}

+ if (configured_pi_f_offset > 0.0) {
+ s->max_f_offset = configured_pi_f_offset * NSEC_PER_SEC;
+ } else {
+ s->max_f_offset = 0.0;
+ }
+
if (configured_pi_max_freq && s->maxppb > configured_pi_max_freq) {
s->maxppb = configured_pi_max_freq;
}
diff --git a/pi.h b/pi.h
index 954f495..2f31bce 100644
--- a/pi.h
+++ b/pi.h
@@ -36,13 +36,22 @@ 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
+ * mechanism.
*
* Note that this variable is measured in seconds, and allows fractional values.
*/
extern double configured_pi_offset;

/**
+ * When set to zero, the clock is not stepped on start. When set to a non-zero
+ * value, the value bahaves as a threshold and the clock is stepped on start if
+ * the offset is bigger than the threshold.
+ *
+ * Note that this variable is measured in seconds, and allows fractional values.
+ */
+extern double configured_pi_f_offset;
+
+/**
* When set to a non-zero value, this variable sets an additional limit for
* the frequency adjustment of the clock. It's in ppb.
*/

--
1.7.1

Miroslav Lichvar

2013-06-18 09:51:14 UTC

Permalink

Post by Ken ICHIKAWA
Current pi servo steps clock without any condition on start.
This patch adds a new servo option "configured_pi_f_offset". The option is similar
to configured_pi_offset but only affects in the first clock update. Therefore,
if this option is set as 0.0, we can prevent clock step on start.
The new servo option can be specified from phc2sys by using -F option.
This feature is usefull when we need to restart phc2sys without system
clock jump. Restarting phc2sys is needed to change its configuration.
change from v1:(http://sourceforge.net/mailarchive/message.php?msg_id=31039874)
- remake as a new servo option.

Some comments are below.

Post by Ken ICHIKAWA
+.BI \-F " step"
+Specify the first step threshold of the PI controller.

This is not very clear to me. How about "Specify the step threshold
applied only on the first clock update"?

Post by Ken ICHIKAWA
It is the maximum offset
+that the controller corrects by stepping the clock on start.

I think it should be the maximum offset that is corrected by adjusting
clock, or the minimum offset corrected by step.

Post by Ken ICHIKAWA
The value of 0.0
+disables stepping on start. The default is 0.0000001 (100 nano seconds).

I'm not sure if nano seconds is a correct term, but in the rest of the
code it's written as nanoseconds.

Post by Ken ICHIKAWA
@@ -87,7 +90,14 @@ static double pi_sample(struct servo *servo,
else if (s->drift > s->maxppb)
s->drift = s->maxppb;
- *state = SERVO_JUMP;
+ if (s->max_f_offset && (s->max_f_offset < fabs(offset)))
+ *state = SERVO_JUMP;
+ else
+ *state = SERVO_LOCKED;

Do you think it would make sense to check here also max_offset, i.e.
apply the -S option to all updates and the -F option only to the
first? Or don't allow -F to be smaller than -S if both are enabled?

Thanks,

--
Miroslav Lichvar

Ken ICHIKAWA

2013-06-19 06:13:21 UTC

Permalink

Post by Miroslav Lichvar

Some comments are below.

Post by Ken ICHIKAWA
+.BI \-F " step"
+Specify the first step threshold of the PI controller.

This is not very clear to me. How about "Specify the step threshold
applied only on the first clock update"?

Thank you. Please let me use the sentence.

Post by Miroslav Lichvar

Post by Ken ICHIKAWA
It is the maximum offset
+that the controller corrects by stepping the clock on start.

I think it should be the maximum offset that is corrected by adjusting
clock, or the minimum offset corrected by step.

Thank you. Same above.

Post by Miroslav Lichvar

Post by Ken ICHIKAWA
The value of 0.0
+disables stepping on start. The default is 0.0000001 (100 nano seconds).

I'm not sure if nano seconds is a correct term, but in the rest of the
code it's written as nanoseconds.

I think "nano seconds" is wrong. I'll fix it.

Post by Miroslav Lichvar

I don't understand the latter approach.
It's OK that -F is smaller than -S, isn't it?
If -F is smaller than -S, the state become SERVO_JUMP in servo step1 when
the servo is reset by max_offset in servo step2.
By contrast, if -F is bigger than -S, I think there is a problem because
the state can become SERVO_LOCKED in servo step1 even when the servo is
reset by max_offset in servo step2.

I understand the former approach as below:

diff --git a/pi.c b/pi.c
index 78cbd3f..ed92a66 100644
--- a/pi.c
+++ b/pi.c
@@ -35,6 +35,7 @@
double configured_pi_kp = 0.0;
double configured_pi_ki = 0.0;
double configured_pi_offset = 0.0;
+double configured_pi_f_offset = 0.0000001; /* 100 nanoseconds */
int configured_pi_max_freq = 900000000;

struct pi_servo {
@@ -46,6 +47,7 @@ struct pi_servo {
double kp;
double ki;
double max_offset;
+ double max_f_offset;
int count;
};

@@ -88,7 +90,12 @@ static double pi_sample(struct servo *servo,
else if (s->drift > s->maxppb)
s->drift = s->maxppb;

- *state = SERVO_JUMP;
+ if ((s->max_f_offset && (s->max_f_offset < fabs(offset))) ||
+ (s->max_offset && (s->max_offset < fabs(offset))))
+ *state = SERVO_JUMP;
+ else
+ *state = SERVO_LOCKED;
+
ppb = s->drift;
s->count = 2;
break;
@@ -152,6 +159,12 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->max_offset = 0.0;
}

+ if (configured_pi_f_offset > 0.0) {
+ s->max_f_offset = configured_pi_f_offset * NSEC_PER_SEC;
+ } else {
+ s->max_f_offset = 0.0;
+ }
+
if (configured_pi_max_freq && s->maxppb > configured_pi_max_freq) {
s->maxppb = configured_pi_max_freq;
}
--
Thanks,
Ken ICHIKAWA

Miroslav Lichvar

2013-06-19 15:13:45 UTC

Permalink

Post by Ken ICHIKAWA

Post by Miroslav Lichvar
Do you think it would make sense to check here also max_offset, i.e.
apply the -S option to all updates and the -F option only to the
first? Or don't allow -F to be smaller than -S if both are enabled?

I don't understand the latter approach.
It's OK that -F is smaller than -S, isn't it?

It is.

Post by Ken ICHIKAWA
If -F is smaller than -S, the state become SERVO_JUMP in servo step1 when
the servo is reset by max_offset in servo step2.
By contrast, if -F is bigger than -S, I think there is a problem because
the state can become SERVO_LOCKED in servo step1 even when the servo is
reset by max_offset in servo step2.

Yes, that's what I was trying to say, sorry for the confusion.

Post by Ken ICHIKAWA
@@ -88,7 +90,12 @@ static double pi_sample(struct servo *servo,
else if (s->drift > s->maxppb)
s->drift = s->maxppb;
- *state = SERVO_JUMP;
+ if ((s->max_f_offset && (s->max_f_offset < fabs(offset))) ||
+ (s->max_offset && (s->max_offset < fabs(offset))))
+ *state = SERVO_JUMP;
+ else
+ *state = SERVO_LOCKED;
+

I think there could be a problem when the offset is smaller than the
stepping threshold after the previous offset was larger than the
threshold and triggered the switch to the unlocked state. There could
be several unncessary switches between the unlocked and locked states.

Adding a "first update" variable and doing the check above only on the
first update would prevent it, I think. It would also make the
"s->max_f_offset = DBL_MIN;" assignment from the original patch
unnecessary.

The second approach might be to set the max_f_offset variable in
the initialization to max_offset if max_f_offset is larger and
max_offset is non-zero.

Thanks,

--
Miroslav Lichvar

Ken ICHIKAWA

2013-06-20 02:21:42 UTC

Permalink

Post by Miroslav Lichvar

I think so.

Post by Miroslav Lichvar
Adding a "first update" variable and doing the check above only on the
first update would prevent it, I think. It would also make the
"s->max_f_offset = DBL_MIN;" assignment from the original patch
unnecessary.

OK.

Post by Miroslav Lichvar
The second approach might be to set the max_f_offset variable in
the initialization to max_offset if max_f_offset is larger and
max_offset is non-zero.

I understand.

I prefer the first approach because I think the first approach is
more straightforward than the second approach though those two are
not so different.

Thanks,
Ken ICHIKAWA