Re: [Linuxptp-devel] [PATCH V1 1/2] Decouple servo state from automotive profile.
Hi Richard, On May 26, 2020, at 2:04 PM, Richard Cochran mailto:richardcoch...@gmail.com>> wrote: On Tue, May 26, 2020 at 05:04:14PM +, Patel, Vedang wrote: Yes, I think it is remnant from the earlier version of the patch set which was implementing message interval request mechanism inside the FSM for the ’noop' BMCA. I think it should be safe to remove this. I will submit a patch to do it. No need... I already re-worked the man page and push out the changes to master. Maybe review the man page to see if it still makes good sense? Looking at the man-page text for servo_offset_threshold (using man -l ptp4l.8), I see: The offset threshold used in order to transition from the SERVO_LOCKED to the SERVO_LOCKED_STABLE state. The transition occurs once the last The default value of offset_threshold is 0 (disabled). Here, a part of text after ‘…occurs once the last’ is not visible. This is probably because of the single quote (`’`) character at the beginning of the sentence. Also, `offset_threshold` should be `servo_offset_threshold`. Thanks, Vedang Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH V1 1/2] Decouple servo state from automotive profile.
On May 26, 2020, at 10:04 AM, Patel, Vedang mailto:vedang.pa...@intel.com>> wrote: Hi Richard, Jacob, On May 18, 2020, at 4:17 PM, Richard Cochran mailto:richardcoch...@gmail.com>> wrote: On Mon, May 18, 2020 at 04:12:52PM -0700, Jacob Keller wrote: +.B msg_interval_request +This will enable sending message interval request when servo transitions to +SERVO_LOCKED_STABLE state. If msg_interval_request is set, entry into +SERVO_LOCKED_STABLE state triggers the sending of message interval request to +adjust the intervals for Sync and Pdelay request messages. The Sync interval +is adjusted via the signaling mechanism and the pdelay request interval is just +adjusted locally. The new values to use for sync message intervals and pdelay +request intervals can be indicated by operLogSyncInterval and +operLogPdelayReqInterval respectively. This mechanism is currently only +supported when BMCA == 'noop'. The default value of msg_interval_request is 0 +(disabled). This claims that this option only is supported in BMCA == noop, but I don't see how that's enforced.. unless it is buried underneath the other functions? Yeah, I think you are right. The text about BMCA==noop appears to be a Red Herring, even before this patch. Yes, I think it is remnant from the earlier version of the patch set which was implementing message interval request mechanism inside the FSM for the ’noop' BMCA. I think it should be safe to remove this. I will submit a patch to do it. Looks like the line has already been removed in the later version of this patch. -Vedang Thanks, Vedang Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net<mailto:Linuxptp-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net<mailto:Linuxptp-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH V1 1/2] Decouple servo state from automotive profile.
Hi Richard, Jacob, > On May 18, 2020, at 4:17 PM, Richard Cochran wrote: > > On Mon, May 18, 2020 at 04:12:52PM -0700, Jacob Keller wrote: >>> +.B msg_interval_request >>> +This will enable sending message interval request when servo transitions >>> to >>> +SERVO_LOCKED_STABLE state. If msg_interval_request is set, entry into >>> +SERVO_LOCKED_STABLE state triggers the sending of message interval request >>> to >>> +adjust the intervals for Sync and Pdelay request messages. The Sync >>> interval >>> +is adjusted via the signaling mechanism and the pdelay request interval is >>> just >>> +adjusted locally. The new values to use for sync message intervals and >>> pdelay >>> +request intervals can be indicated by operLogSyncInterval and >>> +operLogPdelayReqInterval respectively. This mechanism is currently only >>> +supported when BMCA == 'noop'. The default value of msg_interval_request >>> is 0 >>> +(disabled). >> >> This claims that this option only is supported in BMCA == noop, but I >> don't see how that's enforced.. unless it is buried underneath the other >> functions? > > Yeah, I think you are right. The text about BMCA==noop appears to be > a Red Herring, even before this patch. > Yes, I think it is remnant from the earlier version of the patch set which was implementing message interval request mechanism inside the FSM for the ’noop' BMCA. I think it should be safe to remove this. I will submit a patch to do it. Thanks, Vedang > Thanks, > Richard > > > ___ > Linuxptp-devel mailing list > Linuxptp-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linuxptp-devel ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC V2] Add IEEE 802.1AS-2011 time-aware bridge support
Hi Rodney, Just a small correction. On Mar 25, 2020, at 3:26 PM, Rodney Cummings mailto:rodney.cummi...@ni.com>> wrote: - the SYNCs are forwarded upon reception of a SYNC packet; then the correction field is updated. - there is no concept of syncLocked in the current standards. Only the new 802.1AS-REV (and I guess that will be the 802.1AS-2020?! introduces that. But even in that case, the correction field is updated based on the last received sync. So it is more a TC with some artificial update. 802.1AS-2011 supports changing portDS.logSyncInterval with a Signaling message. Linuxptp doesn't support that, which is totally fine, but we might want to support it someday. Linuxptp does support interval change request via signaling message. :) It was added as part of supporting the AVnu automotive profile in https://sourceforge.net/p/linuxptp/code/ci/630ce719fc518227d59900a66d499de836987fc2/ The full series which added support can be found at: https://sourceforge.net/p/linuxptp/mailman/message/36585436/ Thanks, Vedang ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v5 4/7] port: implement message interval request processing
> On May 6, 2019, at 7:30 AM, Richard Cochran wrote:
>
> On Fri, May 03, 2019 at 09:14:21PM -0700, Richard Cochran wrote:
>> On Thu, Mar 28, 2019 at 08:32:28PM -0700, Vedang Patel wrote:
>>> +int port_tx_interval_request(struct port *p,
>>> +Integer8 announceInterval,
>>> +Integer8 timeSyncInterval,
>>> +Integer8 linkDelayInterval)
>>> +{
>>> + struct msg_interval_req_tlv *mir;
>>> + struct PortIdentity tpid;
>>> + struct ptp_message *msg;
>>> + struct tlv_extra *extra;
>>> + int err;
>>> +
>>> + if (!port_capable(p)) {
>>> + return 0;
>>> + }
>>> +
>>> + memset(, 0, sizeof(tpid.clockIdentity));
>>> + tpid.portNumber = 0xFF;
>>
>> Shouldn't that memset() be to all 0xff ?
>>
>> I'm looking at IEEE 1588 Clause 13.12.1.
>>
>> Or maybe the Automotive Profile says something different?
>
> I see where you got this. In 802.1AS-2011 we have:
>
>10.5.4.2.1 targetPortIdentity (PortIdentity)
>
>The value is 0xFF.
>
> That is clearly (yet another) case of sloppiness in 802.1AS. Here is
> how the type "PortIdentity" is defined:
>
>6.3.3.7 PortIdentity
>
>The PortIdentity type identifies a port of a time-aware system.
>
>struct PortIdentity
>{
> ClockIdentity clockIdentity;
> UInteger16 portNumber;
>};
>
> You can not assign the value 0xFF to a structure. That does not make
> any sense. What they really meant was to set all 1s.
>
> I'll fix up the memset() call and apply the series.
>
I agree with this. I was also confused by the wording of this section. Thanks
for fixing it and merging the series.
-Vedang
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v5 0/7] Support for Automotive Profile (Part II)
Hi Richard, A gentle reminder to add this series to your "list of things to review", if you haven’t already. Thanks, Vedang On 3/28/19, 8:33 PM, "Patel, Vedang" wrote: This are the changes from the last version: - Change abs() -> llabs() in servo.c - Reset the servo when slave detects the logSyncInterval rate mentioned in the signaling message is not the same as it’s own rate. This will probably occur when master restarts. - Remove unused header file (errno.h) from port_signaling.c. More information about the series is in the original cover letter at: https://sourceforge.net/p/linuxptp/mailman/message/36433837/ Thanks, Vedang Patel Vedang Patel (7): port: Make port_capable() accessible outside port.c port: Separate unicast specific code from port_signaling_construct() port: Add logPdelayReqInterval. port: implement message interval request processing clock: add APIs to access servo and servo_state port: Add interval update mechanism. port: Add inhibit_delay_req. clock.c | 11 + clock.h | 14 ++ config.c | 7 ++- configs/automotive-master.cfg | 1 + configs/automotive-slave.cfg | 4 ++ configs/default.cfg | 5 ++ msg.h | 6 +++ phc2sys.c | 1 + port.c| 70 +- port_private.h| 17 +-- port_signaling.c | 111 -- ptp4l.8 | 35 + servo.c | 45 - servo.h | 13 + servo_private.h | 3 ++ tlv.c | 4 ++ tlv.h | 12 + unicast_client.c | 14 +++--- unicast_service.c | 4 +- 19 files changed, 348 insertions(+), 29 deletions(-) -- 2.7.3 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v4 6/7] port: Add interval update mechanism.
> On Mar 26, 2019, at 10:00 AM, Patel, Vedang wrote: > > > >> On Mar 25, 2019, at 9:12 PM, Richard Cochran >> wrote: >> >> On Mon, Mar 25, 2019 at 08:03:32PM +, Patel, Vedang wrote: >>> What do you think? >> >> Perhaps the profile explains this case… >> > Unfortunately, the profile only mentions what to do when there are link-down > and link-up events. Nothing about what to do when the offset exceeds a > certain value. I am guessing that implementation will be OEM specific. > Since the behavior is not defined in the profile and it can be OEM specific, I suggest we leave it out for now. The OEMs can use script this behavior using pmc if they want. We will just have to provide a way to set LOG_SYNC_INTERVAL using PMC. We can revisit this behavior again when 802.1AS-rev is publicly released. Thoughts? Thanks, Vedang > Thanks, > Vedang >> Thanks, >> Richard > > > ___ > Linuxptp-devel mailing list > Linuxptp-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linuxptp-devel ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 6/7] port: Add interval update mechanism.
>> @@ -100,6 +113,12 @@ double servo_sample(struct servo *servo,
>>
>> if (*state != SERVO_UNLOCKED)
>> servo->first_update = 0;
>> +else
>> +servo->curr_offset_values = servo->num_offset_values;
>> +
>> +if (*state == SERVO_LOCKED && check_offset_threshold(servo, offset)) {
>> +*state = SERVO_LOCKED_STABLE;
>> +}
>
> This is getting hard to follow. Time for switch/case(*state) with all the
> cases listed explicitly.
>
After pondering a little bit more, I think that switch/case is not suitable in
this scenario. The current we switch/case is implemented is as follows:
switch (*state) {
case SERVO_UNLOCKED:
servo->curr_offset_values = servo->num_offset_values;
break;
case SERVO_LOCKED:
if (check_offset_threshold(servo, offset)) {
*state = SERVO_LOCKED_STABLE;
}
/* fall through. */
case SERVO_JUMP:
servo->first_update = 0;
break;
case SERVO_LOCKED_STABLE:
break;
}
But, here SERVO_LOCKED_STABLE case will never occur. Also, if we decide to move
out the check_offset_threshold() check outside the switch case so that
SERVO_LOCKED_STABLE case is able to execute, the "servo->curr_offset_values =
servo->num_offset_values;” should also be done before that whenever we reset
the state and state is moved to SERVO_UNLOCKED. So, there is a lot of
interdependency within the code in this switch-case statement and it will
probably be cleaner to keep it as if-else.
Thanks,
Vedang
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v4 6/7] port: Add interval update mechanism.
> On Mar 25, 2019, at 9:12 PM, Richard Cochran wrote: > > On Mon, Mar 25, 2019 at 08:03:32PM +0000, Patel, Vedang wrote: >> What do you think? > > Perhaps the profile explains this case… > Unfortunately, the profile only mentions what to do when there are link-down and link-up events. Nothing about what to do when the offset exceeds a certain value. I am guessing that implementation will be OEM specific. Thanks, Vedang > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v4 6/7] port: Add interval update mechanism.
> On Mar 25, 2019, at 9:08 PM, Richard Cochran wrote:
>
> On Mon, Mar 25, 2019 at 07:46:19PM +0000, Patel, Vedang wrote:
>>>> +static int check_offset_threshold(struct servo *s, int64_t offset)
>>>> +{
>>>> + uint64_t abs_offset = abs(offset);
>>>
>>> abs() returns an int...
>>>
>>>> + if (s->offset_threshold) {
>>>> + if (abs_offset < INT64_MAX && abs_offset < s->offset_threshold
>>> ^^
>>> ... so this test is always true
>>>
>> Sorry I missed this. Will change it to INT_MAX.
>
> But are you sure that works?
>
> What happens when your 'int64_t offset' is degraded into an 'int' in
> the call to abs()?
>
> Better to use llabs(), don't you think?
Yeah llabs should work. But, depending on the machine, int64_t will translate
to either ‘long int’ or 'long long int’. So, do you think imaxabs will be
better to use instead?
Thanks,
Vedang
>
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v4 6/7] port: Add interval update mechanism.
> On Mar 25, 2019, at 7:15 AM, Richard Cochran wrote:
>
> On Mon, Mar 18, 2019 at 03:51:30PM -0700, Vedang Patel wrote:
>> @@ -98,8 +114,21 @@ double servo_sample(struct servo *servo,
>>
>> r = servo->sample(servo, offset, local_ts, weight, state);
>>
>> -if (*state != SERVO_UNLOCKED)
>> +switch (*state) {
>> +case SERVO_UNLOCKED:
>> +servo->curr_offset_values = servo->num_offset_values;
>> +break;
>> +case SERVO_LOCKED:
>> +if (check_offset_threshold(servo, offset)) {
>> +*state = SERVO_LOCKED_STABLE;
>> +}
>> +/* fall through. */
>> +case SERVO_JUMP:
>> servo->first_update = 0;
>> +break;
>> +case SERVO_LOCKED_STABLE:
>
> Once you are in this state, and the offset exceeds the threshold,
> shouldn't the FSM transition back to SERVO_LOCKED?
>
> In this case I would expect the slave to request the higher Sync rate
> once again.
>
> Thoughts?
>
It might be a good idea to transition to the SERVO_LOCKED state. But, I am
wondering if it should be the same as initial interval because it assumes that
the clock offset will be huge in the beginning. If the rate is increased
suddenly because of a single spike in the offset, it might affect other
critical streams which are trying to transmit. The initial offset is probably
assuming that the clock offset is a huge and the system is still booting so
other critical processes have not started yet.
What do you think?
-Vedang
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v4 6/7] port: Add interval update mechanism.
> On Mar 25, 2019, at 7:12 AM, Richard Cochran wrote:
>
> On Mon, Mar 18, 2019 at 03:51:30PM -0700, Vedang Patel wrote:
>> @@ -1123,10 +1123,16 @@ static void port_synchronize(struct port *p,
>> c2 = correction_to_tmv(correction2);
>> t1c = tmv_add(t1, tmv_add(c1, c2));
>>
>> +last_state = clock_servo_state(p->clock);
>> state = clock_synchronize(p->clock, t2, t1c);
>> switch (state) {
>> case SERVO_UNLOCKED:
>> port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
>> +p->logPdelayReqInterval = p->logMinPdelayReqInterval;
>> +p->logSyncInterval = p->initialLogSyncInterval;
>> +port_tx_interval_request(p, SIGNAL_NO_CHANGE,
>> + p->logSyncInterval,
>> + SIGNAL_NO_CHANGE);
>
> This sends a message with every Sync. That isn't what we want.
> Instead, we should send the request only when entering SERVO_UNLOCKED,
> and only if the relevant options are configured.
>
Yes. You are right. We should check whether last state was SERVO_UNLOCKED and
the servo_offset_threshold config option is not 0.
Also, p->logSyncInterval here should be replaced by SIGNAL_SET_INITIAL so slave
does not enforce it’s own config options on the master. This is the reason why
the 11-syncinterval and 23-basedelay were failing.
>> break;
>> case SERVO_JUMP:
>> port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
>> @@ -1139,6 +1145,16 @@ static void port_synchronize(struct port *p,
>> case SERVO_LOCKED:
>> port_dispatch(p, EV_MASTER_CLOCK_SELECTED, 0);
>> break;
>> +case SERVO_LOCKED_STABLE:
>> +if (last_state == SERVO_LOCKED) {
>> +p->logPdelayReqInterval = p->operLogPdelayReqInterval;
>> +p->logSyncInterval = p->operLogSyncInterval;
>> +port_tx_interval_request(p, SIGNAL_NO_CHANGE,
>> + p->logSyncInterval,
>> + SIGNAL_NO_CHANGE);
>
> In contrast, this one is okay, since it only sends once, and only if
> the options are set (otherwise you can't enter SERVO_LOCKED_STABLE).
>
>> +}
>> +port_dispatch(p, EV_MASTER_CLOCK_SELECTED, 0);
>> +break;
>> }
>> }
>>
>> @@ -1613,8 +1629,10 @@ int port_initialize(struct port *p)
>> p->localPriority = config_get_int(cfg, p->name,
>> "G.8275.portDS.localPriority");
>> p->initialLogSyncInterval = config_get_int(cfg, p->name,
>> "logSyncInterval");
>> p->logSyncInterval = p->initialLogSyncInterval;
>> +p->operLogSyncInterval = config_get_int(cfg, p->name,
>> "operLogSyncInterval");
>> p->logMinPdelayReqInterval = config_get_int(cfg, p->name,
>> "logMinPdelayReqInterval");
>> p->logPdelayReqInterval= p->logMinPdelayReqInterval;
>> +p->operLogPdelayReqInterval = config_get_int(cfg, p->name,
>> "operLogPdelayReqInterval");
>> p->neighborPropDelayThresh = config_get_int(cfg, p->name,
>> "neighborPropDelayThresh");
>> p->min_neighbor_prop_delay = config_get_int(cfg, p->name,
>> "min_neighbor_prop_delay");
>>
>> diff --git a/port_private.h b/port_private.h
>> index d28ab0712798..bb1d86e08fb5 100644
>> --- a/port_private.h
>> +++ b/port_private.h
>> @@ -115,9 +115,11 @@ struct port {
>> UInteger8 transportSpecific;
>> UInteger8 localPriority;
>> Integer8initialLogSyncInterval;
>> +Integer8operLogSyncInterval;
>> Integer8logSyncInterval;
>> Enumeration8delayMechanism;
>> Integer8logMinPdelayReqInterval;
>> +Integer8operLogPdelayReqInterval;
>> Integer8logPdelayReqInterval;
>> UInteger32 neighborPropDelayThresh;
>> int follow_up_info;
>> diff --git a/ptp4l.8 b/ptp4l.8
>> index 99b085c148c9..fad21bf57724 100644
>> --- a/ptp4l.8
>> +++ b/ptp4l.8
>> @@ -161,6 +161,13 @@ The mean time interval between Sync messages. A shorter
>> interval may improve
>> accuracy of the local clock. It's specified as a power of two in seconds.
>> The default is 0 (1 second).
>> .TP
>> +.B operLogSyncInterval
>> +The mean time interval between Sync messages. This value is only used by the
>> +slave device when interval_update_timer is enabled. Slave will send this
>> +interval to the master to switch to. This is done via a signaling message
>> after
>> +interval_update_timer expires. It's specified as a power of two in seconds.
>> The
>> +default value is 0 (1 second).
>> +.TP
>> .B logMinDelayReqInterval
>> The minimum permitted mean time interval between Delay_Req messages. A
>> shorter
>> interval makes ptp4l react faster to the changes in the path delay. It's
>> @@ -172,6 +179,13 @@ The minimum permitted mean time interval between
>> Pdelay_Req messages. It's
>>
Re: [Linuxptp-devel] [PATCH v3 6/7] port: Add interval update mechanism.
Thanks Richard for the feedback. Some replies inline.
I agree with all the other feedback you provided for the other 3 patches. I
will make the corresponding change and send the patches by tomorrow.
>
>> +{
>> +if (s->offset_threshold && offset) {
>
> Why test offset != 0 here?
It’s not needed. Will remove it.
>
>> +if (abs(offset) < s->offset_threshold && s->curr_offset_values)
>
> Since you are Using abs(), don't you need a special case test for
> offset > INT_MAX ?
>
Yeah, sorry I missed it. Will check that.
>> +s->curr_offset_values--;
>> +return s->curr_offset_values ? 0 : 1;
>> +}
>> +return 0;
>> +}
>> +
>> double servo_sample(struct servo *servo,
>> int64_t offset,
>> uint64_t local_ts,
>> @@ -100,6 +113,12 @@ double servo_sample(struct servo *servo,
>>
>> if (*state != SERVO_UNLOCKED)
>> servo->first_update = 0;
>> +else
>> +servo->curr_offset_values = servo->num_offset_values;
>> +
>> +if (*state == SERVO_LOCKED && check_offset_threshold(servo, offset)) {
>> +*state = SERVO_LOCKED_STABLE;
>> +}
>
> This is getting hard to follow. Time for switch/case(*state) with all the
> cases listed explicitly.
Yeah will change this to switch/case.
Thanks,
Vedang
>
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 0/7] Support for Automotive Profile (Part II)
Hi Richard, A gentle reminder to provide your feedback on v3 of this series. Thanks, Vedang On 2/12/19, 10:23 AM, "Patel, Vedang" wrote: The changes from the last version are as follows: - Dropped patches which exposed master offset, made process_sync() return an event and introduced the ignore_update_interval_req config option. - All new functions added to port_private.h are in alphabetical order. - Modified the commit message in “port: Split port_signaling_construct() to support Interval Request” such that it highlights what the patch is actually doing. - Incorporated all the feedback on the “port: implement message interval request processing” patch. - Added a new state (SERVO_LOCKED_STABLE) to clock servo. The servo will transition to this state whenever the last ‘n’ offset values are less than the threshold. An “interval request” signaling message will be sent from slave to master on this transition. This message will request the master to increase it’s sync message interval. The slave will also update it’s pdelay request interval on the transition. - Made the inhibit_pdelay_request config option more generic to stop all type of delay requests. Renamed it to inhibit_delay_request. More information about the series is in the original cover letter at: https://sourceforge.net/p/linuxptp/mailman/message/36433837/ Thanks, Vedang Vedang Patel (7): port: Make port_capable() accessible outside port.c port: Separate unicast specific code from port_signaling_construct() port: Add logPdelayReqInterval. port: implement message interval request processing clock: add APIs to access servo and servo_state port: Add interval update mechanism. port: Add inhibit_delay_req. clock.c | 11 clock.h | 14 ++ config.c | 5 ++ configs/automotive-master.cfg | 1 + configs/automotive-slave.cfg | 4 ++ configs/default.cfg | 5 ++ msg.h | 6 +++ phc2sys.c | 1 + port.c| 46 ++--- port_private.h| 17 +-- port_signaling.c | 113 -- ptp4l.8 | 35 + servo.c | 19 +++ servo.h | 6 +++ servo_private.h | 3 ++ tlv.c | 4 ++ tlv.h | 12 + unicast_client.c | 14 +++--- unicast_service.c | 4 +- 19 files changed, 297 insertions(+), 23 deletions(-) -- 2.7.3 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] strangeness
Sorry I missed the part where it is stated that you are using ZyncMP and that supports timestamping. So, disregard the below message. -Vedang On 2/27/19, 2:23 PM, "Patel, Vedang" wrote: On 2/27/19, 2:20 PM, "Keller, Jacob E" wrote: > -Original Message- > From: Paul Thomas [mailto:pthomas8...@gmail.com] > Sent: Wednesday, February 27, 2019 2:06 PM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] strangeness > Using gdb I was able to narrow this down to the ioctl(fd, > SIOCSHWTSTAMP, ); on line 88 of sk.c. When single stepping with > gdb before this line ssh still works fine, after this it is broken > until a reboot. > This sounds very suspicious of a driver bug in the driver's SIOCHWTSTAMP handler. > So tomorrow I was planning to try and create a standalone testcase > with the ioctl() and then try to map that in the kernel driver some. I > thought I would go ahead and post this in case anyone else had any > thoughts. > > HW/SW setup: > This is on an arm64 system (xilinx zynqmp) using the macb ethernet driver: > https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18841740/Macb+Driver > I would think the best bet is to investigate the macb ethernet driver and see what it does, and if anything looks like it would break SSH connections. Also, in the "missing features" section of the above link, I see: * No IEEE 1588 support for Zynq as the timestamp implementation in IP is not accurate enough. This makes me think that timestamping is not supported and the ioctl corresponding to that is not handled properly in the driver. Thanks, Vedang Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] strangeness
On 2/27/19, 2:20 PM, "Keller, Jacob E" wrote: > -Original Message- > From: Paul Thomas [mailto:pthomas8...@gmail.com] > Sent: Wednesday, February 27, 2019 2:06 PM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] strangeness > Using gdb I was able to narrow this down to the ioctl(fd, > SIOCSHWTSTAMP, ); on line 88 of sk.c. When single stepping with > gdb before this line ssh still works fine, after this it is broken > until a reboot. > This sounds very suspicious of a driver bug in the driver's SIOCHWTSTAMP handler. > So tomorrow I was planning to try and create a standalone testcase > with the ioctl() and then try to map that in the kernel driver some. I > thought I would go ahead and post this in case anyone else had any > thoughts. > > HW/SW setup: > This is on an arm64 system (xilinx zynqmp) using the macb ethernet driver: > https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18841740/Macb+Driver > I would think the best bet is to investigate the macb ethernet driver and see what it does, and if anything looks like it would break SSH connections. Also, in the "missing features" section of the above link, I see: * No IEEE 1588 support for Zynq as the timestamp implementation in IP is not accurate enough. This makes me think that timestamping is not supported and the ioctl corresponding to that is not handled properly in the driver. Thanks, Vedang Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 09/10] port: Add interval update mechanism.
Hi Richard,
Let's take a step back and consider the design. What you really want
is a new synchronization state. Something like:
enum servo_state {
SERVO_UNLOCKED,
SERVO_JUMP,
SERVO_LOCKED_ALPHA,
SERVO_LOCKED_BETA,
};
The new code should either go into a new servo type or possibly right
into servo.c. (In either case it should wrap the functionality of the
existing servos.)
I am planning on putting this code right into servo_sample() in servo.c so that
it can be used with any servo type.
When the underlying servo becomes LOCKED, then you enter ALPHA and
start your counter. When the time series of the offset has entered
the acceptable zone, you enter the BETA state.
At the top level (clock.c), on the transition from ALPHA:BETA you send
the signaling message to reduce the Sync rate.
I think that this should go in port.c (in port_synchronize()) because the slave
port is the one which sends the "interval request" message to the master after
the offset with the master port has stabilized. This will be especially
important when we consider a boundary clock with multiple Master ports and a
single slave port.
Thanks,
Vedang
See?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 09/10] port: Add interval update mechanism.
Let's take a step back and consider the design. What you really want
is a new synchronization state. Something like:
enum servo_state {
SERVO_UNLOCKED,
SERVO_JUMP,
SERVO_LOCKED_ALPHA,
SERVO_LOCKED_BETA,
};
The new code should either go into a new servo type or possibly right
into servo.c. (In either case it should wrap the functionality of the
existing servos.)
When the underlying servo becomes LOCKED, then you enter ALPHA and
start your counter. When the time series of the offset has entered
the acceptable zone, you enter the BETA state.
At the top level (clock.c), on the transition from ALPHA:BETA you send
the signaling message to reduce the Sync rate.
Hi Richard,
This makes sense to me. I will make the corresponding changes and send you the
updated series.
I have also gone through all the feedback you provided for other patches in the
series and incorporated the changes.
Thanks,
Vedang
See?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 09/10] port: Add interval update mechanism.
Thanks Richard for your inputs on all the patches.
I am on vacation until 27th January. I will take a close look at the patches
once I get back.
-Vedang
On 1/6/19, 5:31 PM, "Richard Cochran" wrote:
On Wed, Nov 14, 2018 at 02:10:12PM -0800, Vedang Patel wrote:
> Here, we are assuming that the gPTP synchronization is achieved whenever
> the last 'n' offsets calculated using the Sync messages are below a
> certain threshold. Both, the number of offsets to consider and the
> offset threshold are configurable.
Idea is okay, but the implementation? See below...
> Four new config options have been added to provide this functionality:
>
> - offset_threshold: All the offset values being considered should be
> below this value.
> - num_offset_values: number of previously received offset
> values to consider.
Prefix these with servo_
> @@ -1796,6 +1796,10 @@ static void handle_state_decision_event(struct
clock *c)
> clock_update_slave(c);
> event = EV_RS_SLAVE;
> break;
> + /* This is a special case only applicable when BMCA == 'noop'.
*/
> + case PS_UNCALIBRATED:
> + event = EV_NONE;
> + break;
As I said before, we don't want special cases all over the place.
> @@ -2242,6 +2260,18 @@ enum fsm_event process_sync(struct port *p, struct
ptp_message *m)
> }
> port_syfufsm(p, event, m);
>
> + /*
> + * If the master resets before the slave notices, there will be a
> + * descrepency in the interval. Slave should transition to
> + * PS_UNCALIBRATED to fix this.
> + */
> + if (p->offset_threshold &&
> + m->header.logMessageInterval < p->logSyncInterval) {
> + return EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES;
> + } else if (check_offset_threshold(p)) {
> + return EV_RS_SLAVE;
> + }
another special case, ugh
> @@ -2382,9 +2412,25 @@ static void port_p2p_transition(struct port *p,
enum port_state next)
> case PS_UNCALIBRATED:
> flush_last_sync(p);
> flush_peer_delay(p);
> - /* fall through */
> + port_set_announce_tmo(p);
> + if (p->offset_threshold) {
> + p->curr_offset_values = p->num_offset_values;
> + p->logPdelayReqInterval = p->logMinPdelayReqInterval;
> + p->logSyncInterval = p->initialLogSyncInterval;
> + port_tx_signaling(p, SIGNAL_NO_CHANGE,
> + SIGNAL_SET_INITIAL,
> + SIGNAL_NO_CHANGE);
> + }
> + break;
> case PS_SLAVE:
> port_set_announce_tmo(p);
> + if (p->offset_threshold) {
> + p->logPdelayReqInterval = p->operLogPdelayReqInterval;
> + p->logSyncInterval = p->operLogSyncInterval;
> + port_tx_signaling(p, SIGNAL_NO_CHANGE,
> + p->operLogSyncInterval,
> + SIGNAL_NO_CHANGE);
and even more special cases.
Let's take a step back and consider the design. What you really want
is a new synchronization state. Something like:
enum servo_state {
SERVO_UNLOCKED,
SERVO_JUMP,
SERVO_LOCKED_ALPHA,
SERVO_LOCKED_BETA,
};
The new code should either go into a new servo type or possibly right
into servo.c. (In either case it should wrap the functionality of the
existing servos.)
When the underlying servo becomes LOCKED, then you enter ALPHA and
start your counter. When the time series of the offset has entered
the acceptable zone, you enter the BETA state.
At the top level (clock.c), on the transition from ALPHA:BETA you send
the signaling message to reduce the Sync rate.
See?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 00/10] Support for Automotive Profile (Part II)
Hi Richard, On 11/14/18, 2:10 PM, "Patel, Vedang" wrote: The changes from the last version are as follows: - When setting inhibit_announce, there was a stray EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES being sent once every state change. The first patch fixes it. - process_sync() is modified to return fsm_event instead of being void function. This is useful for implementing signaling mechanism. - In patch #9,Instead of using a timer for requesting change in intervals, we are now checking previous values. If the last ‘n’ values are below a certain threshold, the slave will send a signaling message to the master requesting change in interval. - Inhibit_delay_req has been renamed to inhibit_pdelay_req and it will only block pdelay request messages. More information about the series is in the original cover letter at: https://sourceforge.net/p/linuxptp/mailman/message/36433837/ This series was posted over a month ago. Do you have any estimated time on when you will be able to take a look at this series? Thanks, Vedang ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 7/7] port: Add inhibit_delay_req.
> -Original Message-
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Monday, November 5, 2018 9:27 PM
> To: Patel, Vedang
> Cc: linuxptp-devel@lists.sourceforge.net; Sanchez-Palencia, Jesus
> ; Gomes, Vinicius
> ; Guedes, Andre ;
> Hindman, Gavin
> Subject: Re: [PATCH v1 7/7] port: Add inhibit_delay_req.
>
> On Fri, Oct 05, 2018 at 04:25:07PM -0700, Vedang Patel wrote:
> > Add provision to disable delay requests. Designated masters who do not
> > need to calculate delay can use this option. This is required by
> > Automotive Profile to reduce network load.
> >
> > Signed-off-by: Vedang Patel
> > ---
> > config.c | 1 +
> > configs/automotive-master.cfg | 1 +
> > port.c| 5 +
> > port_private.h| 1 +
> > ptp4l.8 | 5 +
> > 5 files changed, 13 insertions(+)
> >
> > diff --git a/config.c b/config.c
> > index 9c8f703d4bbd..bd1f53f93bc3 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -246,6 +246,7 @@ struct config_item config_tab[] = {
> > PORT_ITEM_INT("ingressLatency", 0, INT_MIN, INT_MAX),
> > PORT_ITEM_INT("inhibit_announce", 0, 0, 1),
> > PORT_ITEM_INT("inhibit_multicast_service", 0, 0, 1),
> > + PORT_ITEM_INT("inhibit_delay_req", 0, 0, 1),
>
> Shouldn't this be "inhibit_pdelay_req"?
>
> gPTP aka 802.1-AS is peer delay only, and so is the automotive profile.
>
> Inhibiting e2e delay requests doesn't make sense, because master ports never
> send them.
>
Yeah it makes sense. I will move this to only inhibit pdelay messages.
Thanks,
Vedang
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 6/7] port: Add interval update timer.
Hi Richard, Thanks for your inputs. > -Original Message- > From: Richard Cochran [mailto:richardcoch...@gmail.com] > Sent: Monday, November 5, 2018 9:22 PM > To: Patel, Vedang > Cc: linuxptp-devel@lists.sourceforge.net; Sanchez-Palencia, Jesus > ; Gomes, Vinicius > ; Guedes, Andre ; > Hindman, Gavin > Subject: Re: [PATCH v1 6/7] port: Add interval update timer. > > On Fri, Oct 05, 2018 at 04:25:06PM -0700, Vedang Patel wrote: > > This commit adds functionality to increase the sync and pdelay request > > intervals once gptp synchronization has been achieved. This is useful > > while running Automotive Profile where the network is usually static. > > > > This is done by setting up a timer when the first sync message is > > received by the slave. When the timer expires, the slave will send a > > signaling message which tells the master what interval to switch to. > > So I think using a timer isn't the right approach. How can we be sure the > slave is > always ready after a fixed time? > > Instead, I would have expected this: > > Slave goes LISTENING -> UNCALIBRATED. > > When some pre-defined criterion is fulfilled, then the slave goes UNCALIBRATED > -> SLAVE, and on this state transition the slave then request the slower sync > rate. > > Thoughts? Using the mechanism described above is definitely a better approach than just using a timer. About the criterion, we can check the last 'n' offsets and see if all those are below a threshold. If that is true, we can send the signaling message to increase the sync interval. Both, the offset threshold and the number of values to check, will be configurable parameters. The downside is the addition of 2 more configuration options. What do you think about this approach? Thanks, Vedang > > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 0/7] Support for Automotive Profile (Part II)
Hi Richard, Is this series somewhere in you "TODO:" list? Thanks, Vedang > -Original Message- > From: Patel, Vedang > Sent: Friday, October 5, 2018 4:25 PM > To: linuxptp-devel@lists.sourceforge.net > Cc: richardcoch...@gmail.com; Sanchez-Palencia, Jesus palen...@intel.com>; Gomes, Vinicius ; Guedes, > Andre ; Hindman, Gavin > ; Patel, Vedang > Subject: [PATCH v1 0/7] Support for Automotive Profile (Part II) > > This series is the continuation of the automotive series posted at [1]. > Following are the main changes this series makes: > - Provide a mechanism to save the initial values of Sync and Announce > intervals. > - Implement “Message Interval Request” support for linuxptp described as part > of 10.5.4 in 802.1AS[2]. > - Use the interval request mechanism for slave to request a change the > interval > of Sync messages to operLogSyncInterval. Signaling message will be sent to > the master after the interval update timer expires. The timer is setup when > the first Sync Message is received by slave. More information for this in > sections 6.2.1.6, 6.2.3.1 and 6.2.4 of AVnu Automotive Profile[3]. > - The timer expiration also sets the pdelay request interval to > operPdelayReqInterval on slave. This adds functionality mentioned in > Sections > 6.2.1.5 and 6.2.3.2. > - Add a flag to inhibit delay requests. This will be used by the master in the > automotive network because master devices do not need to send pdelay > request > messages. > > Thanks, > Vedang Patel > > [1]- https://sourceforge.net/p/linuxptp/mailman/message/36431575/ > [2]- IEEE 802.1AS Standard: > https://ieeexplore.ieee.org/stamp/stamp.jsp?tp==5741898=1 > [3]- Automotive Ethernet AVB Functional and Interoperability Specification - > http://avnu.org/wp-content/uploads/2014/05/ > Automotive-Ethernet-AVB-Func-Interop-Spec-v1.5-Public.pdf > > Vedang Patel (7): > port: Make port_capable() accessible outside port.c > port: Split port_signaling_construct() to support Interval Request > port: Add logPdelayReqInterval. > port: implement message interval request processing > port: Add ignore_update_interval_req. > port: Add interval update timer. > port: Add inhibit_delay_req. > > config.c | 5 ++ > configs/automotive-master.cfg | 1 + > configs/automotive-slave.cfg | 4 ++ > configs/default.cfg | 4 ++ > fd.h | 3 +- > msg.h | 6 +++ > port.c| 55 ++-- > port_private.h| 20 ++- > port_signaling.c | 118 > +- > ptp4l.8 | 32 > tlv.h | 12 + > unicast_client.c | 14 ++--- > unicast_service.c | 4 +- > 13 files changed, 261 insertions(+), 17 deletions(-) > > -- > 2.7.3 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] port: correction of port name used port_open
On Wed, 2018-10-10 at 11:01 +, Anders Selhammer wrote:
> Correction of fault introduced in "Add BMCA config option."
>
> Signed-off-by: Anders Selhammer
Sorry I missed this in my patches. Thanks for fixing it.
Reviewed-by: Vedang Patel
> ---
> port.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/port.c b/port.c
> index 3e61179..142b970 100644
> --- a/port.c
> +++ b/port.c
> @@ -2901,8 +2901,8 @@ struct port *port_open(int phc_index,
> 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");
> + p->master_only = config_get_int(cfg, interface->name,
> "masterOnly");
> + p->bmca = config_get_int(cfg, interface->name, "BMCA");
>
> if (p->bmca == BMCA_NOOP && transport != TRANS_UDS) {
> if (p->master_only) {
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 0/7] Adding support for Automotive Profile
On Thu, 2018-10-04 at 20:20 -0700, Richard Cochran wrote: > On Wed, Oct 03, 2018 at 09:41:45AM -0700, Vedang Patel wrote: > > > > Changes in v3: > > ~~~ > > > > There is only a couple of changes in this version of the series. > > > > - We clear out the SYNC_RX_TIMER in bc_event() only when BMCA == > > 'noop'. > > - Fix typo in documentation of ignore_source_id. > > > > More information of intention of this and future series at: > > https://sourceforge.net/p/linuxptp/mailman/message/36369408/ > Series applied. I appreciate your following through on this! > Thanks Richard! I will be sending out the next series soon. :) > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Tue, 2018-10-02 at 16:00 -0700, Richard Cochran wrote: > On Tue, Oct 02, 2018 at 04:25:32PM +0000, Patel, Vedang wrote: > > > > We still need to clear out the timer when the device transitions > > out of > > slave state for other events like EV_FAULT_DETECTED. So, I think we > > still need to keep it there. > So, IOW, the reset is a special case for the noop-BCMA, right? > Yes, it is a special case. So, I guess it makes sense to do it only when we are running noop-BMCA. Thanks, Vedang > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Mon, 2018-10-01 at 18:26 -0700, Richard Cochran wrote: > On Mon, Oct 01, 2018 at 06:11:16PM +0000, Patel, Vedang wrote: > > > > On Mon, 2018-10-01 at 10:59 -0700, Richard Cochran wrote: > > > > > > On Mon, Oct 01, 2018 at 05:24:48PM +, Patel, Vedang wrote: > > > > > > > > > > > > I will add the comment for clearing the SYNC_RX_TIMER. It is > > > > basically > > > > to clear out the event returned by poll(). > > > But why? Does the original code have a bug? Please explain. > > > > > In the original code, whenever we receive a FD_SYNC_RX_TIMER > > timeout, > > it is usually accompanied by a state transition. So, > > port_*_transition() will take care of clearing the event for us (by > > calling port_clr_tmo() at the beginning). But, when BMCA == 'noop', > > there is nothing to do, so we are not clearing out the event and > > poll() > > will report the event in every loop. I am just taking care of > > clearing > > this event here. > Then you should have followed through and removed the redundant clear > in the state transition code. Or what am I missing? > We still need to clear out the timer when the device transitions out of slave state for other events like EV_FAULT_DETECTED. So, I think we still need to keep it there. Thanks, Vedang > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 6/7] Add ignore_source_id config option.
On Mon, 2018-10-01 at 21:32 +, Keller, Jacob E wrote: > > > > > -Original Message- > > From: Vedang Patel [mailto:vedang.pa...@intel.com] > > Sent: Wednesday, September 26, 2018 2:58 PM > > To: linuxptp-devel@lists.sourceforge.net > > Cc: Gomes, Vinicius ; Sanchez-Palencia, > > Jesus > > > > Subject: [Linuxptp-devel] [PATCH v2 6/7] Add ignore_source_id > > config option. > > > > This config option will skip the source port identity verification > > in > > the Sync and Follow_up messages. This option is needed when the > > announce > > messages are disabled because the slave cannot know the identity of > > master without announce messages. > > > > This is required by Automotive Profile as part of skipping the Best > > Master Clock Algorithm (BMCA). > > > Would it be better to allow configuring the expected source id > instead? > Manually configuring source ids for all slave devices is not going to be ideal in mass production. Also, the spec[1] (look at Section 6.3, point #2) specifically asks for the source id to be ignored. Thanks, Vedang [1] - https://avnu.org/wp-content/uploads/2014/05/Automotive-Ethernet-A VB-Func-Interop-Spec-v1.5-Public.pdf > Thanks, > Jake > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Mon, 2018-10-01 at 10:59 -0700, Richard Cochran wrote:
> On Mon, Oct 01, 2018 at 05:24:48PM +0000, Patel, Vedang wrote:
> >
> > I will add the comment for clearing the SYNC_RX_TIMER. It is
> > basically
> > to clear out the event returned by poll().
> But why? Does the original code have a bug? Please explain.
>
In the original code, whenever we receive a FD_SYNC_RX_TIMER timeout,
it is usually accompanied by a state transition. So,
port_*_transition() will take care of clearing the event for us (by
calling port_clr_tmo() at the beginning). But, when BMCA == 'noop',
there is nothing to do, so we are not clearing out the event and poll()
will report the event in every loop. I am just taking care of clearing
this event here.
> >
> > But, I don't understand how moving the inhibit_announce before this
> > will help.
> You avoid changing the behavior of the non-noop-BCMA code.
>
> >
> > I am not removing the hunk anywhere.
> You are right. I misread the next patch.
>
> BUT you avoid changing the original behavior by doing this:
>
> if (p->inhibit_announce) {
> port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
> port_clr_tmo(p->fda.fd[FD_SYNC_RX_TIMER]);
> } else {
> port_set_announce_tmo(p);
> }
>
The SYNC_RX_TIMER needs to be cleared out whenever BMCA == 'noop' (or
any other future state machine which does not do any state transition
in this case). I will add the condition to clear out the timer only
when BMCA == 'noop'.
Thanks,
Vedang
> See?
>
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
Hi Richard,
On Sun, 2018-09-30 at 20:02 -0700, Richard Cochran wrote:
> On Sun, Sep 30, 2018 at 08:00:51PM -0700, Richard Cochran wrote:
> >
> > On Wed, Sep 26, 2018 at 02:57:36PM -0700, Vedang Patel wrote:
> > >
> > > @@ -2472,6 +2480,7 @@ static enum fsm_event bc_event(struct port
> > > *p, int fd_index)
> > > if (p->best)
> > > fc_clear(p->best);
> > > port_set_announce_tmo(p);
> > > + port_clr_tmo(p->fda.fd[FD_SYNC_RX_TIMER]);
> > > delay_req_prune(p);
> > > if (clock_slave_only(p->clock) && p-
> > > >delayMechanism != DM_P2P &&
> > > port_renew_transport(p)) {
> > > @@ -2862,10 +2871,24 @@ struct port *port_open(int phc_index,
> > This hunk needs some kind of justification, especially since you
> > undo
> > it later in the series.
> Can you avoid this by putting the inhibit_announce patch first?
>
I will add the comment for clearing the SYNC_RX_TIMER. It is basically
to clear out the event returned by poll().
But, I don't understand how moving the inhibit_announce before this
will help. I am not removing the hunk anywhere.
Thanks,
Vedang
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 0/6] Adding support for Automotive Profile
Thanks Richard for the response. I will work on this and send you a patch soon. Just one question below. On Tue, 2018-09-18 at 06:06 -0700, Richard Cochran wrote: > diff --git a/designated_fsm.c b/designated_fsm.c > index a56f962..9ccf4c9 100644 > --- a/designated_fsm.c > +++ b/designated_fsm.c > @@ -113,14 +107,6 @@ enum port_state designated_slave_fsm(enum > port_state state, > case EV_FAULT_DETECTED: > next = PS_FAULTY; > break; > - /* > - * Go to PS_UNCALIBRATED when there is a loss of > sync messages. > - * This will also be useful in changing the Sync > Interval (to > - * be implemented). > - */ > - case EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES: > - next = PS_UNCALIBRATED; > - break; Is there any particular reason you removed this? This is needed to process the FD_SYNC_RX_TIMER expiration when the master disconnects unexpectedly. Thanks, Vedang > default: > break; > } ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v1 0/6] Adding support for Automotive Profile
Hi Richard,
Is this series somewhere in your list of "things to review"? I just
wanted to check-in and make sure you didnt miss this.
Thanks,
Vedang
On Thu, 2018-08-30 at 16:54 -0700, Vedang Patel wrote:
> Changes in this version:
> ~~~
>
> Since the series has gone through some feedback and is mature now, it
> is being
> posted as PATCH series instead of RFC.
>
> This version incorporates feedback from the last version of this
> series. The
> changes include:
> - Remove AVnu keyword from all commit messages.
> - Instead of adding special checks for BMCA == ‘noop’ in
> port_p2p_transtion, we
> now transition to PS_LISTENING before going to PS_MASTER/PS_SLAVE
> for
> designated master or slave.
> - designated_{master/slave}_fsm() functions were moved to their own
> file.
> - Minor change in check_source_identity function.
> - In all the series adds functionality from Sections 6.2.1.1 and
> Section 6.3
> (pts #1 and #2) of the Automotive Profile[1].
>
> More information of intention of this and future series at:
> https://sourceforge.net/p/linuxptp/mailman/message/36369408/
>
> Thanks,
> Vedang Patel
>
> [1] Automotive Ethernet AVB Functional and Interoperability
> Specification -
> http://avnu.org/wp-content/uploads/2014/05/
> Automotive-Ethernet-AVB-Func-Interop-Spec-v1.5-Public.pdf
>
> Vedang Patel (6):
> port: Add condition to check fc.
> clock: Add NULL check for best clock in clock_update_slave()
> Add BMCA config option.
> Add inhibit_announce config option.
> Add ignore_source_id config option.
> Add example configuration for Automotive Profile.
>
> bmc.c | 11
> clock.c | 24 +++-
> config.c | 9 +++
> configs/automotive-master.cfg | 27 +
> configs/automotive-slave.cfg | 30 ++
> configs/default.cfg | 3 +
> designated_fsm.c | 133
> ++
> designated_fsm.h | 43 ++
> fsm.h | 6 +-
> makefile | 11 ++--
> port.c| 63
> port.h| 8 +++
> port_private.h| 3 +
> ptp4l.8 | 31 --
> 14 files changed, 381 insertions(+), 21 deletions(-)
> create mode 100644 configs/automotive-master.cfg
> create mode 100644 configs/automotive-slave.cfg
> create mode 100644 designated_fsm.c
> create mode 100644 designated_fsm.h
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 0/5] Adding support for AVnu Automotive Profile
On Wed, 2018-08-22 at 23:47 +, 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
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
--
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 0/5] Adding support for AVnu Automotive Profile
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 -- 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 0/5] Adding support for AVnu Automotive Profile
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
--
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 5/5] Add example configuration for AVnu Automotive Profile.
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
--
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 2/5] Add BMCA config option.
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
--
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 4/5] Add ignore_source_id config option.
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(,
> > >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(, >header.sourcePortIdentity)) {
> return -1;
> }
> return 0;
>
> or even:
>
> if (p->ignore_source_id) {
> return 0;
> }
> master = clock_parent_identity(p->clock);
> return pid_eq(, >header.sourcePortIdentity) ? 0 : -1;
>
Will include the second version in v3.
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 0/5] Adding support for AVnu Automotive Profile
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
>
--
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 0/5] Adding support for AVnu Automotive Profile
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 -- 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC v2 0/5] Adding support for AVnu Automotive Profile
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 -- 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC 1/1] Add avnu_ap to enable AVnu Automotive Profile
On Wed, 2018-07-25 at 17:37 -0700, Richard Cochran wrote: > On Wed, Jul 25, 2018 at 05:13:03PM +0000, Patel, Vedang wrote: > > > > In the description above, you mentioned that we should use > > masterOnly > > and slaveOnly. But, from the pseudo code, it seems like you just > > want > > to make decision based only on slaveOnly option. Which one do you > > prefer? > The code should require that one of these is set when BMCA == noop. > > > > > If we are just using a single option, I would prefer masterOnly > > since > > it is a per-port variable and extending support to bridges will be > > easy. > How about this? Leave slaveOnly global. When BMCA == noop then > check > port option masterOnly, then global slaveOnly, else fail. > > Later on, a bridge that has both port types can set slaveOnly as the > global default and configure masterOnly per port as needed. > Thanks for the inputs. I will send out the updated patches soon. -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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC 1/1] Add avnu_ap to enable AVnu Automotive Profile
Thanks Richard for the inputs.
On Tue, 2018-07-24 at 19:55 -0700, Richard Cochran wrote:
> On Tue, Jul 24, 2018 at 05:38:32PM +0000, Patel, Vedang wrote:
> >
> > So, following is what we think will do the job:
> > - Add inhibit_announce option to stop announce messages as well as
> > timeouts. (default: 0 - disabled)
> Sounds okay.
>
> >
> > - Add source_port_identity_check option to disable checking for
> > source
> > port identities in sync and follow-up messages (default: 1 -
> > enabled)
> Or keeping with the negative tone, "ignore_source_id". (Ignore and
> inhibit sound good together.)
>
yeah ignore_source_id seems better. will use that.
> >
> > - Add static_roles config which is a tristate - master, slave and
> > disabled. When master or slave roles are selected, the FSM will
> > directly assign those roles. (default: 'disabled')
> We already have slaveOnly and masterOnly. All you need is a new
> option for your different BCMA, say "BCMA = noop".
>
> Then provide two functions for p->state_machine, something like the
> following pseudo code.
>
> enum port_state designated_master_fsm() {
> return PS_GRAND_MASTER;
> }
> enum port_state designated_slave_fsm() {
> return PS_SLAVE;
> }
>
> port_open() {
> ...
> if (BMCA == noop) {
> p->state_machine = clock_slave_only(clock) ?
> designated_slave_fsm : designated_master_fsm;
> }
> }
>
In the description above, you mentioned that we should use masterOnly
and slaveOnly. But, from the pseudo code, it seems like you just want
to make decision based only on slaveOnly option. Which one do you
prefer?
In my opinion, we should use both the options to remove any ambiguity.
This would mean, if we decide to support brides (in the future), we
will have to change slaveOnly from global to per-port config option.
If we are just using a single option, I would prefer masterOnly since
it is a per-port variable and extending support to bridges will be
easy.
> >
> > We also looked into the slaveOnly FSM for slave. But, we cannot use
> > that because it still expects the announce message from the master
> > before transitioning to UNCALIBRATED state.
> You can surely use the slaveOnly option, but of course you wouldn't
> use the ptp_slave_fsm function itself.
>
> 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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC 1/1] Add avnu_ap to enable AVnu Automotive Profile
On Thu, 2018-07-19 at 21:21 +, Patel, Vedang wrote: > 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. So, following is what we think will do the job: - Add inhibit_announce option to stop announce messages as well as timeouts. (default: 0 - disabled) - Add source_port_identity_check option to disable checking for source port identities in sync and follow-up messages (default: 1 - enabled) - Add static_roles config which is a tristate - master, slave and disabled. When master or slave roles are selected, the FSM will directly assign those roles. (default: 'disabled') Following are the configs needed for each role: master: static_role = 'master', inhibit_announce = 1 slave: static_role = 'slave', source_port_identity_check = 0 We also looked into the slaveOnly FSM for slave. But, we cannot use that because it still expects the announce message from the master before transitioning to UNCALIBRATED state. We also looked into using masterOnly option. But, it will wait for announce receipt timeout before transitioning to MASTER. But, inhibit_announce will disable that. Let us know if this meets your expectations. Thanks, Vedang Patel -- 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC 1/1] Add avnu_ap to enable AVnu Automotive Profile
On Thu, 2018-07-19 at 10:19 -0700, Richard Cochran wrote: > BTW, that CC: Patel is not an address to which we can reply. Please > remove it from future mails. > Looks like a weird setting in my git config. Will fix it. 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC 1/1] Add avnu_ap to enable AVnu Automotive Profile
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC 0/1] Adding support for AVnu Automotive Profile
On Thu, 2018-07-19 at 08:08 -0700, Richard Cochran wrote: > On Mon, Jul 16, 2018 at 02:34:46PM -0700, Vedang Patel wrote: > > > > - Switching Sync message interval and pdelay message intervals to a > > lower value > > once system stabilizes (Sections 6.2.3.1 and 6.2.3.2 in [1]). > This is an interesting option to have, all on its own. > A very basic question: What is the best way for master to determine the system has stabilized and change the sync interval? In the original plan, the slave would know and then it would send a signaling message to increase the interval. > > > > This will also > > add support for Message Interval request TLV (Section 10.5.4 in > > [2]). > This is a complete waste of time, IMHO. The whole idea of this > "profile" is that everything is statically configured. Why on earth > should slaves then adjust the intervals later on? > > Nobody needs this. > > > > > - Option to save some properties like neighborRateRatio when the > > program exits. > > (Sections 6.2.2.2 and 6.2.2.3 in [1]) > No need to save on exit. If you like, script pmc to do this for you. > However, a pre-seeded configuration option makes sense to add. > Ok will add a config option for the rate ratios. > > > > - Option to continue adjusting the clock even when slave does not > > receive Sync > > messages from master. (Section 6.3 bullet 4 sub bullet 6 -- lines > > 228 to 230) > Please explain exactly what you have in mind. > Here, I was just planning to adjust the clock such that it keeps on advancing based on last known value of cumulativeScaledRateOffset. But, after looking a bit more into the code, it looks like this might already be done. Need to look at the code a little more closely to know for sure. 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC 1/1] Add avnu_ap to enable AVnu Automotive Profile
On Mon, 2018-07-16 at 16:20 -0700, Cliff Spradlin wrote: > I don't like the complexity this overlays into ptp4l's state machine. > > Could you explain why you'd want to use this instead of a more > standard profile? Having a grandmaster send an announce message > doesn't seem like a significant burden to an automotive ethernet > network, and ptp4l already supports a slave-only mode. > To take advantage of the quicker startup time the profile specifies. If we look at table 6 in Section 5.5 of the spec[1], grand master is expected to start sending Sync/Follow-Up messages within 250ms. Without skipping BMCA, I don't see it happening. If slaveOnly is used, the device will still participate in BMCA. The Announce messages won't be processed when BMCA is not being run. So, there is no need to send those messages. There will be more features added later on which will further optimize linuxptp to run more efficiently on the automotive network. I have described all of the future enhancements in the cover letter: https://sourceforge.net/p/linuxptp/mailman/message/36369408/ > I don't think that an automotive network is sufficiently different > from an industrial network to necessitate a special profile, > especially one that changes core behavior of the protocol. > > [1] - http://avnu.org/wp-content/uploads/2014/05/Automotive-Ethernet-AV B-Func-Interop-Spec-v1.5-Public.pdf -- 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] linuxptp testing
On Tue, 2018-07-10 at 09:47 -0700, Patel, Vedang wrote: > On Mon, 2018-07-09 at 18:33 -0700, Richard Cochran wrote: > > > > On Mon, Jul 09, 2018 at 09:47:23PM +, Patel, Vedang wrote: > > > > > > > > > Also, are there any tests tests which ensure conformance to > > > specs. > > > (1588, 802.1AS and others) > > There is no conformance test specified for 1588. > > > > There is one for 802.1AS. Contact the avnu people for details. > > > Thanks Richard for the information. > > What are the tests you usually run before you release a new version > of > linuxptp? > > Is the a place where we can submit tests for linuxptp for any new > feature? > I guess linuxptp-testsuite (https://github.com/mlichvar/linuxptp-testsu ite) mentioned in my earlier email is one option. But, is it the one which everyone uses? > Thanks, > Vedang > > > > HTH, > > 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] linuxptp testing
On Mon, 2018-07-09 at 18:33 -0700, Richard Cochran wrote: > On Mon, Jul 09, 2018 at 09:47:23PM +0000, Patel, Vedang wrote: > > > > Also, are there any tests tests which ensure conformance to specs. > > (1588, 802.1AS and others) > There is no conformance test specified for 1588. > > There is one for 802.1AS. Contact the avnu people for details. > Thanks Richard for the information. What are the tests you usually run before you release a new version of linuxptp? Is the a place where we can submit tests for linuxptp for any new feature? Thanks, Vedang > HTH, > 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] linuxptp testing
Hi, I am working in the Real-time Linux team at Intel. I was interested in knowing about the tests which are usually run in linuxptp ensure there are no regressions or performance degradation. Also, are there any tests tests which ensure conformance to specs. (1588, 802.1AS and others) I tried searching for some discussion related to this in the archives. I found: https://github.com/mlichvar/linuxptp-testsuite Is this the one which is usually run? Or is there something else I am missing? Thanks, Vedang -- 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 Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
