Re: [Linuxptp-devel] [RFC 1/1] Add avnu_ap to enable AVnu Automotive Profile

[Patel, V. <ved...@in...>]

Thanks for your inputs Richard. I will rework the patch as you
suggested. Some comments/questions for the patch inline.

On Thu, 2018-07-19 at 08:01 -0700, Richard Cochran wrote:
> On Mon, Jul 16, 2018 at 02:34:47PM -0700, Vedang Patel wrote:
> > 
> > This adds a new config option "avnu_ap" for ptp4l. Setting this
> > option
> > to 1 will enable the AVnu Automotive Profile (AVnu AP) [1] for the
> > device.
> This is not how we do things.
> 
> I have said this more than once before.  We don't add a single
> monolithic Boolean option for some random profile or extension.
> 
> Instead, for each different behavior or attribute, we add a granular
> option.  Then, the profile is simply a configuration file selecting
> the appropriate options. 
> 
Ok. Will split this patch out into different options. Will send a
follow-up email on what I think the best options are.
> > 
> > - BMCA won't run. So, Announce messages (on master) and announce
> >   timeouts (on slave) will be disabled.
> There is no need to avoid the BMCA.  Just make a BMCA that returns
> static values.  We already have slaveOnly and masterOnly.  Your
> profile should use those.
> 
> For the Announce messages, a simple 'inhibit_announce' option will
> do.
> 
Ok Will add it.
> > 
> > - Source port identity won't be checked while processing Sync and
> >   Follow_Up messages.
> Single option.
> 
> > 
> > - SLAVE devices will not transition to MASTER when it receives Sync
> >   timeout.
> slaveOnly.
> 
Ok Will look into the slaveOnly FSM.
> > diff --git a/fsm.c b/fsm.c
> > index ce6efad30937..af05abdae294 100644
> > --- a/fsm.c
> > +++ b/fsm.c
> > @@ -34,6 +34,17 @@ enum port_state ptp_fsm(enum port_state state,
> > enum fsm_event event, int mdiff)
> >  		case EV_INIT_COMPLETE:
> >  			next = PS_LISTENING;
> >  			break;
> > +		/*
> > +		 * The following 2 cases are specific to AVnu AP.
> > When we are
> > +		 * running AVnu AP, BMCA is not run. So, we need
> > to assign the
> > +		 * state right after the ports initialization is
> > complete.
> > +		 */
> > +		case EV_RS_GRAND_MASTER:
> > +			next = PS_GRAND_MASTER;
> > +			break;
> > +		case EV_RS_SLAVE:
> > +			next = PS_UNCALIBRATED;
> > +			break;
> Don't hack your "special" code into 1588's FSM.  Write your own (if
> needed).
> 
Ok  I will move this to a seperate FSM.
> > 
> > @@ -1717,6 +1722,14 @@ int process_announce(struct port *p, struct
> > ptp_message *m)
> >  		return result;
> >  	}
> >  
> > +	/*
> > +	 * Do not process Announce messages when running AVnu
> > Automotive
> > +	 * Profile, conforming to Section 6.3 point 1 (lines 191-
> > 195).
> > +	 */
> > +	if (clock_avnu_ap(p->clock)) {
> > +		return result;
> > +	}
> This is totally unnecessary, and it hurts the end user by taking an
> option away.  If an Announce is received, you should process it.
> 
Ok. Will remove this.
> > 
> > @@ -2460,7 +2509,18 @@ static enum fsm_event bc_event(struct port
> > *p, int fd_index)
> >  		    port_renew_transport(p)) {
> >  			return EV_FAULT_DETECTED;
> >  		}
> > -		return EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES;
> > +
> > +		/*
> > +		 * When AVnu AP is active, we never want the slave
> > to
> > +		 * transition to the master state. So, returning
> > +		 * EV_SYNCHRONIZATION_FAULT ensures it returns
> > back to
> > +		 * PS_UNCALIBRATED.
> > +		 */
> > +		if (clock_avnu_ap(p->clock)) {
> > +			return EV_SYNCHRONIZATION_FAULT;
> > +		} else {
> > +			return
> > EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES;
> > +		}
> Oh man.  Just make a proper FSM please. 
Will probably be taken care of if I decide to use the slaveOnly FSM for
slaves. Else will include it in the new FSM which will set the BMCA
states which you mentioned before.

Thanks,
Vedang
> 
> Thanks,
> Richard