Thread: Re: [Linuxptp-devel] [RFC v2 0/5] Adding support for AVnu Automotive Profile (Page 4)
PTP IEEE 1588 stack for Linux
Brought to you by:
rcochran
Menu
▾
▴
linuxptp-devel
|
From: Richard C. <ric...@gm...> - 2018-08-18 03:33:41
|
On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote: > > Changes in V2: > ~~~~~~~~~~~ This looks a lot better, thanks! I'll have a few small comments on the patches... > Some Opens: > - Currently, we are using masterOnly (a per-port config option) and slaveOnly > (a global config option) to determine role of the devices. This configuration > option might be a little confusing to a new comer. One idea is to change > slaveOnly to per-port config option. Are there any other ideas for this or > the current way is not as confusing as I think? Most users only have a single port. Probably automotive will also mainly be single port devices. Anyone making a BC or TC already has a super confusing swamp full of options to wade through. In other words, I expect such "power users" to know what they are doing. > - In port_p2p_transition(), we are setting up the delay timer when BMCA is set > as ‘noop’. Usually it is initialized then the device transitions to > PS_LISTENING. But, we are skipping the LISTENING state. These exceptional cases in the series don't seem too bad to me. > Another alternative > is to transition to PS_LISTENING and then unconditionally transfer to > PS_MASTER/PS_SLAVE. What do you mean by "unconditionally"? State transitions are triggered by events. Thanks, Richard |
|
From: Patel, V. <ved...@in...> - 2018-08-21 00:15:47
|
Thanks for the feedback Richard. On Fri, 2018-08-17 at 23:33 -0400, Richard Cochran wrote: > On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote: > > > > > > Changes in V2: > > ~~~~~~~~~~~ > This looks a lot better, thanks! I'll have a few small comments on > the patches... > > > > > Some Opens: > > - Currently, we are using masterOnly (a per-port config option) and > > slaveOnly > > (a global config option) to determine role of the devices. This > > configuration > > option might be a little confusing to a new comer. One idea is to > > change > > slaveOnly to per-port config option. Are there any other ideas > > for this or > > the current way is not as confusing as I think? > Most users only have a single port. Probably automotive will also > mainly be single port devices. Anyone making a BC or TC already has > a > super confusing swamp full of options to wade through. In other > words, I expect such "power users" to know what they are doing. > makes sense. > > > > - In port_p2p_transition(), we are setting up the delay timer when > > BMCA is set > > as ‘noop’. Usually it is initialized then the device transitions > > to > > PS_LISTENING. But, we are skipping the LISTENING state. > These exceptional cases in the series don't seem too bad to me. > > > > > Another alternative > > is to transition to PS_LISTENING and then unconditionally > > transfer to > > PS_MASTER/PS_SLAVE. > What do you mean by "unconditionally"? State transitions are > triggered by events. > Yeah, now that I think of it, I don't think triggering anything unconditionally is going to work. -Vedang > Thanks, > Richard |
|
From: Richard C. <ric...@gm...> - 2018-08-18 03:50:17
|
On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote: > - In port_p2p_transition(), we are setting up the delay timer when BMCA is set > as ‘noop’. Usually it is initialized then the device transitions to > PS_LISTENING. But, we are skipping the LISTENING state. Another alternative > is to transition to PS_LISTENING and then unconditionally transfer to > PS_MASTER/PS_SLAVE. But, that seems more of an hack than what is currently > being done. Any other alternatives? Maybe we can reset the delay timer unconditionally when entering SLAVE or MASTER. It will cause a minor glitch in the delay timing, but will it matter? Thanks, Richard |
|
From: Richard C. <ric...@gm...> - 2018-08-18 04:04:05
|
On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote: > - In port_p2p_transition(), we are setting up the delay timer when BMCA is set > as ‘noop’. Usually it is initialized then the device transitions to > PS_LISTENING. But, we are skipping the LISTENING state. Another alternative > is to transition to PS_LISTENING and then unconditionally transfer to > PS_MASTER/PS_SLAVE. But, that seems more of an hack than what is currently > being done. Any other alternatives? Another idea: Can you have INITIALIZING -> LISTENING, and arrange for EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES to happen right away? Then your fsm does LISTENING -> SLAVE on the EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES event. (haven't looked at arranging that timeout, just brainstorming...) Thanks, Richard |
|
From: Richard C. <ric...@gm...> - 2018-08-18 04:07:49
|
On Thu, Aug 16, 2018 at 10:42:12AM -0700, Vedang Patel wrote:
> diff --git a/fsm.c b/fsm.c
> index ce6efad30937..a0c98891e39f 100644
> --- a/fsm.c
> +++ b/fsm.c
> @@ -335,3 +335,103 @@ enum port_state ptp_slave_fsm(enum port_state state, enum fsm_event event,
>
> return next;
> }
> +
> +enum port_state designated_master_fsm(enum port_state state,
> + enum fsm_event event,
> + int mdiff)
> +{
...
> +}
> +
> +enum port_state designated_slave_fsm(enum port_state state,
> + enum fsm_event event,
> + int mdiff)
> +{
...
> +}
Can these two go into their own, new file?
> @@ -2853,10 +2868,24 @@ struct port *port_open(int phc_index,
> goto err_port;
> }
>
> - p->state_machine = clock_slave_only(clock) ? ptp_slave_fsm : ptp_fsm;
> p->phc_index = phc_index;
> p->jbod = config_get_int(cfg, interface->name, "boundary_clock_jbod");
> transport = config_get_int(cfg, interface->name, "network_transport");
> + p->master_only = config_get_int(cfg, p->name, "masterOnly");
> + p->bmca = config_get_int(cfg, p->name, "BMCA");
> +
> + if (p->bmca == BMCA_NOOP && transport != TRANS_UDS) {
> + if (p->master_only) {
> + p->state_machine = designated_master_fsm;
> + } else if (clock_slave_only(clock)) {
> + p->state_machine = designated_slave_fsm;
> + } else {
> + pr_err("Please enable atleast one of masterOnly or slaveOnly when BMCA == noop.\n");
"at least" -----------------------------------^
Thanks,
Richard
|
|
From: Patel, V. <ved...@in...> - 2018-08-21 00:49:41
|
On Sat, 2018-08-18 at 00:07 -0400, Richard Cochran wrote:
> On Thu, Aug 16, 2018 at 10:42:12AM -0700, Vedang Patel wrote:
> >
> > diff --git a/fsm.c b/fsm.c
> > index ce6efad30937..a0c98891e39f 100644
> > --- a/fsm.c
> > +++ b/fsm.c
> > @@ -335,3 +335,103 @@ enum port_state ptp_slave_fsm(enum port_state
> > state, enum fsm_event event,
> >
> > return next;
> > }
> > +
> > +enum port_state designated_master_fsm(enum port_state state,
> > + enum fsm_event event,
> > + int mdiff)
> > +{
> ...
> >
> > +}
> > +
> > +enum port_state designated_slave_fsm(enum port_state state,
> > + enum fsm_event event,
> > + int mdiff)
> > +{
> ...
> >
> > +}
> Can these two go into their own, new file?
>
Yeah will move them out and name the file designated_fsm.c/.h.
> >
> > @@ -2853,10 +2868,24 @@ struct port *port_open(int phc_index,
> > goto err_port;
> > }
> >
> > - p->state_machine = clock_slave_only(clock) ? ptp_slave_fsm
> > : ptp_fsm;
> > p->phc_index = phc_index;
> > p->jbod = config_get_int(cfg, interface->name,
> > "boundary_clock_jbod");
> > transport = config_get_int(cfg, interface->name,
> > "network_transport");
> > + p->master_only = config_get_int(cfg, p->name,
> > "masterOnly");
> > + p->bmca = config_get_int(cfg, p->name, "BMCA");
> > +
> > + if (p->bmca == BMCA_NOOP && transport != TRANS_UDS) {
> > + if (p->master_only) {
> > + p->state_machine = designated_master_fsm;
> > + } else if (clock_slave_only(clock)) {
> > + p->state_machine = designated_slave_fsm;
> > + } else {
> > + pr_err("Please enable atleast one of
> > masterOnly or slaveOnly when BMCA == noop.\n");
> "at least" -----------------------------------^
>
Will fix this.
Thanks,
Vedang
> Thanks,
> Richard |
|
From: Richard C. <ric...@gm...> - 2018-08-18 04:11:58
|
On Thu, Aug 16, 2018 at 10:42:14AM -0700, Vedang Patel wrote:
> +static int check_source_identity(struct port *p, struct ptp_message *m)
> +{
> + struct PortIdentity master;
> +
> + if (!p->ignore_source_id) {
> + master = clock_parent_identity(p->clock);
> + if (!pid_eq(&master, &m->header.sourcePortIdentity)) {
> + return -1;
> + }
> + }
> + return 0;
> +}
I prefer an "early out" pattern, like:
if (p->ignore_source_id) {
return 0;
}
master = clock_parent_identity(p->clock);
if (!pid_eq(&master, &m->header.sourcePortIdentity)) {
return -1;
}
return 0;
or even:
if (p->ignore_source_id) {
return 0;
}
master = clock_parent_identity(p->clock);
return pid_eq(&master, &m->header.sourcePortIdentity) ? 0 : -1;
Thanks,
Richard
|
|
From: Patel, V. <ved...@in...> - 2018-08-21 00:50:07
|
On Sat, 2018-08-18 at 00:11 -0400, Richard Cochran wrote:
> On Thu, Aug 16, 2018 at 10:42:14AM -0700, Vedang Patel wrote:
>
> >
> > +static int check_source_identity(struct port *p, struct
> > ptp_message *m)
> > +{
> > + struct PortIdentity master;
> > +
> > + if (!p->ignore_source_id) {
> > + master = clock_parent_identity(p->clock);
> > + if (!pid_eq(&master, &m-
> > >header.sourcePortIdentity)) {
> > + return -1;
> > + }
> > + }
> > + return 0;
> > +}
> I prefer an "early out" pattern, like:
>
> if (p->ignore_source_id) {
> return 0;
> }
> master = clock_parent_identity(p->clock);
> if (!pid_eq(&master, &m->header.sourcePortIdentity)) {
> return -1;
> }
> return 0;
>
> or even:
>
> if (p->ignore_source_id) {
> return 0;
> }
> master = clock_parent_identity(p->clock);
> return pid_eq(&master, &m->header.sourcePortIdentity) ? 0 : -1;
>
Will include the second version in v3.
Thanks,
Vedang
> Thanks,
> Richard
>
> |
|
From: Richard C. <ric...@gm...> - 2018-08-18 04:22:30
|
On Thu, Aug 16, 2018 at 10:42:15AM -0700, Vedang Patel wrote: > create mode 100644 configs/AVnu-master.cfg > create mode 100644 configs/AVnu-slave.cfg IIRC, AVnu had some earlier best practice paper about using 802.1-AS. So maybe these file names should include the word "automotive" in them. Actually, considering this https://trademarks.justia.com/778/57/avnu-77857554.html we should avoid the word AVnu altogether. Thanks, Richard |
|
From: Patel, V. <ved...@in...> - 2018-08-21 00:29:18
|
On Fri, 2018-08-17 at 23:50 -0400, Richard Cochran wrote: > On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote: > > > > - In port_p2p_transition(), we are setting up the delay timer when > > BMCA is set > > as ‘noop’. Usually it is initialized then the device transitions > > to > > PS_LISTENING. But, we are skipping the LISTENING state. Another > > alternative > > is to transition to PS_LISTENING and then unconditionally > > transfer to > > PS_MASTER/PS_SLAVE. But, that seems more of an hack than what is > > currently > > being done. Any other alternatives? > Maybe we can reset the delay timer unconditionally when entering > SLAVE > or MASTER. It will cause a minor glitch in the delay timing, but > will > it matter? > This will just mean that the interval between path delay calculation might be atmost 2* delay_interval when you calculate delay the first time after transition to master/slave state. I don't any immediate issue with that. Will have to run some tests and see what really happens. Thanks, Vedang > Thanks, > Richard |
|
From: Patel, V. <ved...@in...> - 2018-08-22 23:50:10
|
On Mon, 2018-08-20 at 17:29 -0700, Patel, Vedang wrote: > On Fri, 2018-08-17 at 23:50 -0400, Richard Cochran wrote: > > > > On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote: > > > > > > > > > - In port_p2p_transition(), we are setting up the delay timer > > > when > > > BMCA is set > > > as ‘noop’. Usually it is initialized then the device > > > transitions > > > to > > > PS_LISTENING. But, we are skipping the LISTENING state. Another > > > alternative > > > is to transition to PS_LISTENING and then unconditionally > > > transfer to > > > PS_MASTER/PS_SLAVE. But, that seems more of an hack than what > > > is > > > currently > > > being done. Any other alternatives? > > Maybe we can reset the delay timer unconditionally when entering > > SLAVE > > or MASTER. It will cause a minor glitch in the delay timing, but > > will > > it matter? > > > This will just mean that the interval between path delay calculation > might be atmost 2* delay_interval when you calculate delay the first > time after transition to master/slave state. I don't any immediate > issue with that. Will have to run some tests and see what really > happens. > I tested this and it works fine. I don't have any issues with it. We can use any one of the solutions (setting very small timeout value in port_initialize(), conditional delay timeouts, or unconditional delay timeouts). Thanks, Vedang > Thanks, > Vedang > > > > Thanks, > > Richard |
|
From: Patel, V. <ved...@in...> - 2018-08-21 00:47:51
|
On Sat, 2018-08-18 at 00:03 -0400, Richard Cochran wrote:
> On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote:
> >
> > - In port_p2p_transition(), we are setting up the delay timer when
> > BMCA is set
> > as ‘noop’. Usually it is initialized then the device transitions
> > to
> > PS_LISTENING. But, we are skipping the LISTENING state. Another
> > alternative
> > is to transition to PS_LISTENING and then unconditionally
> > transfer to
> > PS_MASTER/PS_SLAVE. But, that seems more of an hack than what is
> > currently
> > being done. Any other alternatives?
> Another idea:
>
> Can you have INITIALIZING -> LISTENING, and arrange for
> EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES to happen right away?
>
> Then your fsm does LISTENING -> SLAVE on the
> EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES event.
>
> (haven't looked at arranging that timeout, just brainstorming...)
>
We can do something on the lines of the following (completely untested)
snippet:
diff --git a/port.c b/port.c
index f4a9fbb5a8d7..214a8329d27e 100644
--- a/port.c
+++ b/port.c
@@ -1623,7 +1623,7 @@ int port_initialize(struct port *p)
p->fda.fd[FD_FIRST_TIMER + i] = fd[i];
}
- if (port_set_announce_tmo(p)) {
+ if (set_tmo_log(p->fda.fd[FD_ANNOUNCE_TIMER], 1, -10)) {
goto no_tmo;
}
if (unicast_client_enabled(p) && unicast_client_set_tmo(p)) {
This way, the timer will fire once (immediately after it's setup) even
if inhibit_announce is set.
But, there is going to be a likely effect on the normal working of
linuxptp. We can fix this by only doing this when BMCA == noop. Again,
will have to actually test this to see what issues might pop up.
Thanks,
Vedang
> Thanks,
> Richard
> |
|
From: Patel, V. <ved...@in...> - 2018-08-21 00:51:28
|
On Sat, 2018-08-18 at 00:22 -0400, Richard Cochran wrote: > On Thu, Aug 16, 2018 at 10:42:15AM -0700, Vedang Patel wrote: > > > > create mode 100644 configs/AVnu-master.cfg > > create mode 100644 configs/AVnu-slave.cfg > IIRC, AVnu had some earlier best practice paper about using 802.1-AS. > So maybe these file names should include the word "automotive" in > them. > > Actually, considering this > > https://trademarks.justia.com/778/57/avnu-77857554.html > > we should avoid the word AVnu altogether. > Yeah. Renaming files to automotive-{master,slave}.cfg. Also, do you want me to remove AVnu from the whole series code/commit message or just the config file names? Thanks, Vedang > Thanks, > Richard |
|
From: Richard C. <ric...@gm...> - 2018-08-21 01:55:10
|
On Tue, Aug 21, 2018 at 12:51:18AM +0000, Patel, Vedang wrote: > Also, do you want me to remove AVnu from the whole series code/commit > message or just the config file names? I would just remove the word "AVnu" everywhere and just say, the automotive profile. I want to avoid making the impression that AVnu somehow sponsors or endorsesour implementation. You can keep the link to the PDF in the cover letter. Thanks, Richard |
|
From: Patel, V. <ved...@in...> - 2018-08-22 23:47:44
|
On Mon, 2018-08-20 at 17:47 -0700, Patel, Vedang wrote:
> On Sat, 2018-08-18 at 00:03 -0400, Richard Cochran wrote:
> >
> > On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote:
> > >
> > >
> > > - In port_p2p_transition(), we are setting up the delay timer
> > > when
> > > BMCA is set
> > > as ‘noop’. Usually it is initialized then the device
> > > transitions
> > > to
> > > PS_LISTENING. But, we are skipping the LISTENING state. Another
> > > alternative
> > > is to transition to PS_LISTENING and then unconditionally
> > > transfer to
> > > PS_MASTER/PS_SLAVE. But, that seems more of an hack than what
> > > is
> > > currently
> > > being done. Any other alternatives?
> > Another idea:
> >
> > Can you have INITIALIZING -> LISTENING, and arrange for
> > EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES to happen right away?
> >
> > Then your fsm does LISTENING -> SLAVE on the
> > EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES event.
> >
> > (haven't looked at arranging that timeout, just brainstorming...)
> >
> We can do something on the lines of the following (completely
> untested)
> snippet:
>
> diff --git a/port.c b/port.c
> index f4a9fbb5a8d7..214a8329d27e 100644
> --- a/port.c
> +++ b/port.c
> @@ -1623,7 +1623,7 @@ int port_initialize(struct port *p)
> p->fda.fd[FD_FIRST_TIMER + i] = fd[i];
> }
>
> - if (port_set_announce_tmo(p)) {
> + if (set_tmo_log(p->fda.fd[FD_ANNOUNCE_TIMER], 1, -10)) {
> goto no_tmo;
> }
> if (unicast_client_enabled(p) && unicast_client_set_tmo(p)) {
>
> This way, the timer will fire once (immediately after it's setup)
> even
> if inhibit_announce is set.
>
> But, there is going to be a likely effect on the normal working of
> linuxptp. We can fix this by only doing this when BMCA == noop.
> Again,
> will have to actually test this to see what issues might pop up.
>
The above approach does work if I set the timer to expire in something
like 2^-100 secs.
But, I had another concern with the code flow here. Following is the
flow with relevant functions called to explain what is going on:
bc_dispatch() {
....
port_state_update() {
...
port_initialize() {
...
// to be replaced by set_tmo_log()
port_set_announce_tmo();
...
}
...
}
port_p2p_transition() {
port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
}
}
Here, port_clr_tmo() is happening almost immediately after
port_set_announce_tmo() is setting up the timer. So, the timer never
fires.
This is also occuring when I try to run the lastest upstream code with
gPTP profile. Seems like the timer is not going to fire in any
scenario. Is there something I am missing here or this is a bug?
Thanks,
Vedang
> Thanks,
> Vedang
> >
> > Thanks,
> > Richard |
|
From: Patel, V. <ved...@in...> - 2018-08-24 20:27:40
|
On Wed, 2018-08-22 at 23:47 +0000, Patel, Vedang wrote:
> On Mon, 2018-08-20 at 17:47 -0700, Patel, Vedang wrote:
> >
> > On Sat, 2018-08-18 at 00:03 -0400, Richard Cochran wrote:
> > >
> > >
> > > On Thu, Aug 16, 2018 at 10:42:10AM -0700, Vedang Patel wrote:
> > > >
> > > >
> > > >
> > > > - In port_p2p_transition(), we are setting up the delay timer
> > > > when
> > > > BMCA is set
> > > > as ‘noop’. Usually it is initialized then the device
> > > > transitions
> > > > to
> > > > PS_LISTENING. But, we are skipping the LISTENING state.
> > > > Another
> > > > alternative
> > > > is to transition to PS_LISTENING and then unconditionally
> > > > transfer to
> > > > PS_MASTER/PS_SLAVE. But, that seems more of an hack than what
> > > > is
> > > > currently
> > > > being done. Any other alternatives?
> > > Another idea:
> > >
> > > Can you have INITIALIZING -> LISTENING, and arrange for
> > > EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES to happen right away?
> > >
> > > Then your fsm does LISTENING -> SLAVE on the
> > > EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES event.
> > >
> > > (haven't looked at arranging that timeout, just brainstorming...)
> > >
> > We can do something on the lines of the following (completely
> > untested)
> > snippet:
> >
> > diff --git a/port.c b/port.c
> > index f4a9fbb5a8d7..214a8329d27e 100644
> > --- a/port.c
> > +++ b/port.c
> > @@ -1623,7 +1623,7 @@ int port_initialize(struct port *p)
> > p->fda.fd[FD_FIRST_TIMER + i] = fd[i];
> > }
> >
> > - if (port_set_announce_tmo(p)) {
> > + if (set_tmo_log(p->fda.fd[FD_ANNOUNCE_TIMER], 1, -10)) {
> > goto no_tmo;
> > }
> > if (unicast_client_enabled(p) && unicast_client_set_tmo(p))
> > {
> >
> > This way, the timer will fire once (immediately after it's setup)
> > even
> > if inhibit_announce is set.
> >
> > But, there is going to be a likely effect on the normal working of
> > linuxptp. We can fix this by only doing this when BMCA == noop.
> > Again,
> > will have to actually test this to see what issues might pop up.
> >
> The above approach does work if I set the timer to expire in
> something
> like 2^-100 secs.
>
Looks like the above method won't really work.
First, setting the delay to 2^-100 tries to set the timer to 0.
Also, even if I manually try to set the timer to expire in very small
time like in 10ns, it still does not work with the current behaviour
pointed out below. Whenever the timer is cleared (in port_clr_tmo())
the events pending are also lost and won't be received the next time
you call poll().
I apologize for some misinformation in the last email.
Thanks,
Vedang
> But, I had another concern with the code flow here. Following is the
> flow with relevant functions called to explain what is going on:
>
> bc_dispatch() {
>
> ....
> port_state_update() {
> ...
> port_initialize() {
> ...
> // to be replaced by set_tmo_log()
> port_set_announce_tmo();
>
> ...
> }
> ...
> }
> port_p2p_transition() {
> port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
> }
> }
>
> Here, port_clr_tmo() is happening almost immediately after
> port_set_announce_tmo() is setting up the timer. So, the timer never
> fires.
>
> This is also occuring when I try to run the lastest upstream code
> with
> gPTP profile. Seems like the timer is not going to fire in any
> scenario. Is there something I am missing here or this is a bug?
>
> Thanks,
> Vedang
>
> >
> > 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
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X