gPTP: Processing Sync/Announce Receipt Timeouts

[vvacharya]

gPTP: Processing Sync/Announce Receipt Timeouts

In one of the pull requests that were recently merged from open-avb-next (https://github.com/AVnu/OpenAvnu/pull/585), I noticed a discrepancy in how we handle Sync and Announce Receipt Timeouts.

In the IEEE1588Port class, the timeouts were handled as a case-block in the processEvent() method. In the new CommonPort class, it has been moved to a new method called processSyncAnnounceTimeout(). However, the code in this method seems the logical inverse when compared to the above code. I was wondering if this is a bug, or is this being handled elsewhere and I am missing something.

In ieee1588port.cpp, line 790:

IEEE1588Port::processEvent( Event e )
{
   ...
   case ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES:
   case SYNC_RECEIPT_TIMEOUT_EXPIRES:
   {
      ....

      if( clock->getPriority1() == 255 ) {
         // Restart timer for Sync or Announce Timeout
      }

      if (port_state == PTP_INITIALIZING
         || port_state == PTP_UNCALIBRATED
         || port_state == PTP_SLAVE
         || port_state == PTP_PRE_MASTER) {
         {
            // We are the master, so set ClockIdentity
         }
         // Set port state to PTP_MASTER etc.

         if( clock->getPriority1() != 255) {
            clock->addEventTimerLocked
            ( this, SYNC_INTERVAL_TIMEOUT_EXPIRES, 16000000 );
         }
         ...

      }
      ...

   }
   break;
}

In common_port.cpp, line 468

bool CommonPort::processSyncAnnounceTimeout( Event e )
{
   if( clock->getPriority1() != 255 )
      return true;

   // Restart Timer for Sync or Announce Timeout

   if ( getPortState() == PTP_INITIALIZING ||
      getPortState() == PTP_UNCALIBRATED ||
      getPortState() == PTP_SLAVE ||
      getPortState() == PTP_PRE_MASTER )
   {
      {
         // We are the master, so set ClockIdentity
      }

      // Set port state to PTP_MASTER etc.

      ...
      if( clock->getPriority1() != 255) {
         clock->addEventTimerLocked
         ( this, SYNC_INTERVAL_TIMEOUT_EXPIRES, 16000000 );
      }
   }
   return true;
}

[pinealservo]

The early exit if Priority1 != 255 does look a bit wrong, doesn't it? I think overall logic could use a good think-through with respect to the spec about what non-GM-capable endpoints are supposed to do, but this definitely looks like an issue.

@christopher-s-hall Is that what you meant it to be? It seems like we're only handling a timeout waiting for an Announce or Sync if we are not GM-capable now.

[andrew-elder]

Yes, something doesn't seem quite right. At a minimum comments need to be added since it isn't obvious how this is working.

[christopher-s-hall]

It's my copy-paste error. It should be == not != in the compare. The logic is: if in slave only (P1 = 255) mode and there's a timeout waiting for a sync or announce do nothing and don't restart the timer because P1 == 255 means GM "incapable". There's no way to switch to the master role. The rest of the code is also a copy/paste, but I think could be simplified as well. The logic might be worth reviewing as well. I'll send out an RFC patch later today.

[andrew-elder]

@christopher-s-hall - any updates?