Discussion:
[Linuxptp-devel] [PATCH RFC v2 00/19] Improved slave servo and grand master support
Richard Cochran
2013-07-14 20:01:54 UTC
Permalink
* Changes in V2
- for clarity, use switch/case in do_set_action in pmc.c
- omit the '=' in the SET command
- correct a commit message
- after changing GM settings, throw a BMC calculation

Here is my patch queue with a few different topics.

* Patches 1-3 are just simple clean ups found along the way.
* Patch 4 is a bug fix for ports in PASSIVE mode.
* 5-7 add missing configuration options related to grand masters.
* 8-14 add a way to let an external program set grand master attribues.
* 15-18 add the clock servo improvement recently under discussion.
* 19 fixes the BMC logic for the degenerate case of just one clock.

Thanks,
Richard


Miroslav Lichvar (2):
phc2sys: Set servo sync interval.
Adjust constants of PI servo according to sync interval.

Richard Cochran (17):
No need to set kernel_leap twice in a row.
Clean up indented white space.
pmc: fix coding style by using K&R style functions.
Reset announce timer when port is passive.
Add missing option to the default configuration file.
Add a clock variable to hold the value of the time source.
Add a configuration option for the time source.
Introduce a non-portable management TLV for grand masters.
Support the grand master settings management query.
pmc: Support the grandmaster settings management request.
Support configuration of the grand master settings.
Throw a state decision event if the clock quality changes.
pmc: add a utility function for sending a message with the SET
action.
pmc: add our very first SET command.
Let the clock servo know the expected sync interval.
Use a dynamic frequency estimation interval.
Become the grand master when all alone.

bmc.c | 3 --
clock.c | 104 ++++++++++++++++++++++++++++----------------
clock.h | 4 +-
config.c | 42 ++++++++++++++++++
config.h | 6 +++
default.cfg | 8 ++++
ds.h | 1 +
gPTP.cfg | 6 +++
phc2sys.c | 6 ++-
pi.c | 75 ++++++++++++++++++++++++++------
pi.h | 44 +++++++++++++++++++
pmc.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++------
pmc_common.c | 23 ++++++++++
pmc_common.h | 2 +
port.c | 16 ++++---
ptp4l.8 | 54 ++++++++++++++++++++---
ptp4l.c | 7 +++
servo.c | 5 +++
servo.h | 7 +++
servo_private.h | 2 +
tlv.c | 24 +++++++++--
tlv.h | 8 ++++
22 files changed, 492 insertions(+), 84 deletions(-)
--
1.7.10.4
Richard Cochran
2013-07-14 20:01:56 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
tlv.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tlv.c b/tlv.c
index ce72e4b..b50c637 100644
--- a/tlv.c
+++ b/tlv.c
@@ -52,7 +52,7 @@ static uint16_t flip16(uint16_t *p) {
}

static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
- struct tlv_extra *extra)
+ struct tlv_extra *extra)
{
struct defaultDS *dds;
struct currentDS *cds;
@@ -75,7 +75,7 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,

cd->physicalLayerProtocol = (struct PTPText *) buf;
buf += sizeof(struct PTPText);
- buf += cd->physicalLayerProtocol->length;
+ buf += cd->physicalLayerProtocol->length;

cd->physicalAddress = (struct PhysicalAddress *) buf;
u16 = flip16(&cd->physicalAddress->length);
@@ -105,9 +105,9 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
extra_len = buf - m->data;
break;
case USER_DESCRIPTION:
- extra->cd.userDescription = (struct PTPText *) m->data;
+ extra->cd.userDescription = (struct PTPText *) m->data;
extra_len = sizeof(struct PTPText);
- extra_len += extra->cd.userDescription->length;
+ extra_len += extra->cd.userDescription->length;
break;
case DEFAULT_DATA_SET:
if (data_len != sizeof(struct defaultDS))
--
1.7.10.4
Richard Cochran
2013-07-14 20:01:55 UTC
Permalink
This patch removes a redundant initialization of the kernel_leap clock
variable. The field is already set in clock_create a few lines earlier.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/clock.c b/clock.c
index aace086..274a6a0 100644
--- a/clock.c
+++ b/clock.c
@@ -596,7 +596,6 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
sysclk_set_leap(0);
}
c->leap_set = 0;
- c->kernel_leap = dds->kernel_leap;

if (c->clkid != CLOCK_INVALID) {
fadj = (int) clockadj_get_freq(c->clkid);
--
1.7.10.4
Richard Cochran
2013-07-14 20:01:57 UTC
Permalink
---
pmc.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/pmc.c b/pmc.c
index 732297a..66b57fe 100644
--- a/pmc.c
+++ b/pmc.c
@@ -115,7 +115,8 @@ static char *action_string[] = {

#define IFMT "\n\t\t"

-static char *text2str(struct PTPText *text) {
+static char *text2str(struct PTPText *text)
+{
static struct static_ptp_text s;
s.max_symbols = -1;
static_ptp_text_copy(&s, text);
@@ -125,7 +126,8 @@ static char *text2str(struct PTPText *text) {
#define MAX_PRINT_BYTES 16
#define BIN_BUF_SIZE (MAX_PRINT_BYTES * 3 + 1)

-static char *bin2str_impl(Octet *data, int len, char *buf, int buf_len) {
+static char *bin2str_impl(Octet *data, int len, char *buf, int buf_len)
+{
int i, offset = 0;
if (len > MAX_PRINT_BYTES)
len = MAX_PRINT_BYTES;
@@ -143,18 +145,21 @@ static char *bin2str_impl(Octet *data, int len, char *buf, int buf_len) {
return buf;
}

-static char *bin2str(Octet *data, int len) {
+static char *bin2str(Octet *data, int len)
+{
static char buf[BIN_BUF_SIZE];
return bin2str_impl(data, len, buf, sizeof(buf));
}

-static uint16_t align16(uint16_t *p) {
+static uint16_t align16(uint16_t *p)
+{
uint16_t v;
memcpy(&v, p, sizeof(v));
return v;
}

-static char *portaddr2str(struct PortAddress *addr) {
+static char *portaddr2str(struct PortAddress *addr)
+{
static char buf[BIN_BUF_SIZE];
switch(align16(&addr->networkProtocol)) {
case TRANS_UDP_IPV4:
--
1.7.10.4
Richard Cochran
2013-07-14 20:01:58 UTC
Permalink
Whenever a port enters the passive state, it should act like a slaved
port in one respect. Incoming announce messages from the grand master
are supposed to reset the announce timer. This patch fixes the port
logic to properly maintain the passive state.

Signed-off-by: Richard Cochran <***@gmail.com>
Reported-by: Rohrer Hansjoerg <***@mobatime.com>
---
port.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/port.c b/port.c
index d29a70c..c4505d8 100644
--- a/port.c
+++ b/port.c
@@ -1236,11 +1236,12 @@ static int update_current_master(struct port *p, struct ptp_message *m)
if (!msg_source_equal(m, fc))
return add_foreign_master(p, m);

- tds.currentUtcOffset = m->announce.currentUtcOffset;
- tds.flags = m->header.flagField[1];
- tds.timeSource = m->announce.timeSource;
- clock_update_time_properties(p->clock, tds);
-
+ if (p->state != PS_PASSIVE) {
+ tds.currentUtcOffset = m->announce.currentUtcOffset;
+ tds.flags = m->header.flagField[1];
+ tds.timeSource = m->announce.timeSource;
+ clock_update_time_properties(p->clock, tds);
+ }
if (p->pod.path_trace_enabled) {
ptt = (struct path_trace_tlv *) m->announce.suffix;
dad = clock_parent_ds(p->clock);
@@ -1281,9 +1282,9 @@ static int process_announce(struct port *p, struct ptp_message *m)
case PS_PRE_MASTER:
case PS_MASTER:
case PS_GRAND_MASTER:
- case PS_PASSIVE:
result = add_foreign_master(p, m);
break;
+ case PS_PASSIVE:
case PS_UNCALIBRATED:
case PS_SLAVE:
result = update_current_master(p, m);
--
1.7.10.4
Richard Cochran
2013-07-14 20:01:59 UTC
Permalink
While the userDescription field is implemented in the code, the same
option is not present in the sample configuration file. This patch
fixes the issue by adding the option with an empty default value.

Signed-off-by: Richard Cochran <***@gmail.com>
Reported-by: Rohrer Hansjoerg <***@mobatime.com>
---
default.cfg | 1 +
1 file changed, 1 insertion(+)

diff --git a/default.cfg b/default.cfg
index 54b0aae..6297925 100644
--- a/default.cfg
+++ b/default.cfg
@@ -63,3 +63,4 @@ time_stamping hardware
productDescription ;;
revisionData ;;
manufacturerIdentity 00:00:00
+userDescription ;
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:00 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 4 +++-
ds.h | 1 +
ptp4l.c | 1 +
3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index 274a6a0..ea2d97e 100644
--- a/clock.c
+++ b/clock.c
@@ -79,6 +79,7 @@ struct clock {
int utc_timescale;
int leap_set;
int kernel_leap;
+ int time_source; /* grand master role */
enum servo_state servo_state;
tmv_t master_offset;
tmv_t path_delay;
@@ -436,7 +437,7 @@ static void clock_update_grandmaster(struct clock *c)
} else {
c->tds.flags = PTP_TIMESCALE;
}
- c->tds.timeSource = INTERNAL_OSCILLATOR;
+ c->tds.timeSource = c->time_source;
}

static void clock_update_slave(struct clock *c)
@@ -570,6 +571,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
c->free_running = dds->free_running;
c->freq_est_interval = dds->freq_est_interval;
c->kernel_leap = dds->kernel_leap;
+ c->time_source = dds->time_source;
c->desc = dds->clock_desc;

if (c->free_running) {
diff --git a/ds.h b/ds.h
index f653aee..9977e40 100644
--- a/ds.h
+++ b/ds.h
@@ -54,6 +54,7 @@ struct default_ds {
int freq_est_interval; /*log seconds*/
int stats_interval; /*log seconds*/
int kernel_leap;
+ int time_source;
struct clock_description clock_desc;
};

diff --git a/ptp4l.c b/ptp4l.c
index 1e0304e..632166c 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -54,6 +54,7 @@ static struct config cfg_settings = {
.freq_est_interval = 1,
.stats_interval = 0,
.kernel_leap = 1,
+ .time_source = INTERNAL_OSCILLATOR,
.clock_desc = {
.productDescription = {
.max_symbols = 64,
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:01 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 6 ++++++
default.cfg | 1 +
ptp4l.8 | 8 +++++++-
3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index ad63bba..854a703 100644
--- a/config.c
+++ b/config.c
@@ -445,6 +445,12 @@ static enum parser_result parse_global_setting(const char *option,
return r;
cfg->dds.kernel_leap = val;

+ } else if (!strcmp(option, "timeSource")) {
+ r = get_ranged_int(value, &val, 0x10, 0xfe);
+ if (r != PARSED_OK)
+ return r;
+ cfg->dds.time_source = val;
+
} else
return NOT_PARSED;

diff --git a/default.cfg b/default.cfg
index 6297925..e10d257 100644
--- a/default.cfg
+++ b/default.cfg
@@ -64,3 +64,4 @@ productDescription ;;
revisionData ;;
manufacturerIdentity 00:00:00
userDescription ;
+timeSource 0xA0
diff --git a/ptp4l.8 b/ptp4l.8
index fb49b21..5a9e8b4 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -1,4 +1,4 @@
-.TH PTP4l 8 "November 2012" "linuxptp"
+.TH PTP4l 8 "July 2013" "linuxptp"
.SH NAME
ptp4l \- PTP Boundary/Ordinary Clock

@@ -350,6 +350,12 @@ one-second offset slowly by changing the clock frequency (unless the
.B pi_offset_const
option is set to correct such offset by stepping).
Relevant only with software time stamping. The default is 1 (enabled).
+.TP
+.B timeSource
+The time source is a single byte code that gives an idea of the kind
+of local clock in use. The value is purely informational, having no
+effect on the outcome of the Best Master Clock algorithm, and is
+advertised when the clock becomes grand master.

.SH TIME SCALE USAGE
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:02 UTC
Permalink
When running as grand master, the attributes of the local clock are not
known by the ptp4l program. Since these attributes may change over time,
for example when losing signal from GPS satellites, we need to have a
way to provide updated information at run time. This patch provides a
new TLV intended for local IPC that contains the required settings.

Signed-off-by: Richard Cochran <***@gmail.com>
---
tlv.c | 16 ++++++++++++++++
tlv.h | 8 ++++++++
2 files changed, 24 insertions(+)

diff --git a/tlv.c b/tlv.c
index b50c637..35a3aa0 100644
--- a/tlv.c
+++ b/tlv.c
@@ -60,6 +60,7 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
struct timePropertiesDS *tp;
struct portDS *p;
struct time_status_np *tsn;
+ struct grandmaster_settings_np *gsn;
struct mgmt_clock_description *cd;
int extra_len = 0;
uint8_t *buf;
@@ -163,6 +164,14 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
scaled_ns_n2h(&tsn->lastGmPhaseChange);
tsn->gmPresent = ntohl(tsn->gmPresent);
break;
+ case GRANDMASTER_SETTINGS_NP:
+ if (data_len != sizeof(struct grandmaster_settings_np))
+ goto bad_length;
+ gsn = (struct grandmaster_settings_np *) m->data;
+ gsn->clockQuality.offsetScaledLogVariance =
+ ntohs(gsn->clockQuality.offsetScaledLogVariance);
+ gsn->utc_offset = ntohs(gsn->utc_offset);
+ break;
}
if (extra_len) {
if (extra_len % 2)
@@ -183,6 +192,7 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
struct timePropertiesDS *tp;
struct portDS *p;
struct time_status_np *tsn;
+ struct grandmaster_settings_np *gsn;
struct mgmt_clock_description *cd;
switch (m->id) {
case CLOCK_DESCRIPTION:
@@ -236,6 +246,12 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
scaled_ns_h2n(&tsn->lastGmPhaseChange);
tsn->gmPresent = htonl(tsn->gmPresent);
break;
+ case GRANDMASTER_SETTINGS_NP:
+ gsn = (struct grandmaster_settings_np *) m->data;
+ gsn->clockQuality.offsetScaledLogVariance =
+ htons(gsn->clockQuality.offsetScaledLogVariance);
+ gsn->utc_offset = htons(gsn->utc_offset);
+ break;
}
}

diff --git a/tlv.h b/tlv.h
index 443bbdf..1cd8a39 100644
--- a/tlv.h
+++ b/tlv.h
@@ -78,6 +78,7 @@ enum management_action {
#define TRANSPARENT_CLOCK_DEFAULT_DATA_SET 0x4000
#define PRIMARY_DOMAIN 0x4002
#define TIME_STATUS_NP 0xC000
+#define GRANDMASTER_SETTINGS_NP 0xC001

/* Port management ID values */
#define NULL_MANAGEMENT 0x0000
@@ -182,6 +183,13 @@ struct time_status_np {
struct ClockIdentity gmIdentity;
} PACKED;

+struct grandmaster_settings_np {
+ struct ClockQuality clockQuality;
+ Integer16 utc_offset;
+ UInteger8 time_flags;
+ Enumeration8 time_source;
+} PACKED;
+
enum clock_type {
CLOCK_TYPE_ORDINARY = 0x8000,
CLOCK_TYPE_BOUNDARY = 0x4000,
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:03 UTC
Permalink
This patch also replaces the hard coded logic for the UTC offset and the
time property flags with clock variables. This new clock state will be
used for adjusting the grand master attributes at run time.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/clock.c b/clock.c
index ea2d97e..ee265ac 100644
--- a/clock.c
+++ b/clock.c
@@ -79,6 +79,8 @@ struct clock {
int utc_timescale;
int leap_set;
int kernel_leap;
+ int utc_offset; /* grand master role */
+ int time_flags; /* grand master role */
int time_source; /* grand master role */
enum servo_state servo_state;
tmv_t master_offset;
@@ -172,6 +174,7 @@ static int clock_management_get_response(struct clock *c, struct port *p,
struct management_tlv_datum *mtd;
struct ptp_message *rsp;
struct time_status_np *tsn;
+ struct grandmaster_settings_np *gsn;
struct PortIdentity pid = port_identity(p);
struct PTPText *text;

@@ -271,6 +274,15 @@ static int clock_management_get_response(struct clock *c, struct port *p,
datalen = sizeof(*tsn);
respond = 1;
break;
+ case GRANDMASTER_SETTINGS_NP:
+ gsn = (struct grandmaster_settings_np *) tlv->data;
+ gsn->clockQuality = c->dds.clockQuality;
+ gsn->utc_offset = c->utc_offset;
+ gsn->time_flags = c->time_flags;
+ gsn->time_source = c->time_source;
+ datalen = sizeof(*gsn);
+ respond = 1;
+ break;
}
if (respond) {
if (datalen % 2) {
@@ -431,12 +443,8 @@ static void clock_update_grandmaster(struct clock *c)
pds->grandmasterPriority1 = c->dds.priority1;
pds->grandmasterPriority2 = c->dds.priority2;
c->dad.path_length = 0;
- c->tds.currentUtcOffset = CURRENT_UTC_OFFSET;
- if (c->utc_timescale) {
- c->tds.flags = 0;
- } else {
- c->tds.flags = PTP_TIMESCALE;
- }
+ c->tds.currentUtcOffset = c->utc_offset;
+ c->tds.flags = c->time_flags;
c->tds.timeSource = c->time_source;
}

@@ -571,6 +579,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
c->free_running = dds->free_running;
c->freq_est_interval = dds->freq_est_interval;
c->kernel_leap = dds->kernel_leap;
+ c->utc_offset = CURRENT_UTC_OFFSET;
c->time_source = dds->time_source;
c->desc = dds->clock_desc;

@@ -598,6 +607,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
sysclk_set_leap(0);
}
c->leap_set = 0;
+ c->time_flags = c->utc_timescale ? 0 : PTP_TIMESCALE;

if (c->clkid != CLOCK_INVALID) {
fadj = (int) clockadj_get_freq(c->clkid);
@@ -846,6 +856,7 @@ void clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
case TRANSPARENT_CLOCK_DEFAULT_DATA_SET:
case PRIMARY_DOMAIN:
case TIME_STATUS_NP:
+ case GRANDMASTER_SETTINGS_NP:
clock_management_send_error(p, msg, NOT_SUPPORTED);
break;
default:
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:05 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/clock.c b/clock.c
index ee265ac..c2795a1 100644
--- a/clock.c
+++ b/clock.c
@@ -308,7 +308,24 @@ static int clock_management_set(struct clock *c, struct port *p,
int id, struct ptp_message *req)
{
int respond = 0;
+ struct management_tlv *tlv;
+ struct grandmaster_settings_np *gsn;
+
+ tlv = (struct management_tlv *) req->management.suffix;
+
switch (id) {
+ case GRANDMASTER_SETTINGS_NP:
+ if (p != c->port[c->nports]) {
+ /* Sorry, only allowed on the UDS port. */
+ break;
+ }
+ gsn = (struct grandmaster_settings_np *) tlv->data;
+ c->dds.clockQuality = gsn->clockQuality;
+ c->utc_offset = gsn->utc_offset;
+ c->time_flags = gsn->time_flags;
+ c->time_source = gsn->time_source;
+ respond = 1;
+ break;
}
if (respond && !clock_management_get_response(c, p, id, req))
pr_err("failed to send management set response");
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:04 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/pmc.c b/pmc.c
index 66b57fe..f428ff9 100644
--- a/pmc.c
+++ b/pmc.c
@@ -85,6 +85,7 @@ struct management_id idtab[] = {
{ "TRANSPARENT_CLOCK_DEFAULT_DATA_SET", TRANSPARENT_CLOCK_DEFAULT_DATA_SET, not_supported },
{ "PRIMARY_DOMAIN", PRIMARY_DOMAIN, not_supported },
{ "TIME_STATUS_NP", TIME_STATUS_NP, do_get_action },
+ { "GRANDMASTER_SETTINGS_NP", GRANDMASTER_SETTINGS_NP, do_get_action },
/* Port management ID values */
{ "NULL_MANAGEMENT", NULL_MANAGEMENT, null_management },
{ "CLOCK_DESCRIPTION", CLOCK_DESCRIPTION, do_get_action },
@@ -188,6 +189,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
struct parentDS *pds;
struct timePropertiesDS *tp;
struct time_status_np *tsn;
+ struct grandmaster_settings_np *gsn;
struct mgmt_clock_description *cd;
struct portDS *p;
if (msg_type(msg) != MANAGEMENT) {
@@ -384,6 +386,32 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
tsn->gmPresent ? "true" : "false",
cid2str(&tsn->gmIdentity));
break;
+ case GRANDMASTER_SETTINGS_NP:
+ gsn = (struct grandmaster_settings_np *) mgt->data;
+ fprintf(fp, "GRANDMASTER_SETTINGS_NP "
+ IFMT "clockClass %hhu"
+ IFMT "clockAccuracy 0x%02hhx"
+ IFMT "offsetScaledLogVariance 0x%04hx"
+ IFMT "currentUtcOffset %hd"
+ IFMT "leap61 %d"
+ IFMT "leap59 %d"
+ IFMT "currentUtcOffsetValid %d"
+ IFMT "ptpTimescale %d"
+ IFMT "timeTraceable %d"
+ IFMT "frequencyTraceable %d"
+ IFMT "timeSource 0x%02hhx",
+ gsn->clockQuality.clockClass,
+ gsn->clockQuality.clockAccuracy,
+ gsn->clockQuality.offsetScaledLogVariance,
+ gsn->utc_offset,
+ gsn->time_flags & LEAP_61 ? 1 : 0,
+ gsn->time_flags & LEAP_59 ? 1 : 0,
+ gsn->time_flags & UTC_OFF_VALID ? 1 : 0,
+ gsn->time_flags & PTP_TIMESCALE ? 1 : 0,
+ gsn->time_flags & TIME_TRACEABLE ? 1 : 0,
+ gsn->time_flags & FREQ_TRACEABLE ? 1 : 0,
+ gsn->time_source);
+ break;
case PORT_DATA_SET:
p = (struct portDS *) mgt->data;
if (p->portState > PS_SLAVE) {
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:06 UTC
Permalink
Management messages can cause a change in the clock quality. If this
happens, then it is time to run the Best Master Clock algorithm again.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 26 +++++++++++++++-----------
clock.h | 4 +++-
port.c | 3 ++-
3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/clock.c b/clock.c
index c2795a1..f59e753 100644
--- a/clock.c
+++ b/clock.c
@@ -305,7 +305,7 @@ out:
}

static int clock_management_set(struct clock *c, struct port *p,
- int id, struct ptp_message *req)
+ int id, struct ptp_message *req, int *changed)
{
int respond = 0;
struct management_tlv *tlv;
@@ -324,6 +324,7 @@ static int clock_management_set(struct clock *c, struct port *p,
c->utc_offset = gsn->utc_offset;
c->time_flags = gsn->time_flags;
c->time_source = gsn->time_source;
+ *changed = 1;
respond = 1;
break;
}
@@ -791,9 +792,9 @@ static void clock_forward_mgmt_msg(struct clock *c, struct port *p, struct ptp_m
}
}

-void clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
+int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
{
- int i;
+ int changed = 0, i;
struct management_tlv *mgt;
struct ClockIdentity *tcid, wildcard = {
{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}
@@ -805,10 +806,10 @@ void clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
/* Apply this message to the local clock and ports. */
tcid = &msg->management.targetPortIdentity.clockIdentity;
if (!cid_eq(tcid, &wildcard) && !cid_eq(tcid, &c->dds.clockIdentity)) {
- return;
+ return changed;
}
if (msg->tlv_count != 1) {
- return;
+ return changed;
}
mgt = (struct management_tlv *) msg->management.suffix;

@@ -821,24 +822,24 @@ void clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
switch (management_action(msg)) {
case GET:
if (clock_management_get_response(c, p, mgt->id, msg))
- return;
+ return changed;
break;
case SET:
if (mgt->length == 2 && mgt->id != NULL_MANAGEMENT) {
clock_management_send_error(p, msg, WRONG_LENGTH);
- return;
+ return changed;
}
- if (clock_management_set(c, p, mgt->id, msg))
- return;
+ if (clock_management_set(c, p, mgt->id, msg, &changed))
+ return changed;
break;
case COMMAND:
if (mgt->length != 2) {
clock_management_send_error(p, msg, WRONG_LENGTH);
- return;
+ return changed;
}
break;
default:
- return;
+ return changed;
}

switch (mgt->id) {
@@ -883,6 +884,7 @@ void clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
}
break;
}
+ return changed;
}

struct parent_ds *clock_parent_ds(struct clock *c)
@@ -945,6 +947,8 @@ int clock_poll(struct clock *c)
k = N_CLOCK_PFD * i + j;
if (c->pollfd[k].revents & (POLLIN|POLLPRI)) {
event = port_event(c->port[i], j);
+ if (EV_STATE_DECISION_EVENT == event)
+ sde = 1;
}
}

diff --git a/clock.h b/clock.h
index 6fafc08..309f731 100644
--- a/clock.h
+++ b/clock.h
@@ -120,8 +120,10 @@ void clock_install_fda(struct clock *c, struct port *p, struct fdarray fda);
* @param c The clock instance.
* @param p The port on which the message arrived.
* @param msg A management message.
+ * @return One if the management action caused a change that
+ * implies a state decision event, zero otherwise.
*/
-void clock_manage(struct clock *c, struct port *p, struct ptp_message *msg);
+int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg);

/**
* Obtain a clock's parent data set.
diff --git a/port.c b/port.c
index c4505d8..dda5cde 100644
--- a/port.c
+++ b/port.c
@@ -1980,7 +1980,8 @@ enum fsm_event port_event(struct port *p, int fd_index)
case SIGNALING:
break;
case MANAGEMENT:
- clock_manage(p->clock, p, msg);
+ if (clock_manage(p->clock, p, msg))
+ event = EV_STATE_DECISION_EVENT;
break;
}
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:07 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc_common.c | 23 +++++++++++++++++++++++
pmc_common.h | 2 ++
2 files changed, 25 insertions(+)

diff --git a/pmc_common.c b/pmc_common.c
index 6dbeaf7..26666f8 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -161,6 +161,29 @@ int pmc_send_get_action(struct pmc *pmc, int id)
return 0;
}

+int pmc_send_set_action(struct pmc *pmc, int id, void *data, int datasize)
+{
+ int pdulen;
+ struct ptp_message *msg;
+ struct management_tlv *mgt;
+ msg = pmc_message(pmc, SET);
+ if (!msg) {
+ return -1;
+ }
+ mgt = (struct management_tlv *) msg->management.suffix;
+ mgt->type = TLV_MANAGEMENT;
+ mgt->length = 2 + datasize;
+ mgt->id = id;
+ memcpy(mgt->data, data, datasize);
+ pdulen = msg->header.messageLength + sizeof(*mgt) + datasize;
+ msg->header.messageLength = pdulen;
+ msg->tlv_count = 1;
+ pmc_send(pmc, msg, pdulen);
+ msg_put(msg);
+
+ return 0;
+}
+
struct ptp_message *pmc_recv(struct pmc *pmc)
{
struct ptp_message *msg;
diff --git a/pmc_common.h b/pmc_common.h
index f16eb9a..46e080e 100644
--- a/pmc_common.h
+++ b/pmc_common.h
@@ -35,6 +35,8 @@ int pmc_get_transport_fd(struct pmc *pmc);

int pmc_send_get_action(struct pmc *pmc, int id);

+int pmc_send_set_action(struct pmc *pmc, int id, void *data, int datasize);
+
struct ptp_message *pmc_recv(struct pmc *pmc);

#endif
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:08 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 79 insertions(+), 9 deletions(-)

diff --git a/pmc.c b/pmc.c
index f428ff9..05544a1 100644
--- a/pmc.c
+++ b/pmc.c
@@ -42,14 +42,15 @@

static struct pmc *pmc;

-static void do_get_action(int action, int index);
-static void not_supported(int action, int index);
-static void null_management(int action, int index);
+static void do_get_action(int action, int index, char *str);
+static void do_set_action(int action, int index, char *str);
+static void not_supported(int action, int index, char *str);
+static void null_management(int action, int index, char *str);

struct management_id {
char name[64];
int code;
- void (*func)(int action, int index);
+ void (*func)(int action, int index, char *str);
};

struct management_id idtab[] = {
@@ -85,7 +86,7 @@ struct management_id idtab[] = {
{ "TRANSPARENT_CLOCK_DEFAULT_DATA_SET", TRANSPARENT_CLOCK_DEFAULT_DATA_SET, not_supported },
{ "PRIMARY_DOMAIN", PRIMARY_DOMAIN, not_supported },
{ "TIME_STATUS_NP", TIME_STATUS_NP, do_get_action },
- { "GRANDMASTER_SETTINGS_NP", GRANDMASTER_SETTINGS_NP, do_get_action },
+ { "GRANDMASTER_SETTINGS_NP", GRANDMASTER_SETTINGS_NP, do_set_action },
/* Port management ID values */
{ "NULL_MANAGEMENT", NULL_MANAGEMENT, null_management },
{ "CLOCK_DESCRIPTION", CLOCK_DESCRIPTION, do_get_action },
@@ -470,7 +471,7 @@ out:
fflush(fp);
}

-static void do_get_action(int action, int index)
+static void do_get_action(int action, int index, char *str)
{
if (action == GET)
pmc_send_get_action(pmc, idtab[index].code);
@@ -478,12 +479,81 @@ static void do_get_action(int action, int index)
fprintf(stderr, "%s only allows GET\n", idtab[index].name);
}

-static void not_supported(int action, int index)
+static void do_set_action(int action, int index, char *str)
+{
+ struct grandmaster_settings_np gsn;
+ int cnt, code = idtab[index].code;
+ int leap_61, leap_59, utc_off_valid;
+ int ptp_timescale, time_traceable, freq_traceable;
+
+ switch (action) {
+ case GET:
+ pmc_send_get_action(pmc, code);
+ return;
+ case SET:
+ break;
+ case RESPONSE:
+ case COMMAND:
+ case ACKNOWLEDGE:
+ default:
+ fprintf(stderr, "%s only allows GET or SET\n",
+ idtab[index].name);
+ return;
+ }
+ switch (code) {
+ case GRANDMASTER_SETTINGS_NP:
+ cnt = sscanf(str, " %*s %*s "
+ "clockClass %hhu "
+ "clockAccuracy %hhx "
+ "offsetScaledLogVariance %hx "
+ "currentUtcOffset %hd "
+ "leap61 %d "
+ "leap59 %d "
+ "currentUtcOffsetValid %d "
+ "ptpTimescale %d "
+ "timeTraceable %d "
+ "frequencyTraceable %d "
+ "timeSource %hhx ",
+ &gsn.clockQuality.clockClass,
+ &gsn.clockQuality.clockAccuracy,
+ &gsn.clockQuality.offsetScaledLogVariance,
+ &gsn.utc_offset,
+ &leap_61,
+ &leap_59,
+ &utc_off_valid,
+ &ptp_timescale,
+ &time_traceable,
+ &freq_traceable,
+ &gsn.time_source);
+ if (cnt != 11) {
+ fprintf(stderr, "%s SET needs 11 values\n",
+ idtab[index].name);
+ break;
+ }
+ gsn.time_flags = 0;
+ if (leap_61)
+ gsn.time_flags |= LEAP_61;
+ if (leap_59)
+ gsn.time_flags |= LEAP_59;
+ if (utc_off_valid)
+ gsn.time_flags |= UTC_OFF_VALID;
+ if (ptp_timescale)
+ gsn.time_flags |= PTP_TIMESCALE;
+ if (time_traceable)
+ gsn.time_flags |= TIME_TRACEABLE;
+ if (freq_traceable)
+ gsn.time_flags |= FREQ_TRACEABLE;
+ pmc_send_set_action(pmc, code, &gsn, sizeof(gsn));
+ break;
+ }
+}
+
+static void not_supported(int action, int index, char *str)
{
fprintf(stdout, "sorry, %s not supported yet\n", idtab[index].name);
}

-static void null_management(int action, int index)
+static void null_management(int action, int index, char *str)
{
if (action == GET)
pmc_send_get_action(pmc, idtab[index].code);
@@ -567,7 +637,7 @@ static int do_command(char *str)
fprintf(stdout, "sending: %s %s\n",
action_string[action], idtab[id].name);

- idtab[id].func(action, id);
+ idtab[id].func(action, id, str);

return 0;
}
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:09 UTC
Permalink
This patch adds a new servo method to let the algorithm know about the
master clock's reported sync message interval. This information can be
used by the servo to adapt its synchronization parameters.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 2 ++
pi.c | 5 +++++
servo.c | 5 +++++
servo.h | 7 +++++++
servo_private.h | 2 ++
5 files changed, 21 insertions(+)

diff --git a/clock.c b/clock.c
index f59e753..b097911 100644
--- a/clock.c
+++ b/clock.c
@@ -1127,6 +1127,8 @@ void clock_sync_interval(struct clock *c, int n)
pr_warning("summary_interval is too long");
}
c->stats.max_count = (1 << shift);
+
+ servo_sync_interval(c->servo, n < 0 ? 1.0 / (1 << -n) : 1 << n);
}

struct timePropertiesDS *clock_time_properties(struct clock *c)
diff --git a/pi.c b/pi.c
index 89080c4..d42bf0b 100644
--- a/pi.c
+++ b/pi.c
@@ -132,6 +132,10 @@ static double pi_sample(struct servo *servo,
return ppb;
}

+static void pi_sync_interval(struct servo *servo, double interval)
+{
+}
+
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
{
struct pi_servo *s;
@@ -142,6 +146,7 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)

s->servo.destroy = pi_destroy;
s->servo.sample = pi_sample;
+ s->servo.sync_interval = pi_sync_interval;
s->drift = fadj;
s->maxppb = max_ppb;
s->first_update = 1;
diff --git a/servo.c b/servo.c
index a312ad1..45c5832 100644
--- a/servo.c
+++ b/servo.c
@@ -41,3 +41,8 @@ double servo_sample(struct servo *servo,
{
return servo->sample(servo, offset, local_ts, state);
}
+
+void servo_sync_interval(struct servo *servo, double interval)
+{
+ servo->sync_interval(servo, interval);
+}
diff --git a/servo.h b/servo.h
index 4d4a8e7..052740e 100644
--- a/servo.h
+++ b/servo.h
@@ -86,4 +86,11 @@ double servo_sample(struct servo *servo,
uint64_t local_ts,
enum servo_state *state);

+/**
+ * Inform a clock servo about the master's sync interval.
+ * @param servo Pointer to a servo obtained via @ref servo_create().
+ * @param interval The sync interval in seconds.
+ */
+void servo_sync_interval(struct servo *servo, double interval);
+
#endif
diff --git a/servo_private.h b/servo_private.h
index f9c151f..98c7db2 100644
--- a/servo_private.h
+++ b/servo_private.h
@@ -28,6 +28,8 @@ struct servo {
double (*sample)(struct servo *servo,
int64_t offset, uint64_t local_ts,
enum servo_state *state);
+
+ void (*sync_interval)(struct servo *servo, double interval);
};

#endif
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:10 UTC
Permalink
A slow servo (with smaller constants and lower sync rate) needs a
longer, better frequency estimation, but a higher sync rate hardly
needs any estimation at all, since it learns the frequency right away
in any case.

Signed-off-by: Richard Cochran <***@gmail.com>
---
pi.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/pi.c b/pi.c
index d42bf0b..5f4930e 100644
--- a/pi.c
+++ b/pi.c
@@ -30,6 +30,7 @@
#define SWTS_KI 0.001

#define NSEC_PER_SEC 1000000000
+#define FREQ_EST_MARGIN 0.001

/* These take their values from the configuration file. (see ptp4l.c) */
double configured_pi_kp = 0.0;
@@ -64,6 +65,7 @@ static double pi_sample(struct servo *servo,
enum servo_state *state)
{
double ki_term, ppb = 0.0;
+ double freq_est_interval, localdiff;
struct pi_servo *s = container_of(servo, struct pi_servo, servo);

switch (s->count) {
@@ -84,6 +86,18 @@ static double pi_sample(struct servo *servo,
break;
}

+ /* Wait long enough before estimating the frequency offset. */
+ localdiff = (s->local[1] - s->local[0]) / 1e9;
+ localdiff += localdiff * FREQ_EST_MARGIN;
+ freq_est_interval = 0.016 / s->ki;
+ if (freq_est_interval > 1000.0) {
+ freq_est_interval = 1000.0;
+ }
+ if (localdiff < freq_est_interval) {
+ *state = SERVO_UNLOCKED;
+ break;
+ }
+
s->drift += (s->offset[1] - s->offset[0]) * 1e9 /
(s->local[1] - s->local[0]);
if (s->drift < -s->maxppb)
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:11 UTC
Permalink
From: Miroslav Lichvar <***@redhat.com>

[RC: merged servo_sync_interval signature change with earlier patch]

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

diff --git a/phc2sys.c b/phc2sys.c
index 8f9e18b..366587f 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -782,8 +782,12 @@ int main(int argc, char *argv[])

dst_clock.servo = servo_create(CLOCK_SERVO_PI, -ppb, max_ppb, 0);

- if (pps_fd >= 0)
+ if (pps_fd >= 0) {
+ servo_sync_interval(dst_clock.servo, 1.0);
return do_pps_loop(&dst_clock, pps_fd, src, phc_readings);
+ }
+
+ servo_sync_interval(dst_clock.servo, phc_interval);

phc_interval_tp.tv_sec = phc_interval;
phc_interval_tp.tv_nsec = (phc_interval - phc_interval_tp.tv_sec) * 1e9;
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:12 UTC
Permalink
From: Miroslav Lichvar <***@redhat.com>

Instead of using fixed constants, set them by the following formula from
the current sync to allow good performance of the servo even when the
sync interval changes in runtime and to avoid instability.

kp = min(kp_scale * sync^kp_exponent, kp_norm_max / sync)
ki = min(ki_scale * sync^ki_exponent, ki_norm_max / sync)

The scale, exponent and norm_max constants are configurable. The
defaults are chosen so there is no change to the previous default
constants of the servo with one second sync interval. The automatic
adjustment can be disabled by setting the pi_proportional_const and
pi_integral_const options to a non-zero value, but stability of the
servo is always enforced.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
config.c | 36 ++++++++++++++++++++++++++++++++++++
config.h | 6 ++++++
default.cfg | 6 ++++++
gPTP.cfg | 6 ++++++
pi.c | 56 ++++++++++++++++++++++++++++++++++++++++++++------------
pi.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
ptp4l.8 | 46 ++++++++++++++++++++++++++++++++++++++++++----
ptp4l.c | 6 ++++++
8 files changed, 190 insertions(+), 16 deletions(-)

diff --git a/config.c b/config.c
index 854a703..d914afa 100644
--- a/config.c
+++ b/config.c
@@ -307,6 +307,42 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->pi_integral_const = df;

+ } else if (!strcmp(option, "pi_proportional_scale")) {
+ r = get_ranged_double(value, &df, 0.0, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ *cfg->pi_proportional_scale = df;
+
+ } else if (!strcmp(option, "pi_proportional_exponent")) {
+ r = get_ranged_double(value, &df, -DBL_MAX, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ *cfg->pi_proportional_exponent = df;
+
+ } else if (!strcmp(option, "pi_proportional_norm_max")) {
+ r = get_ranged_double(value, &df, DBL_MIN, 1.0);
+ if (r != PARSED_OK)
+ return r;
+ *cfg->pi_proportional_norm_max = df;
+
+ } else if (!strcmp(option, "pi_integral_scale")) {
+ r = get_ranged_double(value, &df, 0.0, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ *cfg->pi_integral_scale = df;
+
+ } else if (!strcmp(option, "pi_integral_exponent")) {
+ r = get_ranged_double(value, &df, -DBL_MAX, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ *cfg->pi_integral_exponent = df;
+
+ } else if (!strcmp(option, "pi_integral_norm_max")) {
+ r = get_ranged_double(value, &df, DBL_MIN, 2.0);
+ if (r != PARSED_OK)
+ return r;
+ *cfg->pi_integral_norm_max = df;
+
} else if (!strcmp(option, "pi_offset_const")) {
r = get_ranged_double(value, &df, 0.0, DBL_MAX);
if (r != PARSED_OK)
diff --git a/config.h b/config.h
index c5de3ff..b014d7e 100644
--- a/config.h
+++ b/config.h
@@ -75,6 +75,12 @@ struct config {

double *pi_proportional_const;
double *pi_integral_const;
+ double *pi_proportional_scale;
+ double *pi_proportional_exponent;
+ double *pi_proportional_norm_max;
+ double *pi_integral_scale;
+ double *pi_integral_exponent;
+ double *pi_integral_norm_max;
double *pi_offset_const;
double *pi_f_offset_const;
int *pi_max_frequency;
diff --git a/default.cfg b/default.cfg
index e10d257..c5493d6 100644
--- a/default.cfg
+++ b/default.cfg
@@ -40,6 +40,12 @@ kernel_leap 1
#
pi_proportional_const 0.0
pi_integral_const 0.0
+pi_proportional_scale 0.0
+pi_proportional_exponent -0.3
+pi_proportional_norm_max 0.7
+pi_integral_scale 0.0
+pi_integral_exponent 0.4
+pi_integral_norm_max 0.3
pi_offset_const 0.0
pi_f_offset_const 0.0000001
pi_max_frequency 900000000
diff --git a/gPTP.cfg b/gPTP.cfg
index c4e0fab..00215b4 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -39,6 +39,12 @@ kernel_leap 1
#
pi_proportional_const 0.0
pi_integral_const 0.0
+pi_proportional_scale 0.0
+pi_proportional_exponent -0.3
+pi_proportional_norm_max 0.7
+pi_integral_scale 0.0
+pi_integral_exponent 0.4
+pi_integral_norm_max 0.3
pi_offset_const 0.0
pi_f_offset_const 0.0000001
pi_max_frequency 900000000
diff --git a/pi.c b/pi.c
index 5f4930e..3c8a635 100644
--- a/pi.c
+++ b/pi.c
@@ -21,13 +21,16 @@
#include <math.h>

#include "pi.h"
+#include "print.h"
#include "servo_private.h"

-#define HWTS_KP 0.7
-#define HWTS_KI 0.3
+#define HWTS_KP_SCALE 0.7
+#define HWTS_KI_SCALE 0.3
+#define SWTS_KP_SCALE 0.1
+#define SWTS_KI_SCALE 0.001

-#define SWTS_KP 0.1
-#define SWTS_KI 0.001
+#define MAX_KP_NORM_MAX 1.0
+#define MAX_KI_NORM_MAX 2.0

#define NSEC_PER_SEC 1000000000
#define FREQ_EST_MARGIN 0.001
@@ -35,6 +38,12 @@
/* These take their values from the configuration file. (see ptp4l.c) */
double configured_pi_kp = 0.0;
double configured_pi_ki = 0.0;
+double configured_pi_kp_scale = 0.0;
+double configured_pi_kp_exponent = -0.3;
+double configured_pi_kp_norm_max = 0.7;
+double configured_pi_ki_scale = 0.0;
+double configured_pi_ki_exponent = 0.4;
+double configured_pi_ki_norm_max = 0.3;
double configured_pi_offset = 0.0;
double configured_pi_f_offset = 0.0000001; /* 100 nanoseconds */
int configured_pi_max_freq = 900000000;
@@ -148,6 +157,18 @@ static double pi_sample(struct servo *servo,

static void pi_sync_interval(struct servo *servo, double interval)
{
+ struct pi_servo *s = container_of(servo, struct pi_servo, servo);
+
+ s->kp = configured_pi_kp_scale * pow(interval, configured_pi_kp_exponent);
+ if (s->kp > configured_pi_kp_norm_max / interval)
+ s->kp = configured_pi_kp_norm_max / interval;
+
+ s->ki = configured_pi_ki_scale * pow(interval, configured_pi_ki_exponent);
+ if (s->ki > configured_pi_ki_norm_max / interval)
+ s->ki = configured_pi_ki_norm_max / interval;
+
+ pr_debug("PI servo: sync interval %.3f kp %.3f ki %.6f",
+ interval, s->kp, s->ki);
}

struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
@@ -164,16 +185,27 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->drift = fadj;
s->maxppb = max_ppb;
s->first_update = 1;
+ s->kp = 0.0;
+ s->ki = 0.0;

if (configured_pi_kp && configured_pi_ki) {
- s->kp = configured_pi_kp;
- s->ki = configured_pi_ki;
- } else if (sw_ts) {
- s->kp = SWTS_KP;
- s->ki = SWTS_KI;
- } else {
- s->kp = HWTS_KP;
- s->ki = HWTS_KI;
+ /* Use the constants as configured by the user without
+ adjusting for sync interval unless they make the servo
+ unstable. */
+ configured_pi_kp_scale = configured_pi_kp;
+ configured_pi_ki_scale = configured_pi_ki;
+ configured_pi_kp_exponent = 0.0;
+ configured_pi_ki_exponent = 0.0;
+ configured_pi_kp_norm_max = MAX_KP_NORM_MAX;
+ configured_pi_ki_norm_max = MAX_KI_NORM_MAX;
+ } else if (!configured_pi_kp_scale || !configured_pi_ki_scale) {
+ if (sw_ts) {
+ configured_pi_kp_scale = SWTS_KP_SCALE;
+ configured_pi_ki_scale = SWTS_KI_SCALE;
+ } else {
+ configured_pi_kp_scale = HWTS_KP_SCALE;
+ configured_pi_ki_scale = HWTS_KI_SCALE;
+ }
}

if (configured_pi_offset > 0.0) {
diff --git a/pi.h b/pi.h
index 2f31bce..0d8994b 100644
--- a/pi.h
+++ b/pi.h
@@ -34,6 +34,50 @@ extern double configured_pi_kp;
extern double configured_pi_ki;

/**
+ * When set to a non-zero value, this variable determines the scale in the
+ * formula used to set the proportional constant of the PI controller from the
+ * sync interval.
+ * kp = min(kp_scale * sync^kp_exponent, kp_norm_max / sync)
+ */
+extern double configured_pi_kp_scale;
+
+/**
+ * This variable determines the exponent in the formula used to set the
+ * proportional constant of the PI controller from the sync interval.
+ * kp = min(kp_scale * sync^kp_exponent, kp_norm_max / sync)
+ */
+extern double configured_pi_kp_exponent;
+
+/**
+ * This variable determines the normalized maximum in the formula used to set
+ * the proportional constant of the PI controller from the sync interval.
+ * kp = min(kp_scale * sync^kp_exponent, kp_norm_max / sync)
+ */
+extern double configured_pi_kp_norm_max;
+
+/**
+ * When set to a non-zero value, this variable determines the scale in the
+ * formula used to set the integral constant of the PI controller from the
+ * sync interval.
+ * ki = min(ki_scale * sync^ki_exponent, ki_norm_max / sync)
+ */
+extern double configured_pi_ki_scale;
+
+/**
+ * This variable determines the exponent in the formula used to set the
+ * integral constant of the PI controller from the sync interval.
+ * ki = min(ki_scale * sync^ki_exponent, ki_norm_max / sync)
+ */
+extern double configured_pi_ki_exponent;
+
+/**
+ * This variable determines the normalized maximum in the formula used to set
+ * the integral constant of the PI controller from the sync interval.
+ * ki = min(ki_scale * sync^ki_exponent, ki_norm_max / sync)
+ */
+extern double configured_pi_ki_norm_max;
+
+/**
* 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.
diff --git a/ptp4l.8 b/ptp4l.8
index 5a9e8b4..6f42c1c 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -254,17 +254,55 @@ servo is implemented, a PI controller.
The default is pi.
.TP
.B pi_proportional_const
-The proportional constant of the PI controller. When set to 0.0, the value will
-be selected from 0.7 and 0.1 for the hardware and software time stamping
-respectively.
+The proportional constant of the PI controller. When set to 0.0, the
+proportional constant will be set by the following formula from the current
+sync interval.
The default is 0.0.
+
+kp = min(kp_scale * sync^kp_exponent, kp_norm_max / sync))
.TP
.B pi_integral_const
-The integral constant of the PI controller. When set to 0.0, the value will be
+The integral constant of the PI controller. When set to 0.0, the
+integral constant will be set by the following formula from the current
+sync interval.
+The default is 0.0.
+
+ki = min(ki_scale * sync^ki_exponent, ki_norm_max / sync)
+.TP
+.B pi_proportional_scale
+The kp_scale constant in the formula used to set the proportional constant of
+the PI controller from the sync interval. When set to 0.0, the value will be
+selected from 0.7 and 0.1 for the hardware and software time stamping
+respectively.
+The default is 0.0.
+.TP
+.B pi_proportional_exponent
+The kp_exponent constant in the formula used to set the proportional constant of
+the PI controller from the sync interval.
+The default is -0.3.
+.TP
+.B pi_proportional_norm_max
+The kp_norm_max constant in the formula used to set the proportional constant of
+the PI controller from the sync interval.
+The default is 0.7
+.TP
+.B pi_integral_scale
+The ki_scale constant in the formula used to set the integral constant of
+the PI controller from the sync interval. When set to 0.0, the value will be
selected from 0.3 and 0.001 for the hardware and software time stamping
respectively.
The default is 0.0.
.TP
+.B pi_integral_exponent
+The ki_exponent constant in the formula used to set the integral constant of
+the PI controller from the sync interval.
+The default is 0.4.
+.TP
+.B pi_integral_norm_max
+The ki_norm_max constant in the formula used to set the integral constant of
+the PI controller from the sync interval.
+The default is 0.3.
+.TP
.B pi_offset_const
The maximum offset the PI controller will correct by changing the clock
frequency instead of stepping the clock. When set to 0.0, the controller will
diff --git a/ptp4l.c b/ptp4l.c
index 632166c..1d4a3e0 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -96,6 +96,12 @@ static struct config cfg_settings = {

.pi_proportional_const = &configured_pi_kp,
.pi_integral_const = &configured_pi_ki,
+ .pi_proportional_scale = &configured_pi_kp_scale,
+ .pi_proportional_exponent = &configured_pi_kp_exponent,
+ .pi_proportional_norm_max = &configured_pi_kp_norm_max,
+ .pi_integral_scale = &configured_pi_ki_scale,
+ .pi_integral_exponent = &configured_pi_ki_exponent,
+ .pi_integral_norm_max = &configured_pi_ki_norm_max,
.pi_offset_const = &configured_pi_offset,
.pi_f_offset_const = &configured_pi_f_offset,
.pi_max_frequency = &configured_pi_max_freq,
--
1.7.10.4
Richard Cochran
2013-07-14 20:02:13 UTC
Permalink
This patch cleans up the BMC logic to allow assuming the GM role when no
other clocks are found in the network. By allowing the "best" to be NULL,
we can let the BMC to naturally pick the local clock as GM. As an added
bonus, this also get rid of the hacky check for a lost master.

Signed-off-by: Richard Cochran <***@gmail.com>
---
bmc.c | 3 ---
clock.c | 31 ++++++++++++-------------------
2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/bmc.c b/bmc.c
index 9d6b1af..462a90d 100644
--- a/bmc.c
+++ b/bmc.c
@@ -133,9 +133,6 @@ enum port_state bmc_state_decision(struct clock *c, struct port *r)
if (!port_best && PS_LISTENING == ps)
return ps;

- if (!clock_best)
- return PS_FAULTY;
-
if (clock_class(c) <= 127) {
if (dscmp(clock_ds, port_best) > 0) {
return PS_GRAND_MASTER; /*M1*/
diff --git a/clock.c b/clock.c
index b097911..8dd6662 100644
--- a/clock.c
+++ b/clock.c
@@ -333,16 +333,6 @@ static int clock_management_set(struct clock *c, struct port *p,
return respond ? 1 : 0;
}

-static int clock_master_lost(struct clock *c)
-{
- int i;
- for (i = 0; i < c->nports; i++) {
- if (PS_SLAVE == port_state(c->port[i]))
- return 0;
- }
- return 1;
-}
-
static void clock_stats_update(struct clock_stats *s,
int64_t offset, double freq)
{
@@ -899,7 +889,7 @@ struct PortIdentity clock_parent_identity(struct clock *c)

int clock_poll(struct clock *c)
{
- int cnt, err, i, j, k, lost = 0, sde = 0;
+ int cnt, err, i, j, k, sde = 0;
enum fsm_event event;

cnt = poll(c->pollfd, ARRAY_SIZE(c->pollfd), -1);
@@ -924,7 +914,7 @@ int clock_poll(struct clock *c)
if (EV_STATE_DECISION_EVENT == event)
sde = 1;
if (EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES == event)
- lost = 1;
+ sde = 1;
err = port_dispatch(c->port[i], event, 0);
/* Clear any fault after a little while. */
if (PS_FAULTY == port_state(c->port[i])) {
@@ -952,8 +942,6 @@ int clock_poll(struct clock *c)
}
}

- if (lost && clock_master_lost(c))
- clock_update_grandmaster(c);
if (sde)
handle_state_decision_event(c);

@@ -1144,6 +1132,7 @@ void clock_update_time_properties(struct clock *c, struct timePropertiesDS tds)
static void handle_state_decision_event(struct clock *c)
{
struct foreign_clock *best = NULL, *fc;
+ struct ClockIdentity best_id;
int fresh_best = 0, i;

for (i = 0; i < c->nports; i++) {
@@ -1154,13 +1143,16 @@ static void handle_state_decision_event(struct clock *c)
best = fc;
}

- if (!best)
- return;
+ if (best) {
+ best_id = best->dataset.identity;
+ } else {
+ best_id = c->dds.clockIdentity;
+ }

pr_notice("selected best master clock %s",
- cid2str(&best->dataset.identity));
+ cid2str(&best_id));

- if (!cid_eq(&best->dataset.identity, &c->best_id)) {
+ if (!cid_eq(&best_id, &c->best_id)) {
clock_freq_est_reset(c);
mave_reset(c->avg_delay);
c->path_delay = 0;
@@ -1168,7 +1160,7 @@ static void handle_state_decision_event(struct clock *c)
}

c->best = best;
- c->best_id = best->dataset.identity;
+ c->best_id = best_id;

for (i = 0; i < c->nports; i++) {
enum port_state ps;
@@ -1179,6 +1171,7 @@ static void handle_state_decision_event(struct clock *c)
event = EV_NONE;
break;
case PS_GRAND_MASTER:
+ pr_notice("assuming the grand master role");
clock_update_grandmaster(c);
event = EV_RS_GRAND_MASTER;
break;
--
1.7.10.4
Miroslav Lichvar
2013-07-16 13:04:32 UTC
Permalink
Post by Richard Cochran
* Changes in V2
- for clarity, use switch/case in do_set_action in pmc.c
- omit the '=' in the SET command
- correct a commit message
- after changing GM settings, throw a BMC calculation
All look good to me.

I did a test with GM switching between two nodes and it seemed to work
fine.

Thanks,
--
Miroslav Lichvar
Loading...