Discussion:
[Linuxptp-devel] [PATCH RFC 00/18] Improved slave servo and grand master support
Richard Cochran
2013-07-07 19:42:57 UTC
Permalink
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.

Sorry to dump this out all at once, but please do review what you
can. After this gets some more testing, then I think it will be time
to release version 1.3.

Thanks,
Richard


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

Richard Cochran (16):
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: Add support for the delay mechanism 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.

clock.c | 73 ++++++++++++++++++++++++---------
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 | 123 +++++++++++++++++++++++++++++++++++++++++++++++++------
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 ++++
21 files changed, 474 insertions(+), 62 deletions(-)
--
1.7.10.4
Richard Cochran
2013-07-07 19:42:58 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
Keller, Jacob E
2013-07-08 21:27:06 UTC
Permalink
Looks good to me.. Note that the offending lines were added from two separate patches..

4e24248a71263ea9d5c144471dbaf8b412587adf ("Add options to not apply leap seconds in kernel.")
e21af9709192acd26b468c173e022de8b9242367 ("ptp4l: Handle leap seconds.")

I'm not 100% sure how the original bug ended up.. but thought Miro might have some insight?

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 01/18] No need to set kernel_leap
twice in a row.
This patch removes a redundant initialization of the kernel_leap clock
variable. The field is already set in clock_create a few lines earlier.
---
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
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-09 03:40:49 UTC
Permalink
Post by Keller, Jacob E
I'm not 100% sure how the original bug ended up.. but thought Miro might have some insight?
It was just an oversight, I guess. Doesn't matter now.

Thanks,
Richard
Miroslav Lichvar
2013-07-09 06:31:56 UTC
Permalink
Post by Richard Cochran
Post by Keller, Jacob E
I'm not 100% sure how the original bug ended up.. but thought Miro might have some insight?
It was just an oversight, I guess. Doesn't matter now.
I think it was. The first patch wasn't supposed to have the
kernel_leap assignment (the code doesn't even compile), sorry.
--
Miroslav Lichvar
Keller, Jacob E
2013-07-09 17:34:01 UTC
Permalink
-----Original Message-----
Sent: Monday, July 08, 2013 11:32 PM
To: Richard Cochran
Subject: Re: [Linuxptp-devel] [PATCH RFC 01/18] No need to set
kernel_leap twice in a row.
Post by Richard Cochran
Post by Keller, Jacob E
I'm not 100% sure how the original bug ended up.. but thought Miro
might have some insight?
Post by Richard Cochran
It was just an oversight, I guess. Doesn't matter now.
I think it was. The first patch wasn't supposed to have the
kernel_leap assignment (the code doesn't even compile), sorry.
--
Miroslav Lichvar
Yea. At least we got it fixed up now :)

- Jake
Richard Cochran
2013-07-07 19:42:59 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
Keller, Jacob E
2013-07-08 21:27:28 UTC
Permalink
ACK

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 02/18] Clean up indented white
space.
---
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;
- 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;
if (data_len != sizeof(struct defaultDS))
--
1.7.10.4
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-07 19:43:00 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
Keller, Jacob E
2013-07-08 21:28:07 UTC
Permalink
ACK

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 03/18] pmc: fix coding style by
using K&R style functions.
---
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)) {
--
1.7.10.4
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-07 19:43:01 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
Keller, Jacob E
2013-07-08 21:31:18 UTC
Permalink
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 04/18] Reset announce timer
when port is passive.
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.
---
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)
result = add_foreign_master(p, m);
break;
The patch description doesn't seem to cover why this change occurred... Care to elaborate what the difference will be?

Thanks

- Jake
result = update_current_master(p, m);
--
1.7.10.4
Richard Cochran
2013-07-09 04:01:08 UTC
Permalink
Post by Keller, Jacob E
Post by Richard Cochran
result = add_foreign_master(p, m);
break;
result = update_current_master(p, m);
The patch description doesn't seem to cover why this change
occurred... Care to elaborate what the difference will be?
There are two functions to process the foreign master information
contained in the announce messages, namely add_foreign_master and
update_current_master.

When the port is in SLAVE state (or UNCALIBRATED, about to become
slave) the information is used to update the master record, if
needed. The update_current_master calls add_foreign_master if the
announce message was from a different node than the current master.
Having changed information or new foreign masters will trigger a BMC
selection.

Now, in the case of PASSIVE state, the port has an excellent local
clock, and it really wants to be a grand master, but it must keep
quiet because there is a *better* clock in the network. So, it really
must act like in SLAVE state and monitor the GM's announce messages,
waiting for a chance to become GM.

Before this change, a PASSIVE port failed to reset the announce timer
and then started sending announce messages over and over again,
instead of keeping quiet like it should.

HTH,
Richard
Keller, Jacob E
2013-07-09 17:27:19 UTC
Permalink
-----Original Message-----
Sent: Monday, July 08, 2013 9:01 PM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] [PATCH RFC 04/18] Reset announce timer
when port is passive.
Post by Keller, Jacob E
Post by Richard Cochran
result = add_foreign_master(p, m);
break;
result = update_current_master(p, m);
The patch description doesn't seem to cover why this change
occurred... Care to elaborate what the difference will be?
There are two functions to process the foreign master information
contained in the announce messages, namely add_foreign_master and
update_current_master.
When the port is in SLAVE state (or UNCALIBRATED, about to become
slave) the information is used to update the master record, if
needed. The update_current_master calls add_foreign_master if the
announce message was from a different node than the current master.
Having changed information or new foreign masters will trigger a BMC
selection.
Now, in the case of PASSIVE state, the port has an excellent local
clock, and it really wants to be a grand master, but it must keep
quiet because there is a *better* clock in the network. So, it really
must act like in SLAVE state and monitor the GM's announce messages,
waiting for a chance to become GM.
Before this change, a PASSIVE port failed to reset the announce timer
and then started sending announce messages over and over again,
instead of keeping quiet like it should.
Ok got it, so update_current_master actually calls add_foreign_master. Thanks!

- Jake
HTH,
Richard
Richard Cochran
2013-07-07 19:43:02 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
Keller, Jacob E
2013-07-08 21:31:41 UTC
Permalink
ACK

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 05/18] Add missing option to the
default configuration file.
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.
---
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
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-07 19:43:03 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
Keller, Jacob E
2013-07-08 21:32:49 UTC
Permalink
Assuming a follow-on patch adds the config option?

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 06/18] Add a clock variable to
hold the value of the time source.
---
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
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-07 19:43:04 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
Keller, Jacob E
2013-07-08 21:34:15 UTC
Permalink
I'm curious why this was separate from the Patch 6 in the series.. Maybe because the patch series is really several patch series sent at once?

But I see it does add the config option, so you can ignore my previous comment..

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 07/18] Add a configuration option
for the time source.
---
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
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-09 04:06:04 UTC
Permalink
Post by Keller, Jacob E
I'm curious why this was separate from the Patch 6 in the series.. Maybe because the patch series is really several patch series sent at once?
It is just because I like to have small, incremental commits. I find
it easier to review, even for my own changes.

Thanks,
Richard
Keller, Jacob E
2013-07-09 17:27:37 UTC
Permalink
-----Original Message-----
Sent: Monday, July 08, 2013 9:06 PM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] [PATCH RFC 07/18] Add a configuration
option for the time source.
Post by Keller, Jacob E
I'm curious why this was separate from the Patch 6 in the series..
Maybe because the patch series is really several patch series sent at
once?
It is just because I like to have small, incremental commits. I find
it easier to review, even for my own changes.
Thanks,
Richard
Yea that makes sense.

- Jake
Richard Cochran
2013-07-07 19:43:05 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
Keller, Jacob E
2013-07-08 21:36:10 UTC
Permalink
Would it make sense to check this only came from over the local connection only? Or is that even necessary? Since it isn't portable I would assume that might make sense..?

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 08/18] Introduce a non-portable
management TLV for grand masters.
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.
---
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;
+ if (data_len != sizeof(struct grandmaster_settings_np))
+ goto bad_length;
+ gsn = (struct grandmaster_settings_np *) m->data;
+ gsn->clockQuality.offsetScaledLogVariance =
+ ntohs(gsn-
Post by Richard Cochran
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) {
@@ -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;
+ gsn = (struct grandmaster_settings_np *) m->data;
+ gsn->clockQuality.offsetScaledLogVariance =
+ htons(gsn-
Post by Richard Cochran
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
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-09 04:17:52 UTC
Permalink
Post by Keller, Jacob E
Would it make sense to check this only came from over the local connection only? Or is that even necessary? Since it isn't portable I would assume that might make sense..?
Yes, that does makes sense.
The check is done at the clock level, in patch #11.

Thanks,
Richard
Keller, Jacob E
2013-07-09 17:33:17 UTC
Permalink
-----Original Message-----
Sent: Monday, July 08, 2013 9:18 PM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] [PATCH RFC 08/18] Introduce a non-
portable management TLV for grand masters.
Post by Keller, Jacob E
Would it make sense to check this only came from over the local
connection only? Or is that even necessary? Since it isn't portable I
would assume that might make sense..?
Yes, that does makes sense.
The check is done at the clock level, in patch #11.
Thanks,
Richard
Alright, that's good :)

- Jake
Richard Cochran
2013-07-07 19:43:06 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
Keller, Jacob E
2013-07-08 21:39:36 UTC
Permalink
This looks good.

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 09/18] Support the grand master
settings management query.
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.
---
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;
+ 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)
clock_management_send_error(p, msg,
NOT_SUPPORTED);
break;
--
1.7.10.4
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-07 19:43:07 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
Miroslav Lichvar
2013-07-09 06:22:43 UTC
Permalink
Post by Richard Cochran
---
pmc.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
I think the commit message "pmc: Add support for the delay mechanism
management request" is wrong, there doesn't seem to be any code
related to the delay mechanism.

Otherwise looks good to me.
--
Miroslav Lichvar
Richard Cochran
2013-07-09 10:17:49 UTC
Permalink
Post by Miroslav Lichvar
I think the commit message "pmc: Add support for the delay mechanism
management request" is wrong, there doesn't seem to be any code
related to the delay mechanism.
Oops, copy-paste-and-no-edit error. Will fix it.

Thanks,
Richard
Keller, Jacob E
2013-07-12 21:29:13 UTC
Permalink
Looks like the pmc man page was not updated.

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 10/18] pmc: Add support for the
delay mechanism management request.
---
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;
+ 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;
p = (struct portDS *) mgt->data;
if (p->portState > PS_SLAVE) {
--
1.7.10.4
Richard Cochran
2013-07-07 19:43:08 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-07 19:43:09 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
Keller, Jacob E
2013-07-12 21:13:28 UTC
Permalink
This patch does not apply cleanly on the tip of the master branch. I believe it has issues with this patch f8be779f28d98b3a819102c8b68ca283073a2a42 ("send errors if mgmt. tlv length doesn't match action") which is odd because that was a fairly long time ago...

- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 12/18] Throw a state decision event
if the clock quality changes.
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.
---
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
}
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-
Post by Richard Cochran
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)) {
if (clock_management_get_response(c, p, mgt->id, msg))
- return;
+ return changed;
break;
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;
if (mgt->length != 2) {
clock_management_send_error(p, msg,
WRONG_LENGTH);
- return;
+ return changed;
}
break;
- 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);
+ * 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)
break;
- clock_manage(p->clock, p, msg);
+ if (clock_manage(p->clock, p, msg))
+ event = EV_STATE_DECISION_EVENT;
break;
}
--
1.7.10.4
Keller, Jacob E
2013-07-12 21:21:22 UTC
Permalink
Interesting, I took a look at the sourceforge site, and somehow my tree got messed up.

You can ignore my previous email

- Jake
-----Original Message-----
Sent: Friday, July 12, 2013 2:13 PM
Subject: Re: [Linuxptp-devel] [PATCH RFC 12/18] Throw a state decision
event if the clock quality changes.
This patch does not apply cleanly on the tip of the master branch. I
believe it has issues with this patch
f8be779f28d98b3a819102c8b68ca283073a2a42 ("send errors if mgmt.
tlv length doesn't match action") which is odd because that was a fairly
long time ago...
- Jake
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 12/18] Throw a state decision
event
if the clock quality changes.
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.
---
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
}
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-
Post by Richard Cochran
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)) {
if (clock_management_get_response(c, p, mgt->id, msg))
- return;
+ return changed;
break;
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;
if (mgt->length != 2) {
clock_management_send_error(p, msg,
WRONG_LENGTH);
- return;
+ return changed;
}
break;
- 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);
+ * 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)
break;
- clock_manage(p->clock, p, msg);
+ if (clock_manage(p->clock, p, msg))
+ event = EV_STATE_DECISION_EVENT;
break;
}
--
1.7.10.4
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/os
tg.clktrk
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-07 19:43:10 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-07 19:43:11 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 73 insertions(+), 9 deletions(-)

diff --git a/pmc.c b/pmc.c
index f428ff9..a1d8375 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,75 @@ 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;
+
+ if (action == GET) {
+ pmc_send_get_action(pmc, code);
+ return;
+ } else if (action != SET) {
+ 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 +631,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
Miroslav Lichvar
2013-07-08 18:18:02 UTC
Permalink
Post by Richard Cochran
+ 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 ",
Is the string fixed? I think the command might be a bit easier to use
if there weren't the = chars in the string as it would allow to use
the output of the GET command. If I wanted to change one value in the
settings I'd probably try to grep/sed the output and send it back. Do
you think it would help?

Thanks,
--
Miroslav Lichvar
Richard Cochran
2013-07-08 18:36:06 UTC
Permalink
Post by Miroslav Lichvar
Is the string fixed? I think the command might be a bit easier to use
if there weren't the = chars in the string as it would allow to use
the output of the GET command. If I wanted to change one value in the
settings I'd probably try to grep/sed the output and send it back. Do
you think it would help?
Good point. We'll do it without the '=' signs.

Thanks,
Richard
Keller, Jacob E
2013-07-08 21:52:09 UTC
Permalink
-----Original Message-----
Sent: Monday, July 08, 2013 11:36 AM
To: Miroslav Lichvar
Subject: Re: [Linuxptp-devel] [PATCH RFC 14/18] pmc: add our very first
SET command.
Post by Miroslav Lichvar
Is the string fixed? I think the command might be a bit easier to use
if there weren't the = chars in the string as it would allow to use
the output of the GET command. If I wanted to change one value in the
settings I'd probably try to grep/sed the output and send it back. Do
you think it would help?
Good point. We'll do it without the '=' signs.
This also matches the configuration file format as well.

- Jake
Thanks,
Richard
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Keller, Jacob E
2013-07-08 21:45:43 UTC
Permalink
Comment inline...
-----Original Message-----
Sent: Sunday, July 07, 2013 12:43 PM
Subject: [Linuxptp-devel] [PATCH RFC 14/18] pmc: add our very first SET
command.
---
pmc.c | 82
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 73 insertions(+), 9 deletions(-)
diff --git a/pmc.c b/pmc.c
index f428ff9..a1d8375 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 },
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,75 @@ 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;
+
+ if (action == GET) {
+ pmc_send_get_action(pmc, code);
+ return;
+ } else if (action != SET) {
+ fprintf(stderr, "%s only allows GET or SET\n",
+ idtab[index].name);
+ return;
+ }
Feel like it would be better coding style to not use == then != as that can be a bit difficult to follow. However I haven't thought up a good solution to not do that either...

- Jake
+ switch (code) {
+ 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 +631,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
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Richard Cochran
2013-07-09 04:13:55 UTC
Permalink
Post by Keller, Jacob E
Post by Richard Cochran
+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;
+
+ if (action == GET) {
+ pmc_send_get_action(pmc, code);
+ return;
+ } else if (action != SET) {
+ fprintf(stderr, "%s only allows GET or SET\n",
+ idtab[index].name);
+ return;
+ }
Feel like it would be better coding style to not use == then != as that can be a bit difficult to follow. However I haven't thought up a good solution to not do that either...
I am using the "early out" code pattern here.
It could also be a switch/case.

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;
}

Does that seem nicer to you?

Thanks,
Richard
Keller, Jacob E
2013-07-09 17:32:49 UTC
Permalink
-----Original Message-----
Sent: Monday, July 08, 2013 9:14 PM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] [PATCH RFC 14/18] pmc: add our very first
SET command.
Post by Keller, Jacob E
Post by Richard Cochran
+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;
+
+ if (action == GET) {
+ pmc_send_get_action(pmc, code);
+ return;
+ } else if (action != SET) {
+ fprintf(stderr, "%s only allows GET or SET\n",
+ idtab[index].name);
+ return;
+ }
Feel like it would be better coding style to not use == then != as that
can be a bit difficult to follow. However I haven't thought up a good
solution to not do that either...
I am using the "early out" code pattern here.
It could also be a switch/case.
switch (action) {
pmc_send_get_action(pmc, code);
return;
break;
fprintf(stderr, "%s only allows GET or SET\n",
idtab[index].name);
return;
}
Does that seem nicer to you?
Thanks,
Richard
Personally, yes. It makes it more clear what's happening. At least I am more used to using a switch statement with early out vs using a dual if statement that early outs.

I almost wonder if it would be better to use something like:

pcm_perform_action(action, pmc, code) {
switch(action) {
case SET:
pmc_send_set_action(...);
return;
case GET:
pmc_send_get_action(...);
return;
default:
}
}

Not really sure if that really makes more sense, it just seems odd to me.. It makes sense your way also because all sets must have a get, but not all gets have to have a set (obviously, since we don't have many yet).

Not really sure it needs changing, but I know I've been caught by mistake before without realizing something exited early.

Thanks

- Jake
Richard Cochran
2013-07-07 19:43:12 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-07 19:43:13 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-07 19:43:14 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-07 19:43:15 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
Miroslav Lichvar
2013-07-09 10:39:50 UTC
Permalink
Post by Richard Cochran
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.
Beside the comments I made for the sscanf string and the patch
comment message, all patches look good to me.

I noticed a minor unrelated issue while reviewing the code. I will
send a patch shortly.

Thanks,
--
Miroslav Lichvar
Loading...