On Thu, Nov 30, 2023 at 04:32:20PM +0100, Andrew Zaborowski wrote:
> > @@ -249,6 +250,9 @@ struct config_item config_tab[] = {
> > GLOB_ITEM_INT("clock_class_threshold",
> > CLOCK_CLASS_THRESHOLD_DEFAULT, 6, CLOCK_CLASS_THRESHOLD_DEFAULT),
> > GLOB_ITEM_ENU("clock_servo", CLOCK_SERVO_PI, clock_servo_enu),
> > GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
> > + PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
>
> I assume the use of this is simply to set unique names for per-port
> sockets. Maybe something like tempnam() could be used instead.
No, it has to be under user control, because the path might be under a
security policy.
> > @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
> > p->fda.fd[i] = -1;
> > }
> >
> > +static int port_cmlds_initialize(struct port *p)
> > +{
> > + struct config *cfg = clock_config(p->clock);
> > + struct subscribe_events_np sen = {0};
> > + const int zero_datalen = 1;
> > + const UInteger8 hops = 0;
> > +
> > + p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
>
> Should this fall back to port_number(p)?
I don't see how, since there is always a default for each
configuration option. Also, I understood from you guys that the port
number _has_ to be different to pass compliance tests. For that, I
thought adding a "starting port number" option would do the trick.
> > + p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
> > + config_get_string(cfg, p->name,
> > "cmlds_client_address"),
> > + config_get_string(cfg, p->name,
> > "cmlds_server_address"),
> > + hops,
> > + config_get_int(cfg, NULL, "domainNumber"),
> > + config_get_int(cfg, p->name,
> > "transportSpecific") << 4,
>
> The sdoId is (by either IEEE 1588 or 802.1AS) different for the CMLDS
> than for a P2P_COMMON port, I think a check in port_ignore will
> discard the messages as a result. Maybe hardcode TS_CMLDS here? (with
> TS_CMLDS defined in msg.h next to TS_IEEE_8021AS)
>
> Using pmc here surely works but I wonder if it's not actually more
> work than reusing c->uds_port.
I considered that, but I invite you to try implementing that to see
which one is simpler.
> > + switch (mgt->id) {
> > + case MID_CMLDS_INFO_NP:
> > + if (msg->header.sourcePortIdentity.portNumber !=
> > p->cmlds_port) {
> > + break;
> > + }
> > + cmlds = (struct cmlds_info_np *) mgt->data;
> > + p->peer_delay = nanoseconds_to_tmv(cmlds->meanLinkDelay >>
> > 16);
> > + p->peerMeanPathDelay = tmv_to_TimeInterval(p->peer_delay);
>
> cmlds->meanLinkDelay is already a TimeInterval in case you want to use that.
good point
> > +
> > + if (p->state == PS_UNCALIBRATED || p->state == PS_SLAVE) {
> > + const struct timestamp dummy = {0};
> > + const tmv_t tx = timestamp_to_tmv(dummy);
>
> tmv_zero() could also be used.
yeah
> > + clock_peer_delay(p->clock, p->peer_delay, tx, tx,
> > + p->nrate.ratio);
>
> p->nrate.ratio hasn't been set yet, I think a line similar to this is missing:
>
> p->nrate.ratio = 1.0 + (double) cmlds->scaledNeighborRateRatio / POW2_41;
Right
> The CMLDS port will also need to ensure that the NRR is calculated
> because now it's done conditionally.
Huh, I didn't change that part?
> In Kishen's patch series struct cmlds_info_np also had a
> .serviceMeasurementValid which enabled the port_capable() logic to
> work and IIRC that is important at least for 802.1AS compliance. If
> that was to be re-added, the event would need to be emitted in an
> extra place in the CMLDS, perhaps where pdr_missing is incremented.
serviceMeasurementValid is implicit, because if you get a PUSH
notification, it is valid.
Thanks,
Richard
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
