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
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
