Discussion:
[Linuxptp-devel] [PATCH 1/3] Added support for IEEE 802.1AS 2011 bridges.
Erik Hons
2017-06-01 16:05:57 UTC
Permalink
This commit only focuses on time transfer and not BMCA changes.

Signed-off-by: Rodney Greenstreet ***@ni.com<mailto:***@ni.com>

---
clock.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
clock.h | 11 ++++++++
foreign.h | 2 +-
port.c | 92 ++++++++++++++++++++++++++++++++++++++++++---------------------
tmv.h | 16 +++++++++++
5 files changed, 171 insertions(+), 32 deletions(-)

diff --git a/clock.c b/clock.c
index b6afba9..0b8fb56 100644
--- a/clock.c
+++ b/clock.c
@@ -67,6 +67,12 @@ struct clock_stats {
unsigned int max_count;
};
+struct clock_port_sync_info {
+ tmv_t upstream_tx_time;
+ tmv_t origin_ts;
+ tmv_t fup_correction;
+};
+
struct clock_subscriber {
LIST_ENTRY(clock_subscriber) list;
uint8_t events[EVENT_BITMASK_CNT];
@@ -116,6 +122,7 @@ struct clock {
struct tsproc *tsproc;
struct freq_estimator fest;
struct time_status_np status;
+ struct clock_port_sync_info port_sync;
double nrr;
struct clock_description desc;
struct clock_stats stats;
@@ -1097,6 +1104,10 @@ struct clock *clock_create(enum clock_type type, struct config *config,
pr_err("Failed to create time stamp processor");
return NULL;
}
+
+ c->port_sync.upstream_tx_time = tmv_zero();
+ c->port_sync.origin_ts = tmv_zero();
+ c->port_sync.fup_correction = tmv_zero();
c->nrr = 1.0;
c->stats_interval = config_get_int(config, NULL, "summary_interval");
c->stats.offset = stats_create();
@@ -1215,6 +1226,58 @@ void clock_follow_up_info(struct clock *c, struct follow_up_info_tlv *f)
c->status.gmTimeBaseIndicator = f->gmTimeBaseIndicator;
memcpy(&c->status.lastGmPhaseChange, &f->lastGmPhaseChange,
sizeof(c->status.lastGmPhaseChange));
+ pr_debug("clock_follow_up_info()");
+ pr_debug(" cumulativeScaledRateOffset: %d", f->cumulativeScaledRateOffset);
+}
+
+void clock_get_follow_up_info(struct clock *c, struct follow_up_info_tlv *f)
+{
+ f->cumulativeScaledRateOffset = (Integer32) (
+ (clock_to_gm_rate_ratio(c) - 1.0) * POW2_41);
+ f->scaledLastGmPhaseChange = c->status.scaledLastGmPhaseChange;
+ f->gmTimeBaseIndicator = c->status.gmTimeBaseIndicator;
+ f->lastGmPhaseChange = c->status.lastGmPhaseChange;
+
+ pr_debug("clock_get_follow_up_info()");
+ pr_debug(" cumulativeScaledRateOffset: %d", f->cumulativeScaledRateOffset);
+}
+
+void clock_port_sync(struct clock *c, tmv_t ingress_ts, tmv_t origin_ts,
+ tmv_t correction)
+{
+ c->port_sync.upstream_tx_time = tmv_sub(ingress_ts, c->path_delay);
+ c->port_sync.origin_ts = origin_ts;
+ c->port_sync.fup_correction = correction;
+
+ pr_debug("clock_port_sync()");
+ pr_debug(" upstream_tx_time: %"PRIu64, tmv_to_nanoseconds(c->port_sync.upstream_tx_time));
+ pr_debug(" origin_ts: %"PRIu64, tmv_to_nanoseconds(c->port_sync.origin_ts));
+ pr_debug(" fup_correction: %"PRIu64, tmv_to_nanoseconds(c->port_sync.fup_correction));
+}
+
+tmv_t clock_sync_origin_ts(struct clock *c)
+{
+ return c->port_sync.origin_ts;
+}
+
+tmv_t clock_sync_upstream_tx_time(struct clock *c)
+{
+ return c->port_sync.upstream_tx_time;
+}
+
+tmv_t clock_sync_fup_correction(struct clock *c)
+{
+ return c->port_sync.fup_correction;
+}
+
+double clock_to_gm_rate_ratio(struct clock *c)
+{
+ pr_debug("clock_to_gm_rate_ratio()");
+ pr_debug(" nrr: %0.9lf", c->nrr);
+ pr_debug(" cumulativeScaledRateOffset: %d", c->status.cumulativeScaledRateOffset);
+ pr_debug(" cumulativeScaledRateOffset / 2^41: %0.9lf", ((double) (c->status.cumulativeScaledRateOffset)) / POW2_41);
+ pr_debug(" return value: %0.9lf", ((double) (c->status.cumulativeScaledRateOffset)) / POW2_41 + c->nrr);
+ return ((double) (c->status.cumulativeScaledRateOffset)) / POW2_41 + c->nrr;
}
int clock_gm_capable(struct clock *c)
@@ -1222,6 +1285,15 @@ int clock_gm_capable(struct clock *c)
return c->grand_master_capable;
}
+int clock_is_gm(struct clock *c)
+{
+ pr_debug("clock_is_gm()");
+ pr_debug(" this clock: %s", cid2str(&c->dds.clockIdentity));
+ pr_debug(" best clock: %s", cid2str(&c->best_id));
+ pr_debug(" cid_eq: %d", cid_eq(&c->dds.clockIdentity, &c->best_id));
+ return cid_eq(&c->dds.clockIdentity, &c->best_id);
+}
+
struct ClockIdentity clock_identity(struct clock *c)
{
return c->dds.clockIdentity;
@@ -1742,6 +1814,16 @@ static void handle_state_decision_event(struct clock *c)
tsproc_reset(c->tsproc, 1);
c->ingress_ts = tmv_zero();
c->path_delay = 0;
+ c->status.cumulativeScaledRateOffset = 0;
+ c->status.scaledLastGmPhaseChange = 0;
+ c->status.gmTimeBaseIndicator = 0;
+ c->status.lastGmPhaseChange.nanoseconds_msb = 0;
+ c->status.lastGmPhaseChange.nanoseconds_lsb = 0;
+ c->status.lastGmPhaseChange.fractional_nanoseconds = 0;
+ c->status.scaledLastGmPhaseChange = 0;
+ c->port_sync.upstream_tx_time = tmv_zero();
+ c->port_sync.origin_ts = tmv_zero();
+ c->port_sync.fup_correction = tmv_zero();
c->nrr = 1.0;
fresh_best = 1;
}
diff --git a/clock.h b/clock.h
index fcd9328..c127ed7 100644
--- a/clock.h
+++ b/clock.h
@@ -118,6 +118,15 @@ struct port *clock_first_port(struct clock *c);
* @param f Pointer to the TLV.
*/
void clock_follow_up_info(struct clock *c, struct follow_up_info_tlv *f);
+void clock_get_follow_up_info(struct clock *c, struct follow_up_info_tlv *f);
+
+void clock_port_sync(struct clock *c, tmv_t ingress_ts, tmv_t origin_ts,
+ tmv_t correction);
+
+tmv_t clock_sync_origin_ts(struct clock *c);
+tmv_t clock_sync_upstream_tx_time(struct clock *c);
+tmv_t clock_sync_fup_correction(struct clock *c);
+double clock_to_gm_rate_ratio(struct clock *c);
/**
* Obtain the gmCapable flag from a clock's default data set.
@@ -127,6 +136,8 @@ void clock_follow_up_info(struct clock *c, struct follow_up_info_tlv *f);
*/
int clock_gm_capable(struct clock *c);
+int clock_is_gm(struct clock *c);
+
/**
* Obtain a clock's identity from its default data set.
* @param c The clock instance.
diff --git a/foreign.h b/foreign.h
index db24b84..0fdc8e3 100644
--- a/foreign.h
+++ b/foreign.h
@@ -54,7 +54,7 @@ struct foreign_clock {
/**
* Contains the information from the latest announce message
- * in a form suitable for comparision in the BMCA.
+ * in a form suitable for comparison in the BMCA.
*/
struct dataset dataset;
};
diff --git a/port.c b/port.c
index ec02825..aeddfbf 100644
--- a/port.c
+++ b/port.c
@@ -23,6 +23,7 @@
#include <string.h>
#include <unistd.h>
#include <sys/queue.h>
+#include <math.h>
#include "bmc.h"
#include "clock.h"
@@ -430,6 +431,7 @@ static int follow_up_info_append(struct port *p, struct ptp_message *m)
memcpy(fui->id, ieee8021_id, sizeof(ieee8021_id));
fui->subtype[2] = 1;
m->tlv_count = 1;
+ clock_get_follow_up_info(p->clock, fui);
return sizeof(*fui);
}
@@ -1026,23 +1028,18 @@ static void port_slave_priority_warning(struct port *p)
pr_warning("port %hu: defaultDS.priority1 probably misconfigured", n);
}
-static void port_synchronize(struct port *p,
- struct timespec ingress_ts,
- struct timestamp origin_ts,
- Integer64 correction1, Integer64 correction2)
+static void port_synchronize(struct port *p, tmv_t ingress_ts, tmv_t origin_ts,
+ tmv_t sync_correction, tmv_t fup_correction)
{
enum servo_state state;
- tmv_t t1, t1c, t2, c1, c2;
+ tmv_t origin_ts_c;
port_set_sync_rx_tmo(p);
- t1 = timestamp_to_tmv(origin_ts);
- t2 = timespec_to_tmv(ingress_ts);
- c1 = correction_to_tmv(correction1);
- c2 = correction_to_tmv(correction2);
- t1c = tmv_add(t1, tmv_add(c1, c2));
+ origin_ts_c = tmv_add(origin_ts,
+ tmv_add(sync_correction, fup_correction));
- state = clock_synchronize(p->clock, t2, t1c);
+ state = clock_synchronize(p->clock, ingress_ts, origin_ts_c);
switch (state) {
case SERVO_UNLOCKED:
port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
@@ -1074,6 +1071,7 @@ static void port_syfufsm(struct port *p, enum syfu_event event,
struct ptp_message *m)
{
struct ptp_message *syn, *fup;
+ Boolean have_match = FALSE;
switch (p->syfu) {
case SF_EMPTY:
@@ -1112,11 +1110,8 @@ static void port_syfufsm(struct port *p, enum syfu_event event,
break;
case FUP_MATCH:
syn = p->last_syncfup;
- port_synchronize(p, syn->hwts.ts, m->ts.pdu,
- syn->header.correction,
- m->header.correction);
- msg_put(p->last_syncfup);
- p->syfu = SF_EMPTY;
+ fup = m;
+ have_match = TRUE;
break;
}
break;
@@ -1130,12 +1125,9 @@ static void port_syfufsm(struct port *p, enum syfu_event event,
p->syfu = SF_HAVE_SYNC;
break;
case SYNC_MATCH:
+ syn = m;
fup = p->last_syncfup;
- port_synchronize(p, m->hwts.ts, fup->ts.pdu,
- m->header.correction,
- fup->header.correction);
- msg_put(p->last_syncfup);
- p->syfu = SF_EMPTY;
+ have_match = TRUE;
break;
case FUP_MISMATCH:
msg_put(p->last_syncfup);
@@ -1147,6 +1139,33 @@ static void port_syfufsm(struct port *p, enum syfu_event event,
}
break;
}
+
+ if (have_match == TRUE) {
+ tmv_t origin_ts, ingress_ts, sync_correction, fup_correction;
+
+ origin_ts = timestamp_to_tmv(fup->ts.pdu);
+ ingress_ts = timespec_to_tmv(syn->hwts.ts);
+ sync_correction = correction_to_tmv(syn->header.correction);
+ fup_correction = correction_to_tmv(fup->header.correction);
+
+ if (p->follow_up_info) {
+ struct follow_up_info_tlv *fui =
+ follow_up_info_extract(fup);
+ if (!fui)
+ return;
+ clock_follow_up_info(p->clock, fui);
+ }
+
+ if (port_is_ieee8021as(p)) {
+ clock_port_sync(p->clock, ingress_ts, origin_ts,
+ fup_correction);
+ }
+
+ port_synchronize(p, ingress_ts, origin_ts, sync_correction,
+ fup_correction);
+ msg_put(p->last_syncfup);
+ p->syfu = SF_EMPTY;
+ }
}
static int port_pdelay_request(struct port *p)
@@ -1370,7 +1389,23 @@ static int port_tx_sync(struct port *p)
fup->header.control = CTL_FOLLOW_UP;
fup->header.logMessageInterval = p->logSyncInterval;
- ts_to_timestamp(&msg->hwts.ts, &fup->follow_up.preciseOriginTimestamp);
+ if (port_is_ieee8021as(p) && !clock_is_gm(p->clock)) {
+ tmv_t fup_correction;
+
+ fup->follow_up.preciseOriginTimestamp = tmv_to_Timestamp(
+ clock_sync_origin_ts(p->clock));
+ fup_correction = timespec_to_tmv(msg->hwts.ts) -
+ clock_sync_upstream_tx_time(p->clock);
+ fup_correction = dbl_tmv(round(tmv_dbl(fup_correction) *
+ clock_to_gm_rate_ratio(p->clock)));
+ fup_correction += clock_sync_fup_correction(p->clock);
+ fup->header.correction = tmv_to_correction(fup_correction);
+
+ pr_debug("port_tx_sync()");
+ pr_debug(" fup_correction: %"PRIu64, fup_correction);
+ } else{
+ ts_to_timestamp(&msg->hwts.ts, &fup->follow_up.preciseOriginTimestamp);
+ }
err = port_prepare_and_send(p, fup, 0);
if (err)
@@ -1727,13 +1762,6 @@ static void process_follow_up(struct port *p, struct ptp_message *m)
if (memcmp(&master, &m->header.sourcePortIdentity, sizeof(master)))
return;
- if (p->follow_up_info) {
- struct follow_up_info_tlv *fui = follow_up_info_extract(m);
- if (!fui)
- return;
- clock_follow_up_info(p->clock, fui);
- }
-
if (p->syfu == SF_HAVE_SYNC &&
p->last_syncfup->header.sequenceId == m->header.sequenceId) {
event = FUP_MATCH;
@@ -2002,8 +2030,10 @@ static void process_sync(struct port *p, struct ptp_message *m)
m->header.correction += p->asymmetry;
if (one_step(m)) {
- port_synchronize(p, m->hwts.ts, m->ts.pdu,
- m->header.correction, 0);
+ port_synchronize(p, timespec_to_tmv(m->hwts.ts),
+ timestamp_to_tmv(m->ts.pdu),
+ correction_to_tmv(m->header.correction),
+ tmv_zero());
flush_last_sync(p);
return;
}
diff --git a/tmv.h b/tmv.h
index 30b41ee..ca8a5ea 100644
--- a/tmv.h
+++ b/tmv.h
@@ -76,6 +76,11 @@ static inline tmv_t correction_to_tmv(Integer64 c)
return c >> 16;
}
+static inline Integer64 tmv_to_correction(tmv_t c)
+{
+ return c << 16;
+}
+
static inline double tmv_dbl(tmv_t x)
{
return (double) x;
@@ -106,4 +111,15 @@ static inline tmv_t timestamp_to_tmv(struct timestamp ts)
return ts.sec * NS_PER_SEC + ts.nsec;
}
+static inline struct Timestamp tmv_to_Timestamp(tmv_t x)
+{
+ struct Timestamp ts;
+
+ ts.seconds_msb = 0;
+ ts.seconds_lsb = x / NS_PER_SEC;
+ ts.nanoseconds = x % NS_PER_SEC;
+
+ return ts;
+}
+
#endif
--
2.7.4
Richard Cochran
2017-06-06 19:03:37 UTC
Permalink
Apart from the actual logic, this patch (and the other two) have
formatting issues.

First off, these patches don't apply. They are white space damaged.

Secondly:

In order to submit a patch from someone else, please follow this
direction from linux/Documentation/process/submitting-patches.rst:

The ``from`` line must be the very first line in the message body,
and has the form:

From: Original Author <***@example.com>

That way, git will have the correct author.

Thanks,
Richard
Richard Cochran
2017-06-06 19:53:52 UTC
Permalink
This patch doesn't let ptp4l work as a Time Aware Bridge!
Post by Erik Hons
This commit only focuses on time transfer and not BMCA changes.
This change log is totally inadequate. You need to tell us why your
changes are needed. Is something not in compliance with 802.1AS? If
so, what? Or are you adding a new feature? If so, why do we need it?
Post by Erik Hons
clock.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
clock.h | 11 ++++++++
foreign.h | 2 +-
port.c | 92 ++++++++++++++++++++++++++++++++++++++++++---------------------
tmv.h | 16 +++++++++++
5 files changed, 171 insertions(+), 32 deletions(-)
This patch is rather large. You are completely re-writing the
synchronization logic without any explanation. Sorry, but that is not
acceptable.
Post by Erik Hons
@@ -1074,6 +1071,7 @@ static void port_syfufsm(struct port *p, enum syfu_event event,
struct ptp_message *m)
{
struct ptp_message *syn, *fup;
+ Boolean have_match = FALSE;
This is not a C language type.
Post by Erik Hons
@@ -1370,7 +1389,23 @@ static int port_tx_sync(struct port *p)
fup->header.control = CTL_FOLLOW_UP;
fup->header.logMessageInterval = p->logSyncInterval;
- ts_to_timestamp(&msg->hwts.ts, &fup->follow_up.preciseOriginTimestamp);
+ if (port_is_ieee8021as(p) && !clock_is_gm(p->clock)) {
+ tmv_t fup_correction;
+
+ fup->follow_up.preciseOriginTimestamp = tmv_to_Timestamp(
+ clock_sync_origin_ts(p->clock));
+ fup_correction = timespec_to_tmv(msg->hwts.ts) -
+ clock_sync_upstream_tx_time(p->clock);
+ fup_correction = dbl_tmv(round(tmv_dbl(fup_correction) *
+ clock_to_gm_rate_ratio(p->clock)));
+ fup_correction += clock_sync_fup_correction(p->clock);
+ fup->header.correction = tmv_to_correction(fup_correction);
What? This is totally wrong.

The function, port_tx_sync(), is used by the Boundary Clock code. You
cannot simply paste on the TAB functionality here. There are multiple
differences between a BC and a TAB, one of which is the fact that a BC
transmits new Sync messages on an internal timer, but the TAB simply
forwards received Syncs immediately. This is an important behavioral
difference that affects synchronization performance.

If you want to implement a TAB, please do it correctly. Didn't you
see the series that I sent to you, Erik, personally?

16.Oct'16 └─>[PATCH RFC 0/6] TC proof of concept
16.Oct'16 ├─>[PATCH RFC 1/6] ptp4l: add a command line switch for TC mode.
16.Oct'16 ├─>[PATCH RFC 2/6] port: make the dispatch and event methods variable based on clock type.
16.Oct'16 ├─>[PATCH RFC 3/6] port: export a private interface.
16.Oct'16 ├─>[PATCH RFC 4/6] port: share init code, peer delay code, and helpers
16.Oct'16 ├─>[PATCH RFC 5/6] tc: add the transparent clock implementation.
16.Oct'16 └─>[PATCH RFC 6/6] p2p_tc: implement a peer to peer transparent clock.

Actually, I am rather annoyed that you totally ignored those patches
and the TODO list. Instead of implementing a proper TC/TAB, you
disfigured the existing BC in a gross way.

If you really want a TAB, please use the proof of concept as your
starting point.

Thanks,
Richard
Rodney Greenstreet
2017-06-07 14:46:39 UTC
Permalink
Hi Richard,

Thank you for your feedback. I want to explain why we chose to add TAB to your BC implementation rather than adding a new type of device. A TAB is mathematically equivalent to a TC but is functionally equivalent to a BC. More specifically, a TAB has the following BC attributes:
- A TAB participates in the BMC selection.
- A TAB has port state.
- A TAB has a separate logSyncInterval dataset member for each port.

It’s the last point that I think needs further clarification. Under clause 10.2.4 ‘Per-port global variables’ of the IEEE Std 802.1AS™-2011 standard, the following per port variables are defined:

10.2.4.3 currentLogSyncInterval: the current value of the logarithm to base 2 of the mean time interval, in seconds, between the sending of successive time-synchronization event messages (see 10.6.2.3).

10.2.4.4 initialLogSyncInterval: the initial value of the logarithm to base 2 of the mean time interval, in seconds, between the sending of successive time-synchronization event messages (see 10.6.2.3).

10.2.4.5 syncInterval: a variable containing the mean time-synchronization event message transmission interval for the port.

Clause 10.6.2.3 ‘time-synchronization event message transmission interval’ adds the following note:

NOTE—The value of initialLogSyncInterval is the value of the sync interval when the port is initialized. The value of the sync interval may be changed, e.g., if the port receives a Signaling message that carries a message interval request TLV (see 10.5.4.3), and the current value is stored in currentLogSyncInterval. The value of the sync interval can be reset to the initial value, e.g., by a message interval request TLV for which the value of the field timeSyncInterval is 126, see 10.5.4.3.7.

Based on these clauses, it’s clear that each port maintains its own sync interval setting and needs to maintain its own sync interval timer. It’s also clear that when the slave port of a TAB receives a sync event, that message is not immediately sent out on downstream ports. Rather, the configured sync interval for a given port must be adhered to.

Regards,
Rodney Greenstreet
Richard Cochran
2017-06-07 15:38:50 UTC
Permalink
Post by Rodney Greenstreet
- A TAB participates in the BMC selection.
- A TAB has port state.
- A TAB has a separate logSyncInterval dataset member for each port.
It’s the last point that I think needs further clarification.
Sure, there is a per-port value, but this is only relevant when the
port becomes the GM.
Post by Rodney Greenstreet
NOTE—The value of initialLogSyncInterval is the value of the sync
interval when the port is initialized. The value of the sync
interval may be changed, e.g., if the port receives a Signaling
message that carries a message interval request TLV (see 10.5.4.3),
BTW, This part is not (yet) relevant for us, since we don't support
signaling messages at all.
Post by Rodney Greenstreet
Based on these clauses, it’s clear that each port maintains its own
sync interval setting and needs to maintain its own sync interval
timer.
Where in the standard is the operation of the timer specified?
Post by Rodney Greenstreet
It’s also clear that when the slave port of a TAB receives a
sync event, that message is not immediately sent out on downstream
ports. Rather, the configured sync interval for a given port must be
adhered to.
You say this is clear. Where can I read about this? Is it modeled in
the state machines somewhere?

Thanks,
Richard
Rodney Greenstreet
2017-06-07 16:24:52 UTC
Permalink
Post by Richard Cochran
Post by Rodney Greenstreet
- A TAB participates in the BMC selection.
- A TAB has port state.
- A TAB has a separate logSyncInterval dataset member for each port.
It’s the last point that I think needs further clarification.
Sure, there is a per-port value, but this is only relevant when the
port becomes the GM.
Please read clause 10.6.2.3 ‘time-synchronization event message transmission
interval’ and clause 10.6.2.4 ‘Interval for providing synchronization information
by ClockMaster entity’. The last clause makes it very clear that the ClockMaster entity
shall set it’s clockMasterLogSyncInterval to a “value is less than or equal to the
smallest currentLogSyncInterval (see 10.6.2.3) value for all the ports of the time-aware
system.”

This clause makes it very clear that each port will send it’s Sync event based on it’s
configured currentLogSyncInterval.
Post by Richard Cochran
Post by Rodney Greenstreet
NOTE—The value of initialLogSyncInterval is the value of the sync
interval when the port is initialized. The value of the sync
interval may be changed, e.g., if the port receives a Signaling
message that carries a message interval request TLV (see 10.5.4.3),
BTW, This part is not (yet) relevant for us, since we don't support
signaling messages at all.
The point of me adding this clause is the ‘e.g.’. This variable can be changed
on a port-by-port basis by mechanisms such as the signaling TLV, the 802.1AS
defined MIB (defined in section 15), and even a proprietary means such as
a config file.
Post by Richard Cochran
Post by Rodney Greenstreet
Based on these clauses, it’s clear that each port maintains its own
sync interval setting and needs to maintain its own sync interval
timer.
Where in the standard is the operation of the timer specified?
Again, read clauses 10.6.2.3 and 10.6.2.4.
Post by Richard Cochran
Post by Rodney Greenstreet
It’s also clear that when the slave port of a TAB receives a
sync event, that message is not immediately sent out on downstream
ports. Rather, the configured sync interval for a given port must be
adhered to.
You say this is clear. Where can I read about this? Is it modeled in
the state machines somewhere?
Please read section 10.2.11 ‘PortSyncSyncSend state machine’. In the state diagram (figure 10-8),
refer to the left transition from the ‘SET_SYNC_RECEIPT_TIMEOUT_TIME’ state to the ‘SEND_MD_SYNC’
state. The transition cannot occur unless 1/2 of the syncInterval time has elapsed. This state machine
is governed by the given port’s syncInterval.
Post by Richard Cochran
Thanks,
Richard
- A TAB participates in the BMC selection.
- A TAB has port state.
- A TAB has a separate logSyncInterval dataset member for each port.
It’s the last point that I think needs further clarification.
Sure, there is a per-port value, but this is only relevant when the
port becomes the GM.
Post by Richard Cochran
NOTE—The value of initialLogSyncInterval is the value of the sync
interval when the port is initialized. The value of the sync
interval may be changed, e.g., if the port receives a Signaling
message that carries a message interval request TLV (see 10.5.4.3),
BTW, This part is not (yet) relevant for us, since we don't support
signaling messages at all.
Post by Richard Cochran
Based on these clauses, it’s clear that each port maintains its own
sync interval setting and needs to maintain its own sync interval
timer.
Where in the standard is the operation of the timer specified?
Post by Richard Cochran
It’s also clear that when the slave port of a TAB receives a
sync event, that message is not immediately sent out on downstream
ports. Rather, the configured sync interval for a given port must be
adhered to.
You say this is clear. Where can I read about this? Is it modeled in
the state machines somewhere?

Thanks,
Richard
Richard Cochran
2017-06-07 16:35:30 UTC
Permalink
Post by Rodney Greenstreet
Please read clause 10.6.2.3 ‘time-synchronization event message transmission
interval’ and clause 10.6.2.4 ‘Interval for providing synchronization information
by ClockMaster entity’. The last clause makes it very clear that the ClockMaster entity
shall set it’s clockMasterLogSyncInterval to a “value is less than or equal to the
smallest currentLogSyncInterval (see 10.6.2.3) value for all the ports of the time-aware
system.”
This clause makes it very clear that each port will send it’s Sync event based on it’s
configured currentLogSyncInterval.
Only when acting as GM.

Thanks,
Richard
Richard Cochran
2017-06-07 16:11:42 UTC
Permalink
Post by Rodney Greenstreet
Based on these clauses, it’s clear that each port maintains its own
sync interval setting and needs to maintain its own sync interval
timer. It’s also clear that when the slave port of a TAB receives a
sync event, that message is not immediately sent out on downstream
ports. Rather, the configured sync interval for a given port must be
adhered to.
Consider clause B.2.2. There the residence time is limited to 10
milliseconds at maximum. Using an internal timer to forward the Sync
message would violate this requirement.

Thanks,
Richard
Richard Cochran
2017-06-07 16:33:40 UTC
Permalink
Post by Richard Cochran
Post by Rodney Greenstreet
Based on these clauses, it’s clear that each port maintains its own
sync interval setting and needs to maintain its own sync interval
timer. It’s also clear that when the slave port of a TAB receives a
sync event, that message is not immediately sent out on downstream
ports. Rather, the configured sync interval for a given port must be
adhered to.
Consider clause B.2.2. There the residence time is limited to 10
milliseconds at maximum. Using an internal timer to forward the Sync
message would violate this requirement.
Better yet, look at Figure 10-2 on page 46.

The flow of an incoming Sync message is:

MDSyncReceive 11.2.13
PortSyncSyncReceive 10.2.7
SiteSyncSync 10.2.6
PortSyncSyncSend 10.2.11
MDSyncSend 11.2.14

If you follow the state machines, you will see that the Sync message
is forwarded without delay in every case. The *only* place that the
syncInterval is considered is in PortSyncSyncSend, but there it is
used as a timeout value.

Thanks,
Richard
Rodney Greenstreet
2017-06-07 16:51:03 UTC
Permalink
Post by Richard Cochran
Post by Richard Cochran
Post by Rodney Greenstreet
Based on these clauses, it’s clear that each port maintains its own
sync interval setting and needs to maintain its own sync interval
timer. It’s also clear that when the slave port of a TAB receives a
sync event, that message is not immediately sent out on downstream
ports. Rather, the configured sync interval for a given port must be
adhered to.
Consider clause B.2.2. There the residence time is limited to 10
milliseconds at maximum. Using an internal timer to forward the Sync
message would violate this requirement.
Better yet, look at Figure 10-2 on page 46.
MDSyncReceive 11.2.13
PortSyncSyncReceive 10.2.7
SiteSyncSync 10.2.6
PortSyncSyncSend 10.2.11
MDSyncSend 11.2.14
If you follow the state machines, you will see that the Sync message
is forwarded without delay in every case. The *only* place that the
syncInterval is considered is in PortSyncSyncSend, but there it is
used as a timeout value.
Please review this state machine again. The syncInterval is used to govern
the transition back to the ‘SEND_MD_SYNC’ state. This transition only occurs
if the the syncInterval has elapsed and the received Sync message is not from
ourselves and the port is enabled and the port is asCapable and the port is
in the master role.
Post by Richard Cochran
Thanks,
Richard
Post by Richard Cochran
Based on these clauses, it’s clear that each port maintains its own
sync interval setting and needs to maintain its own sync interval
timer. It’s also clear that when the slave port of a TAB receives a
sync event, that message is not immediately sent out on downstream
ports. Rather, the configured sync interval for a given port must be
adhered to.
Consider clause B.2.2. There the residence time is limited to 10
milliseconds at maximum. Using an internal timer to forward the Sync
message would violate this requirement.
Better yet, look at Figure 10-2 on page 46.

The flow of an incoming Sync message is:

MDSyncReceive 11.2.13
PortSyncSyncReceive 10.2.7
SiteSyncSync 10.2.6
PortSyncSyncSend 10.2.11
MDSyncSend 11.2.14

If you follow the state machines, you will see that the Sync message
is forwarded without delay in every case. The *only* place that the
syncInterval is considered is in PortSyncSyncSend, but there it is
used as a timeout value.

Thanks,
Richard
Richard Cochran
2017-06-07 17:16:13 UTC
Permalink
Post by Rodney Greenstreet
Please review this state machine again. The syncInterval is used to govern
the transition back to the ‘SEND_MD_SYNC’ state. This transition only occurs
if the the syncInterval has elapsed and the received Sync message is not from
ourselves and the port is enabled and the port is asCapable and the port is
in the master role.
No, you are really not paying attention.

Here is the condition on the lower left of Figure 10-8:

( ( ( rcvdPSSync &&
(currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
rcvdPSSyncPtr->localPortNumber != thisPort) )
|| ( (currentTime ­ lastSyncSentTime >= syncInterval) &&
(lastRcvdPortNum != thisPort) ) )
&& portEnabled && pttPortEnabled && asCapable &&
selectedRole[thisPort] == MasterPort

Lets make this more readable:

1 (
2 (
3 (rcvdPSSync &&
4 (currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
5 rcvdPSSyncPtr->localPortNumber != thisPort)
6 ) || (
7 (currentTime ­ lastSyncSentTime >= syncInterval) &&
8 (lastRcvdPortNum != thisPort)
9 )
10 )
11 && portEnabled
12 && pttPortEnabled
13 && asCapable
14 && selectedRole[thisPort] == MasterPort

The outer expression (lines 1, 10, and 11-14) checks whether the port
is allow to send at all.

The inner expression has two conditions with a logical OR (line 6).

The first condition triggers immediately on "rcvdPSSync", provided
that the interval is not too short. There is no timer, but rather the
interval from the last transmission is used.

See?

Thanks,
Richard
Rodney Greenstreet
2017-06-07 18:10:58 UTC
Permalink
Post by Richard Cochran
Post by Rodney Greenstreet
Please review this state machine again. The syncInterval is used to govern
the transition back to the ‘SEND_MD_SYNC’ state. This transition only occurs
if the the syncInterval has elapsed and the received Sync message is not from
ourselves and the port is enabled and the port is asCapable and the port is
in the master role.
No, you are really not paying attention.
( ( ( rcvdPSSync &&
(currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
rcvdPSSyncPtr->localPortNumber != thisPort) )
|| ( (currentTime ­ lastSyncSentTime >= syncInterval) &&
(lastRcvdPortNum != thisPort) ) )
&& portEnabled && pttPortEnabled && asCapable &&
selectedRole[thisPort] == MasterPort
1 (
2 (
3 (rcvdPSSync &&
4 (currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
5 rcvdPSSyncPtr->localPortNumber != thisPort)
6 ) || (
7 (currentTime ­ lastSyncSentTime >= syncInterval) &&
8 (lastRcvdPortNum != thisPort)
9 )
10 )
11 && portEnabled
12 && pttPortEnabled
13 && asCapable
14 && selectedRole[thisPort] == MasterPort
The outer expression (lines 1, 10, and 11-14) checks whether the port
is allow to send at all.
The inner expression has two conditions with a logical OR (line 6).
The first condition triggers immediately on "rcvdPSSync", provided
that the interval is not too short. There is no timer, but rather the
interval from the last transmission is used.
The first condition does not trigger “immediately”. It is still governed by
the syncInterval, or at least 1/2 of it. So, if this ports syncInterval is set to
1s and the upstream master’s is set to 125ms, the resulting sync rate from this
port would be 500ms. Since each port can have different sync intervals, even if
a sync message can be sent twice as fast as desired, there still must be a timer.
The following presentation given at a 802.1AS working group meeting addresses this
very issue.

http://www.ieee802.org/1/files/public/docs2014/as-noseworthy-sync-interval-0514-v01.pdf

As a result, the 802.1AS corrigendum 1 (802.1AS-Cor-1) has relaxed the requirement
given in Anex B.2.2.
Post by Richard Cochran
See?
Thanks,
Richard
Please review this state machine again. The syncInterval is used to govern
the transition back to the ‘SEND_MD_SYNC’ state. This transition only occurs
if the the syncInterval has elapsed and the received Sync message is not from
ourselves and the port is enabled and the port is asCapable and the port is
in the master role.
No, you are really not paying attention.

Here is the condition on the lower left of Figure 10-8:

( ( ( rcvdPSSync &&
(currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
rcvdPSSyncPtr->localPortNumber != thisPort) )
|| ( (currentTime ­ lastSyncSentTime >= syncInterval) &&
(lastRcvdPortNum != thisPort) ) )
&& portEnabled && pttPortEnabled && asCapable &&
selectedRole[thisPort] == MasterPort

Lets make this more readable:

1 (
2 (
3 (rcvdPSSync &&
4 (currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
5 rcvdPSSyncPtr->localPortNumber != thisPort)
6 ) || (
7 (currentTime ­ lastSyncSentTime >= syncInterval) &&
8 (lastRcvdPortNum != thisPort)
9 )
10 )
11 && portEnabled
12 && pttPortEnabled
13 && asCapable
14 && selectedRole[thisPort] == MasterPort

The outer expression (lines 1, 10, and 11-14) checks whether the port
is allow to send at all.

The inner expression has two conditions with a logical OR (line 6).

The first condition triggers immediately on "rcvdPSSync", provided
that the interval is not too short. There is no timer, but rather the
interval from the last transmission is used.

See?

Thanks,
Richard
Rodney Greenstreet
2017-06-07 18:20:40 UTC
Permalink
I think this SM is optimized for behaving as you suggest, with the assumption all sync intervals are the same. However, a port’s syncInterval, or some fraction of it, must be adhered to. Also, if a Sync event is not received within a syncInterval for a given port, that port will still transition to ‘SEND_MD_SYNC’ and send a Sync event.
Post by Rodney Greenstreet
Please review this state machine again. The syncInterval is used to govern
the transition back to the ‘SEND_MD_SYNC’ state. This transition only occurs
if the the syncInterval has elapsed and the received Sync message is not from
ourselves and the port is enabled and the port is asCapable and the port is
in the master role.
No, you are really not paying attention.

Here is the condition on the lower left of Figure 10-8:

( ( ( rcvdPSSync &&
(currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
rcvdPSSyncPtr->localPortNumber != thisPort) )
|| ( (currentTime ­ lastSyncSentTime >= syncInterval) &&
(lastRcvdPortNum != thisPort) ) )
&& portEnabled && pttPortEnabled && asCapable &&
selectedRole[thisPort] == MasterPort

Lets make this more readable:

1 (
2 (
3 (rcvdPSSync &&
4 (currentTime ­ lastSyncSentTime >= 0.5*syncInterval) &&
5 rcvdPSSyncPtr->localPortNumber != thisPort)
6 ) || (
7 (currentTime ­ lastSyncSentTime >= syncInterval) &&
8 (lastRcvdPortNum != thisPort)
9 )
10 )
11 && portEnabled
12 && pttPortEnabled
13 && asCapable
14 && selectedRole[thisPort] == MasterPort

The outer expression (lines 1, 10, and 11-14) checks whether the port
is allow to send at all.

The inner expression has two conditions with a logical OR (line 6).

The first condition triggers immediately on "rcvdPSSync", provided
that the interval is not too short. There is no timer, but rather the
interval from the last transmission is used.

See?

Thanks,
Richard
Richard Cochran
2017-06-07 19:49:29 UTC
Permalink
Post by Rodney Greenstreet
I think this SM is optimized for behaving as you suggest, with the
assumption all sync intervals are the same. However, a port’s
syncInterval, or some fraction of it, must be adhered to. Also, if a
Sync event is not received within a syncInterval for a given port,
that port will still transition to ‘SEND_MD_SYNC’ and send a Sync
event.
IOW, the received Sync message is forwarded immediately, provided that
it arrives no sooner than syncInterval/2 after the previous message.

The important fact is that the time window is relative to the last
received Sync message, not to some arbitrary internal timer.

Maybe you aren't aware of it, but prior to the publication of 802.1AS,
the AVB group published results demonstrating improved synchronization
performance when forwarding the Sync ASAP.

Your patch completely misses this central point, and as such does not
implement a Time Aware Bridge according to 802.1AS.

Thanks,
Richard

Loading...