On 7/3/2023 7:18 AM, Maciek Machnikowski wrote:
> Currently running servo reports current offsets in numerous tools, but lacks
> statistical data. Having the basic statistical data makes it easier to monitor
> running servo (especially on long runs) and determine servo performance.
>
> This patch reuses existing statistics implemented for tracking clock
> parameters
> and prints basic offset min/avg/max/stddev/rms stats on servo destroy call
> (when the tool is treminated).
>
Only printing the stats on removal seems a bit odd. I wonder if it would
make sense to have a (configurable?) periodic reporting rate that would
print the servo stats maybe every N servo updates, or at intervals?
I like the idea overall, as seeing the status is useful. Alternatively,
it could be something exposed via a non portable operation on the
management port if we don't think it makes sense to log it.
> Signed-off-by: Maciek Machnikowski <mac...@machnikowski.net>
> ---
> makefile | 2 +-
> servo.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> servo_private.h | 6 ++++++
> 3 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/makefile b/makefile
> index 3e3b8b3..a1dea07 100644
> --- a/makefile
> +++ b/makefile
> @@ -24,7 +24,7 @@ CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG)
> $(EXTRA_CFLAGS)
> LDLIBS = -lm -lrt -pthread $(EXTRA_LDFLAGS)
> PRG = ptp4l hwstamp_ctl nsm phc2sys phc_ctl pmc timemaster ts2phc tz2alt
> FILTERS = filter.o mave.o mmedian.o
> -SERVOS = linreg.o ntpshm.o nullf.o pi.o refclock_sock.o servo.o
> +SERVOS = linreg.o ntpshm.o nullf.o pi.o refclock_sock.o stats.o servo.o
> TRANSP = raw.o transport.o udp.o udp6.o uds.o
> TS2PHC = ts2phc.o lstab.o nmea.o serial.o sock.o
> ts2phc_generic_pps_source.o \
> ts2phc_nmea_pps_source.o ts2phc_phc_pps_source.o ts2phc_pps_sink.o
> ts2phc_pps_source.o
> diff --git a/servo.c b/servo.c
> index ea171cd..24713dc 100644
> --- a/servo.c
> +++ b/servo.c
> @@ -26,11 +26,45 @@
> #include "pi.h"
> #include "refclock_sock.h"
> #include "servo_private.h"
> +#include "stats.h"
>
> #include "print.h"
>
> #define NSEC_PER_SEC 1000000000
>
> +static void servo_stats_create(struct servo *servo)
> +{
> + struct servo_stats *stats = &servo->stats;
> +
> + stats->offset = stats_create();
> +}
> +
> +static void servo_stats_destroy(struct servo *servo)
> +{
> + struct servo_stats *stats = &servo->stats;
> +
> + stats_destroy(stats->offset);
> +}
> +
> +void servo_stats_report(struct servo *servo)
> +{
> + struct servo_stats *stats = &servo->stats;
> + struct stats_result os;
> +
> + if (stats_get_result(stats->offset, &os))
> + return;
> +
> + pr_info("offset min/avg/max/stddev/rms %2.0f/%2.0f/%2.0f/%2.0f/%2.0f",
> + os.min, os.mean, os.max, os.stddev, os.rms);
> +}
> +
> +static void servo_stats_add_sample(struct servo *servo, int64_t offset)
> +{
> + struct servo_stats *stats = &servo->stats;
> +
> + stats_add_value(stats->offset, offset);
> +}
> +
> struct servo *servo_create(struct config *cfg, enum servo_type type,
> double fadj, int max_ppb, int sw_ts)
> {
> @@ -90,11 +124,15 @@ struct servo *servo_create(struct config *cfg, enum
> servo_type type,
> servo->num_offset_values = config_get_int(cfg, NULL,
> "servo_num_offset_values");
> servo->curr_offset_values = servo->num_offset_values;
>
> + servo_stats_create(servo);
> +
> return servo;
> }
>
> void servo_destroy(struct servo *servo)
> {
> + servo_stats_report(servo);
> + servo_stats_destroy(servo);
> servo->destroy(servo);
> }
>
> @@ -124,6 +162,8 @@ double servo_sample(struct servo *servo,
>
> r = servo->sample(servo, offset, local_ts, weight, state);
>
> + servo_stats_add_sample(servo, offset);
> +
> switch (*state) {
> case SERVO_UNLOCKED:
> servo->curr_offset_values = servo->num_offset_values;
> @@ -158,6 +198,8 @@ void servo_sync_interval(struct servo *servo, double
> interval)
> void servo_reset(struct servo *servo)
> {
> servo->reset(servo);
> + servo_stats_destroy(servo);
> + servo_stats_create(servo);
> }
>
> double servo_rate_ratio(struct servo *servo)
> diff --git a/servo_private.h b/servo_private.h
> index 4d74ca2..1c1aa9e 100644
> --- a/servo_private.h
> +++ b/servo_private.h
> @@ -23,8 +23,14 @@
>
> #include "contain.h"
> #include "servo.h"
> +#include "stats.h"
> +
> +struct servo_stats {
> + struct stats *offset;
> +};
>
> struct servo {
> + struct servo_stats stats;
> double max_frequency;
> double step_threshold;
> double first_step_threshold;
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
