0

I wonder if there's a smarter way to get these 4 consecutive bytes from a buffer, concatenated in a unsigned long.

// the timestamp starts at byte 40 of the received packet and is four bytes,
// or two words, long. First, extract the two words:

unsigned long highWord = word(packetBuffer[40], packetBuffer[41]);
unsigned long lowWord = word(packetBuffer[42], packetBuffer[43]);
// combine the four bytes (two words) into a long integer
// this is NTP time (seconds since Jan 1 1900):
unsigned long secsSince1900 = highWord << 16 | lowWord;
Serial.print("Seconds since Jan 1 1900 = ");
Serial.println(secsSince1900);

Seems overkill to me, thinking these 4 bytes are already consecutive in the buffer.

Result should be unchanged i.e. having the value in a variable.

  • 1
    It depends if they are in the same endian order (i.e., little-endian) as the Arduino's long type. Judging by your current method they aren't. – Majenko May 19 '18 at 11:57
3

If they were in the same endian order as a long (little-endian on an Arduino, which they don't appear to be) you could just point to them:

uint32_t *l = (uint32_t *)&packetBuffer[40];

Then use *l to access the content as a long.

However since they look to be in a big-endian order it's not that simple. The simplest thing you can do is just re-combine them with bit-shifting:

uint32_t l = ((uint32_t)packetBuffer[40] << 24) | ((uint32_t)packetBuffer[41] << 16) | 
             ((uint32_t)packetBuffer[42] << 8) | (uint32_t)packetBuffer[43];
  • If I add the last line of your code then do Serial.println(secsSince1900, HEX); Serial.println(secsSince1900, HEX); I get DEAAC142 and FFFFC142. Any clue? – neurino May 19 '18 at 15:33
  • Actually, on a little 8-bitter you may need to cast the values to 32 bit before bitshifting to prevent clipping. Been a while since I used little chips like these. – Majenko May 19 '18 at 15:38
  • Indeed. If both int and the type of packetBuffer[40] are less than 32 bits, then packetBuffer[40] << 24 invokes undefined behavior. – Edgar Bonet May 19 '18 at 15:48
  • Thanks for your answers, you helped me finding what was wrong in my code. – neurino May 19 '18 at 16:05
1

An alternative is to use union and struct, and let the compiler do the work:

static inline uint32_t bswap32(uint32_t x)
{
        union {
                uint32_t x;
                struct {
                        uint8_t a;
                        uint8_t b;
                        uint8_t c;
                        uint8_t d;
                } s;
        } in, out;
        in.x = x;
        out.s.a = in.s.d;
        out.s.b = in.s.c;
        out.s.c = in.s.b;
        out.s.d = in.s.a;
        return out.x;
} 

uint32_t sinceEpoch = bswap32(*(uint32_t*) &packetBuffer[40]);

Ref. http://lists.nongnu.org/archive/html/avr-gcc-list/2006-12/msg00076.html

A cleaner rewrite for the packet buffer would be:

static inline uint32_t bswap32(uint8_t* buf)
{
        union {
                uint32_t x;
                struct {
                        uint8_t a;
                        uint8_t b;
                        uint8_t c;
                        uint8_t d;
                } s;
        } in, out;
        out.s.a = buf[3];
        out.s.b = buf[2];
        out.s.c = buf[1];
        out.s.d = buf[0];
        return out.x;
} 

uint32_t sinceEpoch = bswap32(&packetBuffer[40]);

Here is an AVR machine code version:

inline uint32_t bswap32(uint32_t value)
{
  asm volatile("mov __tmp_reg__, %A0"   "\n\t"
           "mov %A0, %D0"       "\n\t"
           "mov %D0, __tmp_reg__"   "\n\t"
           "mov __tmp_reg__, %B0"   "\n\t"
           "mov %B0, %C0"       "\n\t"
           "mov %C0, __tmp_reg__"   "\n\t"
           : "=r" (value)
           : "0" (value)
           );
  return (value);
}

Ref. Cosa/Types.h, https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/Cosa/Types.h#L553, and Cosa-NTP/NTP.cpp, https://github.com/mikaelpatel/Cosa-NTP/blob/master/NTP.cpp#L56

0

I ended up doing this:

unsigned long ntp_time = 0;
for (byte i = 0; i < 4; i++){
  ntp_time = ntp_time << 8 | (byte) packetBuffer[40 + i];
}

This is what I would have used on a high level language and my first attempt before asking — which failed because it lacked casting to byte.

I still have to understand why it failed, being packetBuffer declared as

const byte *packetBuffer

which to me seems addressing bytes already...

Thanks to Majenko for help, his solution still cast all buffer bytes to 4 bytes, I prefer mine that keeps down to bytes.

  • The cast to byte is useless. If it failed without the cast, that must be because of some unrelated issue that you fixed around the same time you added the cast. That's the kind of situation when version tracking software can help. – Edgar Bonet May 19 '18 at 16:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.