[Linuxptp-devel] [PATCH 1/2] ptp4l: Allow P, I constants over 1.0.

[Miroslav Lichvar]

With sub-second sync intervals, it may be useful to set P and I to
values over 1.0.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index f32ab5f..3103ecd 100644
--- a/config.c
+++ b/config.c
@@ -276,12 +276,12 @@ static enum parser_result parse_global_setting(const char *option,
*cfg->tx_timestamp_timeout = val;

} else if (!strcmp(option, "pi_proportional_const")) {
- if (1 != sscanf(value, "%lf", &df) || df < 0.0 || df >= 1.0)
+ 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 || df >= 1.0)
+ if (1 != sscanf(value, "%lf", &df) || df < 0.0)
return BAD_VALUE;
*cfg->pi_integral_const = df;

[Miroslav Lichvar]

This allows update intervals longer than 1 second.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 7649c8a..b1ab552 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -286,7 +286,7 @@ static int do_pps_loop(struct clock *clock, int fd,
}

static int do_sysoff_loop(struct clock *clock, clockid_t src,
- int rate, int n_readings)
+ double rate, int n_readings)
{
uint64_t ts;
int64_t offset, delay;
@@ -306,7 +306,7 @@ static int do_sysoff_loop(struct clock *clock, clockid_t src,
}

static int do_phc_loop(struct clock *clock, clockid_t src,
- int rate, int n_readings)
+ double rate, int n_readings)
{
uint64_t ts;
int64_t offset, delay;
@@ -536,7 +536,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"
- " -R [rate] slave clock update rate in HZ (1)\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"
" -u [num] number of clock updates in summary stats (0)\n"
@@ -556,10 +556,10 @@ int main(int argc, char *argv[])
{
char *progname, *ethdev = NULL;
clockid_t src = CLOCK_INVALID;
- int c, domain_number = 0, phc_readings = 5, phc_rate = 1, pps_fd = -1;
+ 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;
+ double ppb, phc_rate = 1.0;
struct clock dst_clock = {
.clkid = CLOCK_REALTIME,
.servo_state = SERVO_UNLOCKED,
@@ -599,7 +599,7 @@ int main(int argc, char *argv[])
configured_pi_offset = atof(optarg);
break;
case 'R':
- phc_rate = atoi(optarg);
+ phc_rate = atof(optarg);
break;
case 'N':
phc_readings = atoi(optarg);

[Richard Cochran]

Post by Miroslav Lichvar
This allows update intervals longer than 1 second.
---
phc2sys.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

Applied.

Could I ask you to follow up this patch by replacing the call to
usleep() with nanosleep()? The intent of usleep seems to be for
intervals under a second.

Thanks,
Richard

[Richard Cochran]

Post by Miroslav Lichvar
With sub-second sync intervals, it may be useful to set P and I to
values over 1.0.

Not sure this is really a good idea. Doesn't this mean amplifying the
error signal?

See my other email from today about the "scaling" constants.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
With sub-second sync intervals, it may be useful to set P and I to
values over 1.0.

Not sure this is really a good idea. Doesn't this mean amplifying the
error signal?

Not necessarily. With very small jitters and/or poor clock stability,
constants over 1.0 could give better results.

It seems the limit was used to ensure the stability of the servo, but
as it depends on the update interval, which is unknown at the config
parsing time, I'd leave it up to the user to set the constants
correctly.

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran

Post by Miroslav Lichvar
With sub-second sync intervals, it may be useful to set P and I to
values over 1.0.

Not sure this is really a good idea. Doesn't this mean amplifying the
error signal?

Not necessarily. With very small jitters and/or poor clock stability,
constants over 1.0 could give better results.
It seems the limit was used to ensure the stability of the servo, but
as it depends on the update interval, which is unknown at the config
parsing time, I'd leave it up to the user to set the constants
correctly.

According to the analysis in Eidson [1] chapter 5, the stability
region for PI constants is the area under the line P=2-I/2, plotting P
as a function of I. This is independent of the update interval, except
that the computational delay might spoil things at very high update
rates.

I'll go ahead and add an sanity check that all user supplied constants
are within this region.

Thanks,
Richard

1. Eidson, John.
Measurement, Control, and Communication Using IEEE 1588.

If you don't already have this book, I can recommend it as one of
the only works on the topic of network time keeping. With a price
tag of $170, I certainly can't afford it myself, but I did read it
once. The analysis on PI servo design is found in chapter 5, which
you can purchase separately from Springer in PDF format for about
$30 (which is what I have done this week).

[Miroslav Lichvar]

Post by Richard Cochran
According to the analysis in Eidson [1] chapter 5, the stability
region for PI constants is the area under the line P=2-I/2, plotting P
as a function of I.

Nice, that seems to describe the region of instability in the
simulation results wery well. We could use it in the optimal P, I
formulas instead of the rectangle.

Post by Richard Cochran
This is independent of the update interval, except

I think in the linuxptp implementation the update interval has to be
included, i.e. P=2/u-I/2, otherwise it makes no sense. What exactly
are they controlling with the servo? Perhaps it's an offset adjustment
applied once per the interval instead of the frequency offset?

Post by Richard Cochran
that the computational delay might spoil things at very high update
rates.

Yes, but that can be addressed by measuring the actual update
interval.

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar
I think in the linuxptp implementation the update interval has to be
included, i.e. P=2/u-I/2, otherwise it makes no sense.

I misread the Eidson book. In the stability treatment, P=Kp*Kc*T and
I=Ki*Kc*T, where T is the sample period and Kc is the gain from the
plant (ie the clock).

So accounting for the update interval, you do get P=2/u-I/2.

Thanks,
Richard

[Richard Cochran]

Post by Miroslav Lichvar
With sub-second sync intervals, it may be useful to set P and I to
values over 1.0.
---
config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Applied.

Thanks,
Richard