[Linuxptp-devel] [PATCH 1/2] Increase default first step threshold to 20 us.

[Miroslav Lichvar]

Most PHC drivers implement stepping (ADJ_SETOFFSET) by reading the
clock, adjusting the value by the offset and writing it back. This is
not perfectly accurate and if the operation is slow (e.g. due to PCIe
latencies), the error can be in microseconds.

Increase the default first step threshold from 100 nanoseconds to 20
microseconds to step only when the initial offset is larger than
the error in the step.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
default.cfg | 2 +-
gPTP.cfg | 2 +-
phc2sys.8 | 4 ++--
phc2sys.c | 2 +-
ptp4l.8 | 2 +-
servo.c | 2 +-
6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/default.cfg b/default.cfg
index 7038b69..9f01eda 100644
--- a/default.cfg
+++ b/default.cfg
@@ -49,7 +49,7 @@ pi_integral_scale 0.0
pi_integral_exponent 0.4
pi_integral_norm_max 0.3
step_threshold 0.0
-first_step_threshold 0.0000001
+first_step_threshold 0.00002
max_frequency 900000000
clock_servo pi
sanity_freq_limit 200000000
diff --git a/gPTP.cfg b/gPTP.cfg
index fac2aa0..4d0a38c 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -49,7 +49,7 @@ pi_integral_scale 0.0
pi_integral_exponent 0.4
pi_integral_norm_max 0.3
step_threshold 0.0
-first_step_threshold 0.0000001
+first_step_threshold 0.00002
max_frequency 900000000
clock_servo pi
sanity_freq_limit 200000000
diff --git a/phc2sys.8 b/phc2sys.8
index 8688e48..fa3ae20 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -106,7 +106,7 @@ Specify the integral constant of the PI controller. The default is 0.3.
Specify the step threshold of the servo. It is the maximum offset that
the servo corrects by changing the clock frequency instead of stepping the
clock. The clock is stepped on start regardless of the option if the offset is
-larger than 100 nanoseconds (unless the
+larger than 20 microseconds (unless the
.BI \-F
option is used). It's specified in seconds. The value of 0.0 disables stepping
after the start. The default is 0.0.
@@ -115,7 +115,7 @@ after the start. The default is 0.0.
Specify the step threshold applied only on the first update. It is the maximum
offset that is corrected by adjusting clock. It's specified in seconds. The
value of 0.0 disables stepping on start.
-The default is 0.0000001 (100 nanoseconds).
+The default is 0.00002 (20 microseconds).
.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 6221515..5ecb602 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -565,7 +565,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] step threshold only on start (0.0000001)\n"
+ " -F [step] step threshold only on start (0.00002)\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"
diff --git a/ptp4l.8 b/ptp4l.8
index edc7045..cbca9bc 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -355,7 +355,7 @@ The maximum offset the servo will correct by changing the clock
frequency instead of stepping the clock. This is only applied on the first
update. It's specified in seconds. When set to 0.0, the servo won't step
the clock on start.
-The default is 0.0000001 (100 nanoseconds).
+The default is 0.00002 (20 microseconds).
This option used to be called (and can still be set by)
.BR pi_f_offset_const .
.TP
diff --git a/servo.c b/servo.c
index 41e5c9f..6986e41 100644
--- a/servo.c
+++ b/servo.c
@@ -25,7 +25,7 @@
#define NSEC_PER_SEC 1000000000

double servo_step_threshold = 0.0;
-double servo_first_step_threshold = 0.0000001; /* 100 nanoseconds */
+double servo_first_step_threshold = 0.00002; /* 20 microseconds */
int servo_max_frequency = 900000000;

struct servo *servo_create(enum servo_type type, int fadj, int max_ppb, int sw_ts)

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
config.c | 37 +++++++++++++++++++++++++++++--------
ptp4l.8 | 6 +++---
2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 567256c..e0b786b 100644
--- a/config.c
+++ b/config.c
@@ -383,22 +383,19 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->pi_integral_norm_max = df;

- } else if (!strcmp(option, "step_threshold") ||
- !strcmp(option, "pi_offset_const")) {
+ } else if (!strcmp(option, "step_threshold")) {
r = get_ranged_double(value, &df, 0.0, DBL_MAX);
if (r != PARSED_OK)
return r;
*cfg->step_threshold = df;

- } else if (!strcmp(option, "first_step_threshold") ||
- !strcmp(option, "pi_f_offset_const")) {
+ } else if (!strcmp(option, "first_step_threshold")) {
r = get_ranged_double(value, &df, 0.0, DBL_MAX);
if (r != PARSED_OK)
return r;
*cfg->first_step_threshold = df;

- } else if (!strcmp(option, "max_frequency") ||
- !strcmp(option, "pi_max_frequency")) {
+ } else if (!strcmp(option, "max_frequency")) {
r = get_ranged_int(value, &val, 0, INT_MAX);
if (r != PARSED_OK)
return r;
@@ -563,7 +560,9 @@ static enum parser_result parse_global_setting(const char *option,
return PARSED_OK;
}

-static enum parser_result parse_setting_line(char *line, char **option, char **value)
+static enum parser_result parse_setting_line(char *line,
+ const char **option,
+ const char **value)
{
*option = line;

@@ -583,12 +582,32 @@ static enum parser_result parse_setting_line(char *line, char **option, char **v
return PARSED_OK;
}

+static void check_deprecated_options(const char **option)
+{
+ const char *new_option = NULL;
+
+ if (!strcmp(*option, "pi_offset_const")) {
+ new_option = "step_threshold";
+ } else if (!strcmp(*option, "pi_f_offset_const")) {
+ new_option = "first_step_threshold";
+ } else if (!strcmp(*option, "pi_max_frequency")) {
+ new_option = "max_frequency";
+ }
+
+ if (new_option) {
+ fprintf(stderr, "option %s is deprecated, please use %s instead\n",
+ *option, new_option);
+ *option = new_option;
+ }
+}
+
int config_read(char *name, struct config *cfg)
{
enum config_section current_section = UNKNOWN_SECTION;
enum parser_result parser_res;
FILE *fp;
- char buf[1024], *line, *c, *option, *value;
+ char buf[1024], *line, *c;
+ const char *option, *value;
int current_port = 0, line_num;

fp = 0 == strncmp(name, "-", 2) ? stdin : fopen(name, "r");
@@ -642,6 +661,8 @@ int config_read(char *name, struct config *cfg)
goto parse_error;
}

+ check_deprecated_options(&option);
+
if (current_section == GLOBAL_SECTION)
parser_res = parse_global_setting(option, value, cfg);
else
diff --git a/ptp4l.8 b/ptp4l.8
index cbca9bc..0f06ee1 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -347,7 +347,7 @@ The maximum offset the servo will correct by changing the clock
frequency instead of stepping the clock. When set to 0.0, the servo will
never step the clock except on start. It's specified in seconds.
The default is 0.0.
-This option used to be called (and can still be set by)
+This option used to be called
.BR pi_offset_const .
.TP
.B first_step_threshold
@@ -356,7 +356,7 @@ frequency instead of stepping the clock. This is only applied on the first
update. It's specified in seconds. When set to 0.0, the servo won't step
the clock on start.
The default is 0.00002 (20 microseconds).
-This option used to be called (and can still be set by)
+This option used to be called
.BR pi_f_offset_const .
.TP
.B max_frequency
@@ -364,7 +364,7 @@ The maximum allowed frequency adjustment of the clock in parts per billion
(ppb). This is an additional limit to the maximum allowed by the hardware. When
set to 0, the hardware limit will be used.
The default is 900000000 (90%).
-This option used to be called (and can still be set by)
+This option used to be called
.BR pi_max_frequency .
.TP
.B sanity_freq_limit

[Richard Cochran]

Post by Miroslav Lichvar
---
config.c | 37 +++++++++++++++++++++++++++++--------
ptp4l.8 | 6 +++---
2 files changed, 32 insertions(+), 11 deletions(-)

Applied.

Thanks,
Richard

[Richard Cochran]

Post by Miroslav Lichvar
Most PHC drivers implement stepping (ADJ_SETOFFSET) by reading the
clock, adjusting the value by the offset and writing it back. This is
not perfectly accurate and if the operation is slow (e.g. due to PCIe
latencies), the error can be in microseconds.
Increase the default first step threshold from 100 nanoseconds to 20
microseconds to step only when the initial offset is larger than
the error in the step.
---

Applied.

Thanks,
Richard