[Linuxptp-devel] [PATCH 1/3] tsproc: Track the validity of the filtered delay explicitly.

[Richard Cochran]

We will want to test whether a valid filtered delay value has been
calculated or not. However, we cannot simply test for zero since that is
a legitimate possible delay value. This patch adds a flag that reflects
the state of the filtered delay field.

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

diff --git a/tsproc.c b/tsproc.c
index cf5f0dc..1eb24a1 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -42,6 +42,7 @@ struct tsproc {

/* Current filtered delay */
tmv_t filtered_delay;
+ int filtered_delay_valid;

/* Delay filter */
struct filter *delay_filter;
@@ -115,6 +116,7 @@ void tsproc_set_clock_rate_ratio(struct tsproc *tsp, double clock_rate_ratio)
void tsproc_set_delay(struct tsproc *tsp, tmv_t delay)
{
tsp->filtered_delay = delay;
+ tsp->filtered_delay_valid = 1;
}

tmv_t get_raw_delay(struct tsproc *tsp)
@@ -149,6 +151,7 @@ int tsproc_update_delay(struct tsproc *tsp, tmv_t *delay)

raw_delay = get_raw_delay(tsp);
tsp->filtered_delay = filter_sample(tsp->delay_filter, raw_delay);
+ tsp->filtered_delay_valid = 1;

pr_debug("delay filtered %10" PRId64 " raw %10" PRId64,
tsp->filtered_delay, raw_delay);
@@ -199,5 +202,6 @@ void tsproc_reset(struct tsproc *tsp, int full)
if (full) {
tsp->clock_rate_ratio = 1.0;
filter_reset(tsp->delay_filter);
+ tsp->filtered_delay_valid = 0;
}
}

[Richard Cochran]

The time stamp processor features four modes resulting from the
combination of two independent Boolean options. Even though we have a
proper enumerated type to represent the mode, still the code coverts
the mode into two variables.

The tests on the variables are currently simple enough, but soon we
will want to add some more complexity. In the interest of clarity,
this patch converts the paired Boolean tests into a switch/case
pattern.

No functional change.

Signed-off-by: Richard Cochran <***@gmail.com>
---
tsproc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/tsproc.c b/tsproc.c
index 1eb24a1..550f32c 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -26,8 +26,7 @@

struct tsproc {
/* Processing options */
- int raw_mode;
- int weighting;
+ enum tsproc_mode mode;

/* Current ratio between remote and local clock frequency */
double clock_rate_ratio;
@@ -48,6 +47,19 @@ struct tsproc {
struct filter *delay_filter;
};

+static int weighting(struct tsproc *tsp)
+{
+ switch (tsp->mode) {
+ case TSPROC_FILTER:
+ case TSPROC_RAW:
+ return 0;
+ case TSPROC_FILTER_WEIGHT:
+ case TSPROC_RAW_WEIGHT:
+ return 1;
+ }
+ return 0;
+}
+
struct tsproc *tsproc_create(enum tsproc_mode mode,
enum filter_type delay_filter, int filter_length)
{
@@ -59,20 +71,10 @@ struct tsproc *tsproc_create(enum tsproc_mode mode,

switch (mode) {
case TSPROC_FILTER:
- tsp->raw_mode = 0;
- tsp->weighting = 0;
- break;
case TSPROC_RAW:
- tsp->raw_mode = 1;
- tsp->weighting = 0;
- break;
case TSPROC_FILTER_WEIGHT:
- tsp->raw_mode = 0;
- tsp->weighting = 1;
- break;
case TSPROC_RAW_WEIGHT:
- tsp->raw_mode = 1;
- tsp->weighting = 1;
+ tsp->mode = mode;
break;
default:
free(tsp);
@@ -156,24 +158,46 @@ int tsproc_update_delay(struct tsproc *tsp, tmv_t *delay)
pr_debug("delay filtered %10" PRId64 " raw %10" PRId64,
tsp->filtered_delay, raw_delay);

- if (delay)
- *delay = tsp->raw_mode ? raw_delay : tsp->filtered_delay;
+ if (!delay) {
+ return 0;
+ }
+
+ switch (tsp->mode) {
+ case TSPROC_FILTER:
+ case TSPROC_FILTER_WEIGHT:
+ *delay = tsp->filtered_delay;
+ break;
+ case TSPROC_RAW:
+ case TSPROC_RAW_WEIGHT:
+ *delay = raw_delay;
+ break;
+ }

return 0;
}

int tsproc_update_offset(struct tsproc *tsp, tmv_t *offset, double *weight)
{
- tmv_t delay, raw_delay = 0;
+ tmv_t delay = 0, raw_delay = 0;

if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
tmv_is_zero(tsp->t3))
return -1;

- if (tsp->raw_mode || tsp->weighting)
+ switch (tsp->mode) {
+ case TSPROC_FILTER:
+ delay = tsp->filtered_delay;
+ break;
+ case TSPROC_RAW:
+ case TSPROC_RAW_WEIGHT:
raw_delay = get_raw_delay(tsp);
-
- delay = tsp->raw_mode ? raw_delay : tsp->filtered_delay;
+ delay = raw_delay;
+ break;
+ case TSPROC_FILTER_WEIGHT:
+ raw_delay = get_raw_delay(tsp);
+ delay = tsp->filtered_delay;
+ break;
+ }

/* offset = t2 - t1 - delay */
*offset = tmv_sub(tmv_sub(tsp->t2, tsp->t1), delay);
@@ -181,7 +205,7 @@ int tsproc_update_offset(struct tsproc *tsp, tmv_t *offset, double *weight)
if (!weight)
return 0;

- if (tsp->weighting && tsp->filtered_delay > 0 && raw_delay > 0) {
+ if (weighting(tsp) && tsp->filtered_delay > 0 && raw_delay > 0) {
*weight = (double)tsp->filtered_delay / raw_delay;
if (*weight > 1.0)
*weight = 1.0;

[Richard Cochran]

After a servo jump, the call to tsproc_reset() clears tsp->t3. This
causes the next call to tsproc_update_offset() to fail. However the
offset calculation does not necessarily depend on t3, i.e. when
filtered_delay is available and neither raw nor weighting is used.

This patch fixes the issue by allowing the stored filtered delay to be
used when appropriate.

Signed-off-by: Richard Cochran <***@gmail.com>
Reported-by: Burkhard Ilsen <***@gmail.com>
---
tsproc.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tsproc.c b/tsproc.c
index 550f32c..63c989d 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -180,20 +180,28 @@ int tsproc_update_offset(struct tsproc *tsp, tmv_t *offset, double *weight)
{
tmv_t delay = 0, raw_delay = 0;

- if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
- tmv_is_zero(tsp->t3))
+ if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2))
return -1;

switch (tsp->mode) {
case TSPROC_FILTER:
+ if (!tsp->filtered_delay_valid) {
+ return -1;
+ }
delay = tsp->filtered_delay;
break;
case TSPROC_RAW:
case TSPROC_RAW_WEIGHT:
+ if (tmv_is_zero(tsp->t3)) {
+ return -1;
+ }
raw_delay = get_raw_delay(tsp);
delay = raw_delay;
break;
case TSPROC_FILTER_WEIGHT:
+ if (tmv_is_zero(tsp->t3) || !tsp->filtered_delay_valid) {
+ return -1;
+ }
raw_delay = get_raw_delay(tsp);
delay = tsp->filtered_delay;
break;

[Miroslav Lichvar]

Post by Richard Cochran
After a servo jump, the call to tsproc_reset() clears tsp->t3. This
causes the next call to tsproc_update_offset() to fail. However the
offset calculation does not necessarily depend on t3, i.e. when
filtered_delay is available and neither raw nor weighting is used.
This patch fixes the issue by allowing the stored filtered delay to be
used when appropriate.

All three patches look good to me. Thanks.

[Keller, Jacob E]

-----Original Message-----
Sent: Saturday, April 08, 2017 12:21 PM
Subject: [Linuxptp-devel] [PATCH 3/3] tsproc: Allow clock synchronization
immediately after jump.
After a servo jump, the call to tsproc_reset() clears tsp->t3. This
causes the next call to tsproc_update_offset() to fail. However the
offset calculation does not necessarily depend on t3, i.e. when
filtered_delay is available and neither raw nor weighting is used.
This patch fixes the issue by allowing the stored filtered delay to be
used when appropriate.

Yes, this looks much clearer on the intent now.

Thanks,
Jake

---
tsproc.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tsproc.c b/tsproc.c
index 550f32c..63c989d 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -180,20 +180,28 @@ int tsproc_update_offset(struct tsproc *tsp, tmv_t
*offset, double *weight)
{
tmv_t delay = 0, raw_delay = 0;
- if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
- tmv_is_zero(tsp->t3))
+ if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2))
return -1;
switch (tsp->mode) {
+ if (!tsp->filtered_delay_valid) {
+ return -1;
+ }
delay = tsp->filtered_delay;
break;
+ if (tmv_is_zero(tsp->t3)) {
+ return -1;
+ }
raw_delay = get_raw_delay(tsp);
delay = raw_delay;
break;
+ if (tmv_is_zero(tsp->t3) || !tsp->filtered_delay_valid) {
+ return -1;
+ }
raw_delay = get_raw_delay(tsp);
delay = tsp->filtered_delay;
break;
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Keller, Jacob E]

-----Original Message-----
Sent: Saturday, April 08, 2017 12:21 PM
Subject: [Linuxptp-devel] [PATCH 1/3] tsproc: Track the validity of the filtered
delay explicitly.
We will want to test whether a valid filtered delay value has been
calculated or not. However, we cannot simply test for zero since that is
a legitimate possible delay value. This patch adds a flag that reflects
the state of the filtered delay field.

Makes sense.

Thanks,
Jake

---
tsproc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tsproc.c b/tsproc.c
index cf5f0dc..1eb24a1 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -42,6 +42,7 @@ struct tsproc {
/* Current filtered delay */
tmv_t filtered_delay;
+ int filtered_delay_valid;
/* Delay filter */
struct filter *delay_filter;
@@ -115,6 +116,7 @@ void tsproc_set_clock_rate_ratio(struct tsproc *tsp,
double clock_rate_ratio)
void tsproc_set_delay(struct tsproc *tsp, tmv_t delay)
{
tsp->filtered_delay = delay;
+ tsp->filtered_delay_valid = 1;
}
tmv_t get_raw_delay(struct tsproc *tsp)
@@ -149,6 +151,7 @@ int tsproc_update_delay(struct tsproc *tsp, tmv_t *delay)
raw_delay = get_raw_delay(tsp);
tsp->filtered_delay = filter_sample(tsp->delay_filter, raw_delay);
+ tsp->filtered_delay_valid = 1;
pr_debug("delay filtered %10" PRId64 " raw %10" PRId64,
tsp->filtered_delay, raw_delay);
@@ -199,5 +202,6 @@ void tsproc_reset(struct tsproc *tsp, int full)
if (full) {
tsp->clock_rate_ratio = 1.0;
filter_reset(tsp->delay_filter);
+ tsp->filtered_delay_valid = 0;
}
}
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Burkhard Ilsen]

Yes, this works well.
Also using a flag instead of a special value is more linuxptp style.
Thanks,
Burkhard

Post by Richard Cochran
We will want to test whether a valid filtered delay value has been
calculated or not. However, we cannot simply test for zero since that is
a legitimate possible delay value. This patch adds a flag that reflects
the state of the filtered delay field.
---
tsproc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tsproc.c b/tsproc.c
index cf5f0dc..1eb24a1 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -42,6 +42,7 @@ struct tsproc {
/* Current filtered delay */
tmv_t filtered_delay;
+ int filtered_delay_valid;
/* Delay filter */
struct filter *delay_filter;
@@ -115,6 +116,7 @@ void tsproc_set_clock_rate_ratio(struct tsproc *tsp, double clock_rate_ratio)
void tsproc_set_delay(struct tsproc *tsp, tmv_t delay)
{
tsp->filtered_delay = delay;
+ tsp->filtered_delay_valid = 1;
}
tmv_t get_raw_delay(struct tsproc *tsp)
@@ -149,6 +151,7 @@ int tsproc_update_delay(struct tsproc *tsp, tmv_t *delay)
raw_delay = get_raw_delay(tsp);
tsp->filtered_delay = filter_sample(tsp->delay_filter, raw_delay);
+ tsp->filtered_delay_valid = 1;
pr_debug("delay filtered %10" PRId64 " raw %10" PRId64,
tsp->filtered_delay, raw_delay);
@@ -199,5 +202,6 @@ void tsproc_reset(struct tsproc *tsp, int full)
if (full) {
tsp->clock_rate_ratio = 1.0;
filter_reset(tsp->delay_filter);
+ tsp->filtered_delay_valid = 0;
}
}
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Burkhard Ilsen]

Paches 2 and 3 work for me as well.
Code is cleared, performance improved.
Burkhard