UDP gives wrong NTP time stamp.

From time to time the code to get a NTP timestamp from the router returns the wrong number. It is anywhere between roughly 13 and 52 years out! The code I'm using is a slight modification of the standard UTP packet that is in the Arduino Cookbook. I've found that using 'union' saves a few bytes and I'm working to a 0Hr 1st Jan 2000 base.

unsigned long GetNTP(byte NTP_Address[4]) 
{
  union                            //Define union of unsigned long ntpEpoch
  {                                //to get time stamp from bigendian
    unsigned long value;           //bytes in the returned buffer
    byte element[4];
  }ntpEpoch;
  ntpEpoch.value = 0;              // set it to zero to clear random values.
  
  const byte ntp_BUFFER_LEN = 48;
  byte ntp_Buffer[ntp_BUFFER_LEN];
  // set all bytes in the inputBuffer to 0
  memset(ntp_Buffer, 0, ntp_BUFFER_LEN);
    
  // Initialize values needed to form NTP request
  // (see URL above for details on the packets)
   ntp_Buffer[0] = 0b11100011;   // LI, Version, Mode
   ntp_Buffer[1] = 0;            // Stratum, or type of clock
   ntp_Buffer[2] = 6;            // Polling Interval
   ntp_Buffer[3] = 0xEC;         // Peer Clock Precision
    // 8 bytes of zero for Root Delay & Root Dispersion
   ntp_Buffer[12]  = 49; 
   ntp_Buffer[13]  = 0x4E;
   ntp_Buffer[14]  = 49;
   ntp_Buffer[15]  = 52;
    
    // all NTP fields have been given values, now
    // you can send a packet requesting a timestamp
    Udp.begin(localPort);
    Udp.beginPacket(NTP_Address, 123); //NTP requests are to port 123
    Udp.write(ntp_Buffer,ntp_BUFFER_LEN);
    Udp.endPacket();
    delay(2000); //could do with being bigger but may cause problem with 'one second interupt'
    if (Udp.parsePacket()) {
      // We've received a packet, read the data from it
      Udp.read(ntp_Buffer,ntp_BUFFER_LEN);  // read the packet into the buffer
      //the timestamp starts at byte 40 of the received packet and is four bytes,
      //long higest byte first combine the four bytes into a long integer
      
      ntpEpoch.element[0] = ntp_Buffer[43];
      ntpEpoch.element[1] = ntp_Buffer[42];
      ntpEpoch.element[2] = ntp_Buffer[41];
      ntpEpoch.element[3] = ntp_Buffer[40];

      // this is NTP time (seconds since Jan 1 1900):
      // now convert NTP time into everyday time:
      // Unix time starts on Jan 1 1970. In seconds, that's seconds 2208988800 later:
      // but we are working on a 2000 base so subtract 100 years:
      // 2000 is 3,155,673,600UL from Jan 1900:

      
     ntpEpoch.value = ntpEpoch.value - 3155673600UL;
                                                     
    }  //end 'If ( Udp.parsePacket() )' if we didnt have a time ntpEpoch.value is still zero
  delay(1); 
  Udp.stop();
  return ntpEpoch.value;
  }

I've checked that the error is not a shift left/right of the required bytes and it seem to be completely random.

Given that the process is loosing something I'm wondering if it would be worthwhile to load the package with the local NTP value before writing the packet out to UDP. I could then extract T2 and T3 from the returned information as well as T4, check how big the 'offset' and 'delays' are. Then react accordingly.

Any thoughts on checking that UDP.endPacket returns 1 to note that it was sent properly or that UDP.paresPacket returns the correct size of the reply at 48 bytes

solar_eta:
Any thoughts on checking that UDP.endPacket returns 1 to note that it was sent properly or that UDP.paresPacket returns the correct size of the reply at 48 bytes

that could not hurt and would be a first hint.

What board are you using? The ESP8286 core comes with an NTP Client example.

as a side note, careful with the way you use types

  union                            //Define union of unsigned long ntpEpoch
  {                                //to get time stamp from bigendian
    unsigned long value;           //bytes in the returned buffer
    byte element[4];
  }ntpEpoch;

an unsigned long could be 64 bits on a 64 bit architecture. as you want to enforce 4 bytes, best would be to use

  union                            //Define union of unsigned long ntpEpoch
  {                                //to get time stamp from bigendian
    uint32_t value;           //bytes in the returned buffer
    uint8_t element[4];
  }ntpEpoch;

and of course there are endless debate about using union this way, it works 'most of the time' until it does not due to compiler adding padding, reorganizing things etc.

if you were to refer to uint8_t* epochPtr = ((uint8_t*) & ntpEpoch), that would be the first byte in memory and you could do assign values using the pointer. No need for union.

Sorry I should have said I'm using a Leonardo with a W5100 ethernet shield.

Its highly unlikely that this code will be ported to a 64bit platform but I take the note of caution about the compiler thinking it needs to put some padding in the structure. I need to do some Ram crawling just to make sure that the compilers not making a union of integers rather than bytes. I don't think it is because most of the time it gives the right answer