Re: [Linuxptp-devel] [PATCH RFC v3 02/10] Introduce the power profile.

[Olivier D. <oli...@or...>]

Hello Richard,

The 11/09/2021 12:11, Richard Cochran wrote:
> Signed-off-by: Richard Cochran <ric...@gm...>
> ---
>  config.c        | 15 +++++++++++++-
>  port.c          | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  port_private.h  |  3 +++
>  power_profile.h | 31 +++++++++++++++++++++++++++++
>  tlv.h           |  8 ++++++++
>  5 files changed, 109 insertions(+), 1 deletion(-)
>  create mode 100644 power_profile.h
> 
> diff --git a/config.c b/config.c
> index f3c52ba..0996896 100644
> --- a/config.c
> +++ b/config.c
> @@ -30,6 +30,7 @@
>  #include "config.h"
>  #include "ether.h"
>  #include "hash.h"
> +#include "power_profile.h"
>  #include "print.h"
>  #include "util.h"
> 
> @@ -62,7 +63,7 @@ typedef union {
>         char *s;
>  } any_t;
> 
> -#define CONFIG_LABEL_SIZE 32
> +#define CONFIG_LABEL_SIZE 64
> 
>  #define CFG_ITEM_STATIC (1 << 0) /* statically allocated, not to be freed */
>  #define CFG_ITEM_LOCKED (1 << 1) /* command line value, may not be changed */
> @@ -184,6 +185,13 @@ static struct config_enum hwts_filter_enu[] = {
>         { NULL, 0 },
>  };
> 
> +static struct config_enum ieee_c37_238_enu[] = {
> +       { "none", IEEE_C37_238_VERSION_NONE },
> +       { "2011", IEEE_C37_238_VERSION_2011 },
> +       { "2017", IEEE_C37_238_VERSION_2017 },
> +       { NULL, 0 },
> +};
> +
>  static struct config_enum nw_trans_enu[] = {
>         { "L2",    TRANS_IEEE_802_3 },
>         { "UDPv4", TRANS_UDP_IPV4   },
> @@ -292,6 +300,11 @@ struct config_item config_tab[] = {
>         GLOB_ITEM_DBL("pi_proportional_exponent", -0.3, -DBL_MAX, DBL_MAX),
>         GLOB_ITEM_DBL("pi_proportional_norm_max", 0.7, DBL_MIN, 1.0),
>         GLOB_ITEM_DBL("pi_proportional_scale", 0.0, 0.0, DBL_MAX),
> +       PORT_ITEM_ENU("power_profile.version", IEEE_C37_238_VERSION_NONE, ieee_c37_238_enu),
> +       PORT_ITEM_INT("power_profile.2011.grandmasterTimeInaccuracy", 0xFFFFFFFF, 0, INT_MAX),
> +       PORT_ITEM_INT("power_profile.2011.networkTimeInaccuracy", 0, 0, INT_MAX),
> +       PORT_ITEM_INT("power_profile.2017.totalTimeInaccuracy", 0xFFFFFFFF, 0, INT_MAX),
> +       PORT_ITEM_INT("power_profile.grandmasterID", 0, 0, 0xFFFF),
>         GLOB_ITEM_INT("priority1", 128, 0, UINT8_MAX),
>         GLOB_ITEM_INT("priority2", 128, 0, UINT8_MAX),
>         GLOB_ITEM_STR("productDescription", ";;"),
> diff --git a/port.c b/port.c
> index 7e51e77..acd142f 100644
> --- a/port.c
> +++ b/port.c
> @@ -411,6 +411,46 @@ static int follow_up_info_append(struct ptp_message *m)
>         return 0;
>  }
> 
> +static int ieee_c37_238_append(struct port *p, struct ptp_message *m)
> +{
> +       struct ieee_c37_238_2017_tlv *p17;
> +       struct ieee_c37_238_2011_tlv *p11;
> +       struct tlv_extra *extra;
> +
> +       switch (p->pwr.version) {
> +       case IEEE_C37_238_VERSION_NONE:
> +               return 0;
> +       case IEEE_C37_238_VERSION_2011:
> +               extra = msg_tlv_append(m, sizeof(*p11));
> +               if (!extra) {
> +                       return -1;
> +               }
> +               p11 = (struct ieee_c37_238_2011_tlv *) extra->tlv;
> +               p11->type = TLV_ORGANIZATION_EXTENSION;
> +               p11->length = sizeof(*p11) - sizeof(p11->type) - sizeof(p11->length);
> +               memcpy(p11->id, ieeec37_238_id, sizeof(ieeec37_238_id));
> +               p11->subtype[2] = 1;
> +               p11->grandmasterID = p->pwr.grandmasterID;
> +               p11->grandmasterTimeInaccuracy = p->pwr.grandmasterTimeInaccuracy;
> +               p11->networkTimeInaccuracy = p->pwr.networkTimeInaccuracy;
> +               break;
> +       case IEEE_C37_238_VERSION_2017:
> +               extra = msg_tlv_append(m, sizeof(*p17));
> +               if (!extra) {
> +                       return -1;
> +               }
> +               p17 = (struct ieee_c37_238_2017_tlv *) extra->tlv;
> +               p17->type = TLV_ORGANIZATION_EXTENSION;
> +               p17->length = sizeof(*p17) - sizeof(p17->type) - sizeof(p17->length);
> +               memcpy(p17->id, ieeec37_238_id, sizeof(ieeec37_238_id));
> +               p17->subtype[2] = 2;
> +               p17->grandmasterID = p->pwr.grandmasterID;
> +               p17->totalTimeInaccuracy = p->pwr.totalTimeInaccuracy;
> +               break;
> +       }
> +       return 0;
> +}
> +
>  static int net_sync_resp_append(struct port *p, struct ptp_message *m)
>  {
>         struct timePropertiesDS tp = clock_time_properties(p->clock);
> @@ -1501,6 +1541,9 @@ int port_tx_announce(struct port *p, struct address *dst, uint16_t sequence_id)
>         if (p->path_trace_enabled && path_trace_append(p, msg, dad)) {
>                 pr_err("%s: append path trace failed", p->log_name);
>         }
> +       if (ieee_c37_238_append(p, msg)) {
> +               pr_err("%s: append power profile failed", p->log_name);
> +       }
> 
>         err = port_prepare_and_send(p, msg, TRANS_GENERAL);
>         if (err) {
> @@ -3171,6 +3214,16 @@ struct port *port_open(const char *phc_device,
>         p->state = PS_INITIALIZING;
>         p->delayMechanism = config_get_int(cfg, p->name, "delay_mechanism");
>         p->versionNumber = PTP_MAJOR_VERSION;
> +       p->pwr.version =
> +               config_get_int(cfg, p->name, "power_profile.version");
> +       p->pwr.grandmasterID =
> +               config_get_int(cfg, p->name, "power_profile.grandmasterID");
> +       p->pwr.grandmasterTimeInaccuracy =
> +               config_get_int(cfg, p->name, "power_profile.2011.grandmasterTimeInaccuracy");
> +       p->pwr.networkTimeInaccuracy =
> +               config_get_int(cfg, p->name, "power_profile.2011.networkTimeInaccuracy");
> +       p->pwr.totalTimeInaccuracy =
> +               config_get_int(cfg, p->name, "power_profile.2017.totalTimeInaccuracy");
>         p->slave_event_monitor = clock_slave_monitor(clock);
> 
>         if (!port_is_uds(p) && unicast_client_initialize(p)) {
> diff --git a/port_private.h b/port_private.h
> index 19eb944..98ae31b 100644
> --- a/port_private.h
> +++ b/port_private.h
> @@ -26,6 +26,7 @@
>  #include "fsm.h"
>  #include "monitor.h"
>  #include "msg.h"
> +#include "power_profile.h"
>  #include "tmv.h"
> 
>  #define NSEC2SEC 1000000000LL
> @@ -150,6 +151,8 @@ struct port {
>         LIST_HEAD(fm, foreign_clock) foreign_masters;
>         /* TC book keeping */
>         TAILQ_HEAD(tct, tc_txd) tc_transmitted;
> +       /* power profile */
> +       struct ieee_c37_238_settings_np pwr;

I had the feeling that power profiles attributes such as
totalTimeInaccuracy should be properties of the clock.
In case of a simple BC or jbod, all the ports need to share the
same inaccuracy.

This way, we don't rely on users to forward the inaccuracy between ports
with management tlvs. We also know that the totalTimeInaccuracy will
always be the sum of other fields. So i don't think we should allow
setting it through mgt.

For instance:

((T)TI is for (Total)TimeInaccuracy)

clock.internalTI = 0
clock.recv_TTI = 0

update_clockTTI():
	clock.totalTimeInaccuracy = cloc.recv_TTI + clock.internalTI

port received ieee_c37_238 tlv:
	-> clock.recv_TTI = tlv.TTI
	update_clockTTI()

port received MID_POWER_PROFILE_SETTINGS_NP
	-> BC ?
	    (clock.internalTI = distributionTI) :
	    clock.internalTI = grandMasterTI + networkTI
	update_clockTTI()


Olivier