[Linuxptp-devel] [v3 PATCH 0/5] Series short description

[Jacob Keller]

The following series updates the servo selection code to enable easier selection
of new servo types. The intent behind this code originally was to add a new jump
servo which would jump the clock when beyond a certain value. However due to the
similarity to the pi servo, this code has been added as a configurable feature
of the pi servo. The new default is to never jump unless the offset is
configured greater than 0.0

The first 3 patches modify ptp4l's clock servo selection code by creating an
enum, and removing reliance on the string value. It also adds a configuration
option to the config file (though it is mostly useless since there is only one
servo at this time)

The last two patches modify the pi servo with the new option and update the
configuration files to show the use of the new options

-v2-
* Pull jump servo code into the pi servo

-v3-
* Fix bug in forgetting to multiply by NSEC_PER_SEC

---

Jacob Keller (5):
ptp4l: modify servo setup to take an enum rather than string
ptp4l: modify clock_create to take servo as argument
ptp4l: add servo selection to the configuration file
ptp4l: add maximum offset to pi servo
ptp4l: update the configuration files


clock.c | 5 +++--
clock.h | 4 +++-
config.c | 12 ++++++++++++
config.h | 6 ++++++
default.cfg | 5 +++++
gPTP.cfg | 5 +++++
pi.c | 24 ++++++++++++++++++++++++
pi.h | 9 +++++++++
ptp4l.c | 7 ++++++-
servo.c | 4 ++--
servo.h | 11 +++++++++--
11 files changed, 84 insertions(+), 8 deletions(-)

--
Signature

[Jacob Keller]

passing a string as the servo type seems ugly when there are only a few
choices. This patch modifies the servo_create to take an enum instead.

Signed-off-by: Jacob Keller <***@intel.com>
---
clock.c | 2 +-
servo.c | 4 ++--
servo.h | 11 +++++++++--
3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/clock.c b/clock.c
index dc0e6c6..9b2c374 100644
--- a/clock.c
+++ b/clock.c
@@ -437,7 +437,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
if (c->clkid != CLOCK_INVALID) {
fadj = (int) clock_ppb_read(c->clkid);
}
- c->servo = servo_create("pi", -fadj, max_adj, sw_ts);
+ c->servo = servo_create(CLOCK_SERVO_PI, -fadj, max_adj, sw_ts);
if (!c->servo) {
pr_err("Failed to create clock servo");
return NULL;
diff --git a/servo.c b/servo.c
index 8a644aa..97c2e60 100644
--- a/servo.c
+++ b/servo.c
@@ -21,9 +21,9 @@
#include "pi.h"
#include "servo_private.h"

-struct servo *servo_create(char *name, int fadj, int max_ppb, int sw_ts)
+struct servo *servo_create(enum servo_type type, int fadj, int max_ppb, int sw_ts)
{
- if (!strncmp(name, "pi", 2)) {
+ if (type == CLOCK_SERVO_PI) {
return pi_servo_create(fadj, max_ppb, sw_ts);
}
return NULL;
diff --git a/servo.h b/servo.h
index 0147d79..0d2ab71 100644
--- a/servo.h
+++ b/servo.h
@@ -24,6 +24,13 @@
struct servo;

/**
+ * Defines the available servo cores
+ */
+enum servo_type {
+ CLOCK_SERVO_PI,
+};
+
+/**
* Defines the caller visible states of a clock servo.
*/
enum servo_state {
@@ -47,7 +54,7 @@ enum servo_state {

/**
* Create a new instance of a clock servo.
- * @param name The name of the servo flavor to create.
+ * @param type The type of the servo to create.
* @param fadj The clock's current adjustment in parts per billion.
* @param max_ppb The absolute maxinum adjustment allowed by the clock
* in parts per billion. The clock servo will clamp its
@@ -56,7 +63,7 @@ enum servo_state {
* and the servo should use more aggressive filtering.
* @return A pointer to a new servo on success, NULL otherwise.
*/
-struct servo *servo_create(char *name, int fadj, int max_ppb, int sw_ts);
+struct servo *servo_create(enum servo_type type, int fadj, int max_ppb, int sw_ts);

/**
* Destroy an instance of a clock servo.

[Jacob Keller]

this patch modifies the clock_create to take the servo type as an argument

Signed-off-by: Jacob Keller <***@intel.com>
---
clock.c | 5 +++--
clock.h | 4 +++-
ptp4l.c | 2 +-
3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/clock.c b/clock.c
index 9b2c374..beda1da 100644
--- a/clock.c
+++ b/clock.c
@@ -398,7 +398,8 @@ UInteger8 clock_class(struct clock *c)
}

struct clock *clock_create(int phc_index, struct interface *iface, int count,
- enum timestamp_type timestamping, struct defaultDS *ds)
+ enum timestamp_type timestamping, struct defaultDS *ds,
+ enum servo_type servo)
{
int i, fadj = 0, max_adj, sw_ts = timestamping == TS_SOFTWARE ? 1 : 0;
struct clock *c = &the_clock;
@@ -437,7 +438,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
if (c->clkid != CLOCK_INVALID) {
fadj = (int) clock_ppb_read(c->clkid);
}
- c->servo = servo_create(CLOCK_SERVO_PI, -fadj, max_adj, sw_ts);
+ c->servo = servo_create(servo, -fadj, max_adj, sw_ts);
if (!c->servo) {
pr_err("Failed to create clock servo");
return NULL;
diff --git a/clock.h b/clock.h
index 7e60334..ca2071d 100644
--- a/clock.h
+++ b/clock.h
@@ -66,10 +66,12 @@ UInteger8 clock_class(struct clock *c);
* @param count The number of elements in @a interfaces.
* @param timestamping The timestamping mode for this clock.
* @param ds A pointer to a default data set for the clock.
+ * @param servo The servo that this clock will use.
* @return A pointer to the single global clock instance.
*/
struct clock *clock_create(int phc_index, struct interface *iface, int count,
- enum timestamp_type timestamping, struct defaultDS *ds);
+ enum timestamp_type timestamping, struct defaultDS *ds,
+ enum servo_type servo);

/**
* Obtains a clock's default data set.
diff --git a/ptp4l.c b/ptp4l.c
index 6fc210a..b35f559 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -272,7 +272,7 @@ int main(int argc, char *argv[])
}

clock = clock_create(phc_index, iface, cfg_settings.nports,
- *timestamping, ds);
+ *timestamping, ds, CLOCK_SERVO_PI);
if (!clock) {
fprintf(stderr, "failed to create a clock\n");
return -1;

[Jacob Keller]

this patch adds the servo selection to the configuration file

Signed-off-by: Jacob Keller <***@intel.com>
---
config.c | 7 +++++++
config.h | 4 ++++
ptp4l.c | 5 ++++-
3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 2c66d6b..79341b6 100644
--- a/config.c
+++ b/config.c
@@ -303,6 +303,13 @@ static void scan_global_line(const char *s, struct config *cfg)
cfg->transport = TRANS_IEEE_802_3;

}
+
+ } else if (1 == sscanf(s, " clock_servo %1023s", string)) {
+
+ if (0 == strcasecmp("pi", string))
+
+ cfg->clock_servo = CLOCK_SERVO_PI;
+
}
}

diff --git a/config.h b/config.h
index 7c52225..9691478 100644
--- a/config.h
+++ b/config.h
@@ -23,6 +23,7 @@
#include "ds.h"
#include "dm.h"
#include "transport.h"
+#include "servo.h"

#define MAX_PORTS 8
#define MAX_IFNAME_SIZE 16
@@ -60,6 +61,9 @@ struct config {
int *assume_two_step;
int *tx_timestamp_retries;
int *rx_timestamp_l2only;
+
+ enum servo_type clock_servo;
+
double *pi_proportional_const;
double *pi_integral_const;
unsigned char *ptp_dst_mac;
diff --git a/ptp4l.c b/ptp4l.c
index b35f559..7a2ef61 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -65,6 +65,9 @@ static struct config cfg_settings = {
.assume_two_step = &assume_two_step,
.tx_timestamp_retries = &sk_tx_retries,
.rx_timestamp_l2only = &sk_prefer_layer2,
+
+ .clock_servo = CLOCK_SERVO_PI,
+
.pi_proportional_const = &configured_pi_kp,
.pi_integral_const = &configured_pi_ki,
.ptp_dst_mac = ptp_dst_mac,
@@ -272,7 +275,7 @@ int main(int argc, char *argv[])
}

clock = clock_create(phc_index, iface, cfg_settings.nports,
- *timestamping, ds, CLOCK_SERVO_PI);
+ *timestamping, ds, cfg_settings.clock_servo);
if (!clock) {
fprintf(stderr, "failed to create a clock\n");
return -1;

[Jacob Keller]

this patch updates the default.cfg and gPTP.cfg to show the new defaults

Signed-off-by: Jacob Keller <***@intel.com>
---
default.cfg | 5 +++++
gPTP.cfg | 5 +++++
2 files changed, 10 insertions(+)

diff --git a/default.cfg b/default.cfg
index f87b8d5..eeb0b48 100644
--- a/default.cfg
+++ b/default.cfg
@@ -30,8 +30,13 @@ tx_timestamp_retries 2
rx_timestamp_l2only 0
use_syslog 1
verbose 0
+#
+# Servo Options
+#
pi_proportional_const 0.0
pi_integral_const 0.0
+pi_offset_const 0.0
+clock_servo pi
#
# Transport options
#
diff --git a/gPTP.cfg b/gPTP.cfg
index fab2694..07cc2c2 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -30,8 +30,13 @@ tx_timestamp_retries 2
rx_timestamp_l2only 1
use_syslog 1
verbose 0
+#
+# Servo options
+#
pi_proportional_const 0.0
pi_integral_const 0.0
+pi_offset_const 0.0
+clock_servo pi
#
# Transport options
#

[Jacob Keller]

this patch modifies the pi servo to add a configurable max offset (default
infinity). When ever the detected offset is larger than this value, the clock
will jump and reset the servo state. The value of this feature is for decreasing
time to stabalize when clock is off by a large ammount during late running. This
can occur when the upstream master changes, or when the clock is reset due to
outside forces. The method used to reset clock is simply to reset the pi servo
to the unlocked state.

Signed-off-by: Jacob Keller <***@intel.com>
---
config.c | 5 +++++
config.h | 2 ++
pi.c | 24 ++++++++++++++++++++++++
pi.h | 9 +++++++++
ptp4l.c | 2 ++
5 files changed, 42 insertions(+)

diff --git a/config.c b/config.c
index 79341b6..f12ebc5 100644
--- a/config.c
+++ b/config.c
@@ -223,6 +223,11 @@ static void scan_global_line(const char *s, struct config *cfg)
if (df > 0.0 && df < 1.0)
*cfg->pi_integral_const = df;

+ } else if (1 == sscanf(s, " pi_offset_const %lf", &df)) {
+
+ if (df > 0.0)
+ *cfg->pi_offset_const = df;
+
} else if (MAC_LEN == sscanf(s, " ptp_dst_mac %hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
&mac[0], &mac[1], &mac[2], &mac[3], &mac[4], &mac[5])) {

diff --git a/config.h b/config.h
index 9691478..8840b41 100644
--- a/config.h
+++ b/config.h
@@ -66,6 +66,8 @@ struct config {

double *pi_proportional_const;
double *pi_integral_const;
+ double *pi_offset_const;
+
unsigned char *ptp_dst_mac;
unsigned char *p2p_dst_mac;

diff --git a/pi.c b/pi.c
index d003587..cf83f9f 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 <math.h>

#include "pi.h"
#include "servo_private.h"
@@ -28,9 +29,12 @@
#define SWTS_KP 0.1
#define SWTS_KI 0.001

+#define NSEC_PER_SEC 1000000000
+
/* These two take their values from the configuration file. (see ptp4l.c) */
double configured_pi_kp;
double configured_pi_ki;
+double configured_pi_offset;

struct pi_servo {
struct servo servo;
@@ -40,6 +44,7 @@ struct pi_servo {
double maxppb;
double kp;
double ki;
+ double max_offset;
int count;
};

@@ -81,6 +86,19 @@ static double pi_sample(struct servo *servo,
s->count = 4;
break;
case 4:
+ /*
+ * reset the clock servo when offset is greater than the max
+ * offset value. Note that the clock jump will be performed in
+ * step 3, so it is not necessary to have clock jump
+ * immediately. This allows re-calculating drift as in initial
+ * clock startup.
+ */
+ if (s->max_offset && (s->max_offset < fabs(offset))) {
+ *state = SERVO_UNLOCKED;
+ s->count = 0;
+ break;
+ }
+
ki_term = s->ki * offset;
ppb = s->kp * offset + s->drift + ki_term;
if (ppb < -s->maxppb) {
@@ -121,5 +139,11 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->ki = HWTS_KI;
}

+ if (configured_pi_offset > 0.0) {
+ s->max_offset = configured_pi_offset * NSEC_PER_SEC;
+ } else {
+ s->max_offset = 0.0;
+ }
+
return &s->servo;
}
diff --git a/pi.h b/pi.h
index 2208832..eff90aa 100644
--- a/pi.h
+++ b/pi.h
@@ -33,6 +33,15 @@ extern double configured_pi_kp;
*/
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
+ *
+ * Note that this variable is measured in seconds, and allows fractional values.
+ */
+extern double configured_pi_offset;
+
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts);

#endif
diff --git a/ptp4l.c b/ptp4l.c
index 7a2ef61..9a9ba9e 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -70,6 +70,8 @@ static struct config cfg_settings = {

.pi_proportional_const = &configured_pi_kp,
.pi_integral_const = &configured_pi_ki,
+ .pi_offset_const = &configured_pi_offset,
+
.ptp_dst_mac = ptp_dst_mac,
.p2p_dst_mac = p2p_dst_mac,

[Richard Cochran]

Post by Jacob Keller
The following series updates the servo selection code to enable easier selection
of new servo types. The intent behind this code originally was to add a new jump
servo which would jump the clock when beyond a certain value. However due to the
similarity to the pi servo, this code has been added as a configurable feature
of the pi servo. The new default is to never jump unless the offset is
configured greater than 0.

Applied, with tiny follow up patch.

Thanks,
Richard