Re: [Linuxptp-devel] [PATCH v5 00/13] Dynamic sync direction for ts2phc

2022-07-23 Thread Richard Cochran
On Mon, Mar 07, 2022 at 11:27:02PM +0200, Vladimir Oltean wrote:
> Would you mind taking another look at these patches?

Sorry about the massive delay.  I'm done reviewing now.

If you are willing to consider the feed back and re-submit the series,
I'll apply it.

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v5 13/13] ts2phc_phc_pps_source: make use of new kernel API for perout waveform

2022-07-23 Thread Richard Cochran
On Tue, Nov 23, 2021 at 02:14:21AM +0200, Vladimir Oltean wrote:
> This API was introduced for 2 reasons:
> 
> 1. Some hardware can emit PPS signals but not starting from arbitrary
>absolute times, but rather just emit at a certain phase offset from
>the beginning of each second. We _could_ patch ts2phc to always
>specify a start time of 0.0 to PTP_PEROUT_REQUEST, and in
>theory that should then become the kernel's responsibility to advance
>that time in the past by an integer number of seconds while keeping
>the phase untouched, but in practice, we would never know whether
>that would actually work with all in-kernel PHC drivers, since it
>wasn't enforced as a requirement before. So there was a need for a
>new flag that only specifies the phase of the periodic signal, and
>not the absolute start time.
> 
> 2. Some hardware can, rather unfortunately, not distinguish between a
>rising and a falling extts edge. And, since whatever rises also has
>to fall before rising again, the strategy in ts2phc is to set a
>'large' pulse width (half the period) and ignore the extts event
>corresponding to the mid-way between one second and another. This is
>all fine, but currently, ts2phc.pulsewidth is a read-only property in
>the config file. The kernel is not instructed in any way to use this
>value, it is simply that must be configured based on prior knowledge
>of the PHC's implementation. This API changes that.
> 
> The introduction of a phase adjustment for the PHC kind of PPS sources
> means we have to adjust our approximation of the precise perout

It is not an approximation.  It is exact, but the correct association
of event to ToD is implicit.

> timestamp. We put that code into a common function and convert all call
> sites to call that. We also need to do the same thing for the edge
> ignoring logic.
> 
> Signed-off-by: Vladimir Oltean 
> ---
> v4->v5: rebase on top of variable renames
> v3->v4: patch is new.
> 
>  config.c|  1 +
>  missing.h   | 52 +
>  ts2phc.8| 17 +++-
>  ts2phc.c| 86 +++--
>  ts2phc.h|  1 +
>  ts2phc_phc_pps_source.c | 44 ++---
>  ts2phc_pps_sink.c   | 16 +++-
>  7 files changed, 180 insertions(+), 37 deletions(-)
> 
> diff --git a/config.c b/config.c
> index f3c52baff765..760d0e12b0b6 100644
> --- a/config.c
> +++ b/config.c
> @@ -321,6 +321,7 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_STR("ts2phc.nmea_remote_host", ""),
>   GLOB_ITEM_STR("ts2phc.nmea_remote_port", ""),
>   GLOB_ITEM_STR("ts2phc.nmea_serialport", "/dev/ttyS0"),
> + PORT_ITEM_INT("ts2phc.perout_phase", -1, 0, 9),
>   PORT_ITEM_INT("ts2phc.pin_index", 0, 0, INT_MAX),
>   GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
>   PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
> diff --git a/missing.h b/missing.h
> index 89cb51360ef7..7f06da3220f2 100644
> --- a/missing.h
> +++ b/missing.h
> @@ -97,6 +97,58 @@ struct compat_ptp_clock_caps {
>  
>  #endif /*LINUX_VERSION_CODE < 5.8*/
>  
> +/*
> + * Bits of the ptp_perout_request.flags field:
> + */
> +
> +#ifndef PTP_PEROUT_ONE_SHOT
> +#define PTP_PEROUT_ONE_SHOT  (1<<0)
> +#endif
> +
> +#ifndef PTP_PEROUT_DUTY_CYCLE
> +#define PTP_PEROUT_DUTY_CYCLE(1<<1)
> +#endif
> +
> +#ifndef PTP_PEROUT_PHASE
> +#define PTP_PEROUT_PHASE (1<<2)
> +#endif
> +
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(5,9,0)
> +
> +struct compat_ptp_perout_request {
> + union {
> + /*
> +  * Absolute start time.
> +  * Valid only if (flags & PTP_PEROUT_PHASE) is unset.
> +  */
> + struct ptp_clock_time start;
> + /*
> +  * Phase offset. The signal should start toggling at an
> +  * unspecified integer multiple of the period, plus this value.
> +  * The start time should be "as soon as possible".
> +  * Valid only if (flags & PTP_PEROUT_PHASE) is set.
> +  */
> + struct ptp_clock_time phase;
> + };
> + struct ptp_clock_time period; /* Desired period, zero means disable. */
> + unsigned int index;   /* Which channel to configure. */
> + unsigned int flags;
> + union {
> + /*
> +  * The "on" time of the signal.
> +  * Must be lower than the period.
> +  * Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
> +  */
> + struct ptp_clock_time on;
> + /* Reserved for future use. */
> + unsigned int rsv[4];
> + };
> +};
> +
> +#define ptp_perout_request compat_ptp_perout_request
> +
> +#endif /* LINUX_VERSION_CODE < 5.9 */
> +
>  #ifndef PTP_MAX_SAMPLES
>  #define PTP_MAX_SAMPLES 25 /* Maximum allowed 

Re: [Linuxptp-devel] [PATCH v5 12/13] ts2phc: allow PHC PPS sources to be synchronized

2022-07-23 Thread Richard Cochran
On Tue, Nov 23, 2021 at 02:14:20AM +0200, Vladimir Oltean wrote:
> Now that we are registering a clock even for the PPS source when it
> supports that (i.e. when it is a PHC), introduce a new API to retrieve
> its clock in order to add timestamps to it.
> 
> The timestamps are a mere approximation.

The time stamps are not an approximation.  They are quite exact.
The phase is definitely at the full second, but the ToD of that second
is only known implicitly.

The word you are looking for is "ambiguous".

> We configure the kernel to emit
> periodic output and then never hear back from that PHC again. We just
> assume that it emits periodic output as promised, and that each pulse is
> emitted at the beginning of each second. We rely on the PPS sinks to
> report an extts event first, and we know who generated that - the PPS
> source, of course. So then we proceed to read the PPS source's PHC time,
> and round that to what is the most plausible integer second in its time
> base. We believe that to be the 'timestamp'. This is fed into the servo
> algorithm.
> 
> The PHC PPS source can be synchronized to the extts events of a PHC
> slave, when in automatic mode.
> 
> Signed-off-by: Vladimir Oltean 
> ---
> v4->v5:
> - rebase on top of data structure renames
> v3->v4:
> Add one more paragraph to commit message.
> v2->v3:
> Implement ts2phc_master_get_clock() as part of ts2phc_master.c instead
> of ts2phc_phc_master.c.
> 
>  ts2phc.c| 45 -
>  ts2phc_phc_pps_source.c |  9 
>  ts2phc_pps_source.c |  8 +++
>  ts2phc_pps_source.h |  2 ++
>  ts2phc_pps_source_private.h |  1 +
>  5 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/ts2phc.c b/ts2phc.c
> index 0968ef28ca73..25c8bb3a5fa6 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -482,6 +482,42 @@ static void ts2phc_synchronize_clocks(struct 
> ts2phc_private *priv, int autocfg)
>   }
>  }
>  
> +static int ts2phc_collect_pps_source_tstamp(struct ts2phc_private *priv)
> +{
> + struct ts2phc_clock *pps_src_clock;
> + struct timespec source_ts;
> + int err;
> +
> + pps_src_clock = ts2phc_pps_source_get_clock(priv->src);
> + /*
> +  * PPS source isn't a PHC (it may be a generic or a GPS PPS source),
> +  * don't error out, just don't do anything. If it doesn't have a PHC,
> +  * there is nothing to synchronize, which is the only point of
> +  * collecting its perout timestamp in the first place.
> +  */
> + if (!pps_src_clock)
> + return 0;
> +
> + err = ts2phc_pps_source_getppstime(priv->src, _ts);
> + if (err < 0) {
> + pr_err("source ts not valid");
> + return err;
> + }
> +
> + /*
> +  * As long as the kernel doesn't support a proper API for reporting
> +  * a precise perout timestamp, we'll have to use this crude
> +  * approximation.
> +  */

Not crude and not an approximation.

> + if (source_ts.tv_nsec > NS_PER_SEC / 2)
> + source_ts.tv_sec++;
> + source_ts.tv_nsec = 0;
> +
> + ts2phc_clock_add_tstamp(pps_src_clock, timespec_to_tmv(source_ts));
> +
> + return 0;
> +}

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel