Bitshifting bytes to form a long var fails!

Projects Programming
41 
            addr.nybble.a1 = e;

addr.nybble.a2 = d;
            addr.nybble.b1 = c;
            addr.nybble.b2 = b;
            addr.nybble.c1 = a;
            addr.nybble.c2 = 0x0;
            addr.nybble.d1 = 0x0;
            addr.nybble.d2 = 0x0; // */

How is all this defined? nybble for example?

Use code tags please, not quotes. Post the full code (something that compiles), and post your output.

I think I've mentioned twice already that saying "it doesn't work" is not helpful. Post your output and explain what you expect to see, and how it differs from what you actually see.

42 
byte str4 = (Start & 0xF000) >> 12;

Not very efficient:

byte str4 = (Start >> 12) & 0x0f;

address = long (a) << 16 | long (b) << 12 | c << 8 | d << 4 | e;

Try this:

address = ((unsigned long) a << 16) | ((unsigned long) b << 12) | (c << 8) | (d << 4) | e;

assuming a/b/../e are bytes.

43

To nick gammon

union ByteToLong
{
    struct
    {
      byte a1: 4;
      byte a2: 4;
      byte b1: 4;
      byte b2: 4;
      byte c1: 4;
      byte c2: 4;
      byte d1: 4;
      byte d2: 4;
    } nybble;
    unsigned long whole;
};

The wrong output is

8000
8010

The correct is

8000
8002
8004
8006
8008
8010

If you put the code snippet that i posted in a sketch and call it:

ReadData(8000, 8010);

you will get the output!

I will try the mods from dhenry!

44

Ok here is a working code without unions and stuff. Corrected some things and it works as it should:

void ReadData(long Start, long Stop) {
  char result[3];
  word counter = 0;
  long address;
  byte str5;
  
  union ByteToLong addr;
  
  byte str1 = (Start & 0x0F);
  byte str2 = (Start & 0xF0) >> 4;
  byte str3 = (Start & 0xF00) >> 8;
  //byte str4 = (Start & 0xF000) >> 12;
  byte str4 = (Start >> 12) & 0x0F;
  if (Start & 0xF0000) {
   //str5 = (Start & 0xF0000) >> 16;
   str5 = (Start >> 16) & 0x0F;
  }
  else {
    str5 = 0x00;
  }
  
   for (byte a = str5; a <= 0x0F; a++) {
    for (byte b = str4; b <= 0x0F; b++) {
      for (byte c = str3; c <= 0x0F; c++) {
        for (byte d = str2; d <= 0x0F; d++) {
          for (byte e = str1; e <= 0x0F; e+=2) {
            counter++;            
            address = ((unsigned long) a << 16) | ((unsigned long) b << 12) | (c << 8) | (d << 4) | e;
            //sprintf(result, "%04X",(char*) GetWord(address));
            print_hex(GetWord(address), 16);
            if (counter == ((Stop - Start)/2)) goto out;
          }
          Serial.println();
        }
      }
    }
  }
out:
  Serial.println();
  return;
}

The only problem is the leading "0" in hex numbers. I read words and where there is a 0 like a 081F it displays like 81F! Any work around this? print_hex() is really slow on that! Something faster would be great!

Thanx guys

45

If you want to reconstitute a long type from char types, here is something that you can try beyond all the shifting / union.

unsigned char *uc_ptr; //unsigned char ptr, used for conversion 

...
  uc_ptr = (unsigned char *) &address; //point uc_ptr at the conversion results
  *uc_ptr++=dat1; //assign the 1st char
  *uc_ptr++=dat2; //assign the 2nd char
  *uc_ptr++=dat3; //assign the 3rd char
  *uc_ptr   =dat4; //assign the 4th char
  //after this point, address should contain the values represented by the four char types

Speed-wise, this will be as fast as the union approach and potentially faster. However, like the union approach, it is dependent on the compiler's endianess - so the code's portability is poor.

46

print_hex() is really slow on that! Something faster would be great!

The whole piece doesn't make a lot of sense to me, with all the left shifting + right shifting involved. If you can articulate what you are trying to do, people may be able to better help you.

47

psyche:
I will try the mods from dhenry!

Oh yes? And what stopped you trying my suggestion in reply #15?

  address = long (a) << 16 | long (b) << 12 | c << 8 | d << 4 | e;

We are up to reply #43 and you act all surprised that a suggestion in reply #41 works. When (apart from the "unsigned" part) that was suggested in reply #15.

48

There is no reason for me to lie to anyone here! I tried your suggestion but it didn't work. I dont know the reason why, maybe it is the bitshifting parts or a bug in Arduino IDE. 99% of the errors are made from humans, i wrote some code which may had some mistakes BUT Arduino IDE is made by humans also! I didn't blame anyone and yes i was surprised that it did work! I don't know exactly who acts weird here but it is not me! I tried all the suggestions and two of them work! I don't want to offend anyone and i dont intend too!
Who knows, maybe it was a combination of mistakes that i did, which led to a non working code when you replied! I really can't know!
What i know is that when dhenry suggested me to change a few things, the code worked, thats all! If someone has a problem with dhenry's replies or anyone else's replies, itsnot me and i dont care who does! What i care is that i came here asking for help and i got it! I know who helped me and who didn't. The rest is history alreay!

Keep up the good work guys
Thank you for your help!

49

Who knows, maybe it was a combination of mistakes that i did

@psyche good to see you have it working, the previous examples were vaild.

And just to be picky:

dhenry:
If you want to reconstitute a long type from char types, here is something that you can try beyond all the shifting / union.

unsigned char *uc_ptr; //unsigned char ptr, used for conversion

...
 uc_ptr = (unsigned char *) &address; //point uc_ptr at the conversion results
 *uc_ptr++=dat1; //assign the 1st char
 *uc_ptr++=dat2; //assign the 2nd char
 *uc_ptr++=dat3; //assign the 3rd char
 *uc_ptr   =dat4; //assign the 4th char
 //after this point, address should contain the values represented by the four char types



Speed-wise, this will be as fast as the union approach and potentially faster. However, like the union approach, it is dependent on the compiler's endianess - so the code's portability is poor.

@dhnery, I hope there is more to come, most of your assumptions above are wrong. The task at hand is interlacing nibbles, more specifically 'un-serialisation' of data. The algorithm above is not intended for re-factoring and is almost guaranteed to be slower. One rule of thumb when designing an algorithm is assignments can be 'equal to' in speed compared to initialisations but never faster.

The algorithm above is typically used as a replacement for a loop, or used in conjunction with one, to trade off size for greater speed.

Also on a side note, most compilers suffer a small side effect relating to using a 'pointer to' or 'reference of' constant data, as a result these constants cannot be considered 'compile time constants'.

As for comparing the above to my union in reply #30, just counting the operations shows it does less work before the compiler even gets to it.

Moving on to the versions in replies 15, 41, they are not quite as fast as the union due to their shifts greater than the native processor width, however they are faster than the above example too.
I haven't considered the union here as its use of bit fields is very inefficient.
And as for them being non-portable, there is no reason to not include endianess handling into the struct which would give you portablility.

Hope this gives some thoughts.
Cheers.

50

The task at hand is interlacing nibbles, more specifically 'un-serialisation' of data.

To help you see it more explicitly:

#define n2uc(high_nibble, low_nibble)  (((high_nibble) & 0xf0) | ((low_nibble) & 0x0f))

...
  uc_ptr = (unsigned char *) &address;
  *uc_ptr++ = n2uc(d, e);
  *uc_ptr++ = n2uc(b, c);
  *uc_ptr   = n2uc(0, a);
  //done

You can find out which is faster.

51

That won't do you justice.

For starters you don't write anything to the most significant byte. A variable declared on the stack will have an undefined contents until initialised or assigned. Therefore you read rubbish data off the stack.

Secondly you don't combine the data properly, you mask away the bits of 'high_nibble'

And no, apart from the fact the code is unusable, it is not faster.

This is probably what you intended.

  uc_ptr = (unsigned char *) &address;
  *uc_ptr++ = (( d << 4 ) | e);
  *uc_ptr++ = (( b << 4 ) | c);
  *uc_ptr++ = a;
  *uc_ptr = 0;

Which also is slower than a union construct.