[Linuxptp-devel] [PATCH RFC] phc2sys: add -X option that disallows to step on start

Discussion:

Ken ICHIKAWA

2013-06-11 05:58:32 UTC

This patch adds -X option that doesn't allow to step the slave clock
based on the master clock when phc2sys starts.
Instead the offset is slowly corrected by changing the clock frequency
unless -S option is used.

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

Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
phc2sys.8 | 14 ++++++++++++--
phc2sys.c | 15 +++++++++++++--
2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/phc2sys.8 b/phc2sys.8
index 92081cf..86a0fc1 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -29,6 +29,8 @@ phc2sys \- synchronize two clocks
] [
.B \-x
] [
+.B \-X
+] [
.BI \-l " print-level"
] [
.B \-m
@@ -95,8 +97,10 @@ 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
+.B \-X
+option is used). The value of 0.0 disables stepping after the start. The
+default is 0.0.
.TP
.BI \-R " update-rate"
Specify the slave clock update rate when running in the direct synchronization
@@ -140,6 +144,12 @@ clock frequency (unless the
.B \-S
option is used).
.TP
+.B \-X
+Don't allow the slave clock to be stepped on start so that the offset is
+always corrected slowly by changing the clock frequency (unless the
+.B \-S
+option is used).
+.TP
.BI \-l " print-level"
Set the maximum syslog level of messages which should be printed or sent to
the system logger. The default is 6 (LOG_INFO).
diff --git a/phc2sys.c b/phc2sys.c
index d4c9b65..108fcea 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -139,6 +139,7 @@ struct clock {
int leap;
int leap_set;
int kernel_leap;
+ int skip_step;
struct pmc *pmc;
int pmc_ds_idx;
int pmc_ds_requested;
@@ -200,7 +201,12 @@ static void update_clock(struct clock *clock,
case SERVO_UNLOCKED:
break;
case SERVO_JUMP:
- clockadj_step(clock->clkid, -offset);
+ if (clock->skip_step)
+ /* Skip step only once to avoid conflict
+ between -S option and -X option. */
+ clock->skip_step = 0;
+ else
+ clockadj_step(clock->clkid, -offset);
/* Fall through. */
case SERVO_LOCKED:
clockadj_set_freq(clock->clkid, -ppb);
@@ -562,6 +568,7 @@ static void usage(char *progname)
" -w wait for ptp4l\n"
" -n [num] domain number (0)\n"
" -x apply leap seconds by servo instead of kernel\n"
+ " -X don't allow to step on start\n"
" -l [num] set the logging level to 'num' (6)\n"
" -m print messages to stdout\n"
" -q do not print messages to the syslog\n"
@@ -584,6 +591,7 @@ int main(int argc, char *argv[])
.clkid = CLOCK_REALTIME,
.servo_state = SERVO_UNLOCKED,
.kernel_leap = 1,
+ .skip_step = 0,
};

configured_pi_kp = KP;
@@ -593,7 +601,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:R:N:O:i:u:wn:xXl:mqvh"))) {
switch (c) {
case 'c':
dst_clock.clkid = clock_open(optarg);
@@ -666,6 +674,9 @@ int main(int argc, char *argv[])
case 'x':
dst_clock.kernel_leap = 0;
break;
+ case 'X':
+ dst_clock.skip_step = 1;
+ break;
case 'l':
if (get_arg_val_i(c, optarg, &print_level,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX))

--
1.7.1

Miroslav Lichvar

2013-06-11 09:57:51 UTC

Permalink

Post by Ken ICHIKAWA
This patch adds -X option that doesn't allow to step the slave clock
based on the master clock when phc2sys starts.
Instead the offset is slowly corrected by changing the clock frequency
unless -S option is used.
This feature is usefull when we need to restart phc2sys without system
clock jump. Restarting phc2sys is needed to change the configuration.
@@ -200,7 +201,12 @@ static void update_clock(struct clock *clock,
break;
- clockadj_step(clock->clkid, -offset);
+ if (clock->skip_step)
+ /* Skip step only once to avoid conflict
+ between -S option and -X option. */
+ clock->skip_step = 0;
+ else
+ clockadj_step(clock->clkid, -offset);

I like the idea, but I think it would be cleaner if it was a new
option of the servo and its requests to step the clock were not
ignored. With the PI servo it probably doesn't matter, but there could
be a different servo implemented which keeps old samples to calculate
the offset and frequency, or do some statistics, and this would
invalidate the old samples.

In addition to the -S step threshold there could be a new threshold
applied only in the first clock update. By default it could have some
small value (e.g. 100 nanoseconds) and 0.0 would disable it.

What do you think?

Thanks,

--
Miroslav Lichvar

Ken ICHIKAWA

2013-06-12 08:34:33 UTC

Permalink

Thank you for your comment.
I'd like to check my understanding.

Post by Miroslav Lichvar

I like the idea, but I think it would be cleaner if it was a new
option of the servo and its requests to step the clock were not
ignored.

Do you mean that the decision of clock step should be done in servo
(not in update_clock())?
So if the new option is used, servo state will become SERVO_LOCKED
directly from SERVO_UNLOCKED on start, right?

Post by Miroslav Lichvar
With the PI servo it probably doesn't matter, but there could
be a different servo implemented which keeps old samples to calculate
the offset and frequency, or do some statistics, and this would
invalidate the old samples.

Do you mean that maybe we should make a new servo but the basic mechanism
of PI servo itself does not need to be changed?

Post by Miroslav Lichvar
In addition to the -S step threshold there could be a new threshold
applied only in the first clock update. By default it could have some
small value (e.g. 100 nanoseconds) and 0.0 would disable it.

Sounds good.

Ken ICHIKAWA

Post by Miroslav Lichvar
What do you think?
Thanks,

Miroslav Lichvar

2013-06-12 10:07:17 UTC

Permalink

Post by Ken ICHIKAWA

Post by Miroslav Lichvar
I like the idea, but I think it would be cleaner if it was a new
option of the servo and its requests to step the clock were not
ignored.

Do you mean that the decision of clock step should be done in servo
(not in update_clock())?

Yes.

Post by Ken ICHIKAWA
So if the new option is used, servo state will become SERVO_LOCKED
directly from SERVO_UNLOCKED on start, right?

Right.

Post by Ken ICHIKAWA

Do you mean that maybe we should make a new servo but the basic mechanism
of PI servo itself does not need to be changed?

I wasn't suggesting to create a new PI servo for the new step
threshold applied on start, if that's what you are asking. There was a
discussion recently about implementing a better servo which would
adapt to the stability of the clock and the network, avoiding the
difficult configuration of the PI servo. If it has a memory of old
samples, it will need to be able to rely on that all its adjustments
are applied to the clock, so the history can be updated according to
the adjustments and kept valid.

Thanks,

--
Miroslav Lichvar

Ken ICHIKAWA

2013-06-13 00:29:43 UTC

Permalink

Post by Miroslav Lichvar

Post by Ken ICHIKAWA

Do you mean that maybe we should make a new servo but the basic mechanism
of PI servo itself does not need to be changed?

I understood.
So I wait for the work of the servo done.

Thank you for your kind reply,
Ken ICHIKAWA

Miroslav Lichvar

2013-06-14 08:11:01 UTC

Permalink

Post by Ken ICHIKAWA

Post by Miroslav Lichvar
I wasn't suggesting to create a new PI servo for the new step
threshold applied on start, if that's what you are asking. There was a
discussion recently about implementing a better servo which would
adapt to the stability of the clock and the network, avoiding the
difficult configuration of the PI servo. If it has a memory of old
samples, it will need to be able to rely on that all its adjustments
are applied to the clock, so the history can be updated according to
the adjustments and kept valid.

I understood.
So I wait for the work of the servo done.

Please note that for me it was a long-term plan and it probably will
be a lot of work. Even if a new servo is implemented, I think the PI
servo will be kept and would benefit from the start step threshold
option too.

Thanks,

--
Miroslav Lichvar

Ken ICHIKAWA

2013-06-14 08:52:25 UTC

Permalink

Post by Miroslav Lichvar

Post by Ken ICHIKAWA

I understood.
So I wait for the work of the servo done.

You mean current PI servo and the servo discussed recently will be selectable in the future?
If so, I'll work for current PI servo to add this feature.

Thanks,
Ken ICHIKAWA

Miroslav Lichvar

2013-06-14 09:09:45 UTC

Permalink

Post by Ken ICHIKAWA
You mean current PI servo and the servo discussed recently will be selectable in the future?
If so, I'll work for current PI servo to add this feature.

Yes, the code allows the user to select between different servos by
the clock_servo option, but currently only one is implemented.

Thanks,

--
Miroslav Lichvar