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

[Richard C. <ric...@gm...>]

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. 

> - 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.

> - 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.

> 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).

> @@ -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.

> @@ -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. 

Thanks,
Richard