how to pack mix of (up to 4) uint_8t and int8_t into one uint32_t?

Question

how to pack mix of (up to 4) uint_8t and int8_t into one uint32_t?

How to write set of macros to pack mix of signed and unsigned byte values (max 4 of them) to one 32-bit variable

Motivation:

I have (third party) library, which offers function with uint32_t agument, which defacto contains 1-4 bytes, each of them can be signed ot unsigned, depending on some other circumstates. Internally it is decomposed to 4 bytes, then transmited somewhere elese, where the bytes are read and interpreted as signed or unsigned and used.

I have a lot of code, which would call this function, but it could be more readable, if I could write some simple macro/macros to pack the right number of values into one and call that function in one line.

the prototype of the function is:

void get_data(int addr,uint32_t cmd,uint8_t cmdlen,uint8_t *data,uint8_t max_len)

and I would like to have shortcuts like this:

#define GET_DATA1(cmd1,var,len) get_data(this->i2c,PACK1(cmd1),(uint8_t *)&(var),len)
#define GET_DATA2(cmd1,cmd2,var,len) get_data(this->i2c,PACK2(cmd1,cmd2),(uint8_t *)&(var),len)
#define GET_DATA3(cmd1,cmd2,cmd3,var,len) get_data(this->i2c,PACK3(cmd1,cmd2,cmd3),(uint8_t *)&(var),len)
#define GET_DATA4(cmd1,cmd2,cmd3,cmd4,var,len) get_data(this->i2c,PACK4(cmd1,cmd2,cmd3,cmd4),(uint8_t *)&(var),len)

to call it as follows:

uint8_t status
uint8_t item=130;
char name[5];
int8_t speed = -20;

GET_DATA1(READ_STATUS,status,1); // reads 1 byte status
GET_DATA2(READ_ITEM_NAME,item,name,5); // read 5 chars long name of item 130
GET_DATA3(SET_SPEED_LIMIT,30,status,1); // set maximum speed to 30 (globally) and read 1 byte status
GET_DATA4(SET_ITEM_SPEED,item,speed,status,1); // set speed of unit 130 to -20 (reversal) and read 1 byte status
// and so on for different combination of values and commands and such

where the PACKx macros are now like that:

#define PACK1(cmd1) (cmd1),1
#define PACK2(cmd1,cmd2) ((cmd1<<8)|cmd2),2
#define PACK3(cmd1,cmd2,cmd3) ((cmd1<<16)|(cmd2<<8)|cmd3),3
#define PACK4(cmd1,cmd2,cmd3,cmd4) ((cmd1<<24)|(cmd2<<16)|(cmd3<<8)|cmd4),4

but it does not work as I want.

1) it takes only last 2 params (probabelly because some casting to 16bits variable somewhere)

2) it works wrong for signed variables (maybe making them longer somewhere along the way)

I get lots of warnings like this one:

PACK.ino:5:55: warning: left shift count >= width of type
#define PACK4(cmd1,cmd2,cmd3,cmd4) ((cmd1<<24)|(cmd2<<16)|(cmd3<<8)|cmd4)

Example code:

#define PACK1(cmd1) (cmd1)
#define PACK2(cmd1,cmd2) ((cmd1<<8)|cmd2)
#define PACK3(cmd1,cmd2,cmd3) ((cmd1<<16)|(cmd2<<8)|cmd3)
#define PACK4(cmd1,cmd2,cmd3,cmd4) ((cmd1<<24)|(cmd2<<16)|(cmd3<<8)|cmd4)

#define ww(a) Serial.write(a);
#define pp(a,b) Serial.print(a,b);

void hh(uint32_t x){
    uint8_t v;
    ww("     ( 0x");
    for (int i=3;i>=0;i--) { v= (x>>(8*i))& 0xFF;if(v<16) ww("0");pp(v,HEX);};
    ww(" )");
}
#define TEST1(cmd1)         ww("\r\n"); ww(" 0x");pp(cmd1,HEX); ww(" = 0x");pp(PACK1(cmd1),HEX);    cmd=PACK1(cmd1);    ww(" => 0x");pp(cmd,HEX);   hh(cmd);
#define TEST2(cmd1,cmd2)        ww("\r\n"); ww(" 0x");pp(cmd1,HEX);ww(" 0x");pp(cmd2,HEX);  ww(" = 0x");pp(PACK2(cmd1,cmd2),HEX);   cmd=PACK2(cmd1,cmd2);   ww(" => 0x");pp(cmd,HEX);   hh(cmd);
#define TEST3(cmd1,cmd2,cmd3)       ww("\r\n"); ww(" 0x");pp(cmd1,HEX);ww(" 0x");pp(cmd2,HEX);ww(" 0x");pp(cmd3,HEX);   ww(" = 0x");pp(PACK3(cmd1,cmd2,cmd3),HEX);  cmd=PACK3(cmd1,cmd2,cmd3);  ww(" => 0x");pp(cmd,HEX);   hh(cmd);
#define TEST4(cmd1,cmd2,cmd3,cmd4)  ww("\r\n"); ww(" 0x");pp(cmd1,HEX);ww(" 0x");pp(cmd2,HEX);ww(" 0x");pp(cmd3,HEX); ww(" 0x");pp(cmd4,HEX);   ww(" = 0x");pp(PACK4(cmd1,cmd2,cmd3,cmd4),HEX); cmd=PACK4(cmd1,cmd2,cmd3,cmd4); ww(" => 0x");pp(cmd,HEX);   hh(cmd);

uint32_t cmd;

uint8_t u1,u2,u3,u4;
int8_t i1,i2,i3,i4;


void setup()
{
    Serial.begin(115200);           // start serial for output
    u1=0x12;
    u2=0x34;
    u3=0x56;
    u4=0x78;

    i1=1;
    i2=-1;
    i3=8;
    i4=-8;

    TEST1(u1)
    TEST2(u1,u2)
    TEST3(u1,u2,u3)
    TEST4(u1,u2,u3,u4)
    ww("\r\n=========================================================");
    TEST1(i1)
    TEST2(i1,i2)
    TEST3(i1,i2,i3)
    TEST4(i1,i2,i3,i4)
    ww("\r\n=========================================================");
    TEST4(u1,u2,u3,u4)
    TEST4(u1,u2,u3,u4)
    TEST4(u1,u2,u3,u4)
    TEST4(u1,u2,u3,u4)
    ww("\r\n=========================================================");
    TEST4(i1,u2,u3,u4)
    TEST4(u1,i2,u3,u4)
    TEST4(u1,u2,i3,u4)
    TEST4(u1,u2,u3,i4)
    ww("\r\n=========================================================");
    TEST4(i2,u2,u3,u4)
    TEST4(u1,i2,u3,u4)
    TEST4(u1,u2,i2,u4)
    TEST4(u1,u2,u3,i2)

}
void loop()
{
}

Result (bad):

 0x12 = 0x12 => 0x12     ( 0x00000012 )
 0x12 0x34 = 0x1234 => 0x1234    ( 0x00001234 )
 0x12 0x34 0x56 = 0x3456 => 0x3456       ( 0x00003456 )
 0x12 0x34 0x56 0x78 = 0x5678 => 0x5678  ( 0x00005678 )
=========================================================
 0x1 = 0x1 => 0x1        ( 0x00000001 )
 0x1 0xFFFFFFFF = 0xFFFFFFFF => 0xFFFFFFFF       ( 0xFFFFFFFF )
 0x1 0xFFFFFFFF 0x8 = 0xFFFFFF08 => 0xFFFFFF08   ( 0xFFFFFF08 )
 0x1 0xFFFFFFFF 0x8 0xFFFFFFF8 = 0xFFFFFFF8 => 0xFFFFFFF8        ( 0xFFFFFFF8 )
=========================================================
 0x12 0x34 0x56 0x78 = 0x5678 => 0x5678  ( 0x00005678 )
 0x12 0x34 0x56 0x78 = 0x5678 => 0x5678  ( 0x00005678 )
 0x12 0x34 0x56 0x78 = 0x5678 => 0x5678  ( 0x00005678 )
 0x12 0x34 0x56 0x78 = 0x5678 => 0x5678  ( 0x00005678 )
=========================================================
 0x1 0x34 0x56 0x78 = 0x5678 => 0x5678   ( 0x00005678 )
 0x12 0xFFFFFFFF 0x56 0x78 = 0x5678 => 0x5678    ( 0x00005678 )
 0x12 0x34 0x8 0x78 = 0x878 => 0x878     ( 0x00000878 )
 0x12 0x34 0x56 0xFFFFFFF8 = 0xFFFFFFF8 => 0xFFFFFFF8    ( 0xFFFFFFF8 )
=========================================================
 0xFFFFFFFF 0x34 0x56 0x78 = 0x5678 => 0x5678    ( 0x00005678 )
 0x12 0xFFFFFFFF 0x56 0x78 = 0x5678 => 0x5678    ( 0x00005678 )
 0x12 0x34 0xFFFFFFFF 0x78 = 0xFFFFFF78 => 0xFFFFFF78    ( 0xFFFFFF78 )
 0x12 0x34 0x56 0xFFFFFFFF = 0xFFFFFFFF => 0xFFFFFFFF    ( 0xFFFFFFFF )

Problems (again):

1) it takes only last 2 params (probabelly because some casting to 16bits variable somewhere)

2) it works wrong for signed variables (maybe making them longer somewhere along the way)

c++

macros

avr-gcc

asked on Stack Overflow Mar 6, 2017 by

gilhad • edited Mar 7, 2017 by

gilhad

3 Answers

I'm not sure about the library and what's your use-case, but I can see the following error in your PACKx macros:

#define PACK3(cmd1,cmd2,cmd3) ((cmd1<<16)|(cmd2<<8)|cmd3) // ! (cmd2 << 8) == 0

Since the cmd2 type is limited to 8 bits, left-shifting by eight bits (or more) will cause the original 8bit type to overflow, so the result is probably 0...

...To be more precise, the result is "undefined" (credit to @GavinPortwood in the comments)... to be even more precise, we would have to get into integer promotion details and wonder why your system promotes uint8_t into a 16 bit type instead of a 32 bit type...

...but either way, it's a good practice to manually cast the type to the number of bits required as well as to truncate the results to the number of bits wanted.

The same is true for cmd3.

You should probably also consider that bit-shifting a signed variable is inconsistent across systems, since some systems protect the signed bit from being shifted - so that (-4 >> 1) == -2 instead of loosing the signed bit, which might give you 126 (if my math is right).

My last pointer is that I would avoid having a long list of macros when possible.

I think wrapping the library would probably be a better choice and it will make maintenance far easier.

Having said that, here's my untested take on the macro solution:

struct GET_DATA_args {
  uint8_t cmd;
  uint8_t arg1;
  uint8_t arg2;
  uint8_t arg3;
  uint8_t argc;
  uint8_t *dest;
  uint8_t limit;
  int addr;
};

static inline void GET_DATA(struct GET_DATA_args args) {
  uint32_t cmd = args.cmd | ((uint32_t)args.arg1 << 8) |
                 ((uint32_t)args.arg2 << 16) | ((uint32_t)args.arg3 << 24);
  if (!args.limit) /* use 1 as a default value. */
    args.limit = 1;
  get_data(args.addr, cmd, args.argc + 1, args.dest, args.limit);
}

#define GET_DATA(...)                                                          \
  GET_DATA((struct GET_DATA_args){.addr = this->i2c, __VA_ARGS__})

You can use it like this:

GET_DATA(.argc = 2, .cmd = SET_SPEED_LIMIT,
         .arg1 = 30, .arg2 = status,
         .dest = &status /*, .limit = 1 - by default */);

answered on Stack Overflow Mar 6, 2017 by

Myst • edited Mar 6, 2017 by

Myst

So the problem was on bad types (as was pointed out) and could be solved by explicitelly casting all arguments to 8-bit unsigned (to preserve value and do not expand sign) and then casting the shift to long enough type.

This one works for me (the example code compiles without warnings and yelds correct results) (as Lundin pointed out the casts should be organized a little different way, so I fixed SHIFT_IT. Still yelds the same good result):

#define SHIFT_IT(cmd,bits) (((uint32_t)(uint8_t)cmd)<<bits)

#define PACK1(cmd1) (SHIFT_IT(cmd1,0))
#define PACK2(cmd1,cmd2) (SHIFT_IT(cmd1,8)|SHIFT_IT(cmd2,0))
#define PACK3(cmd1,cmd2,cmd3) (SHIFT_IT(cmd1,16)|SHIFT_IT(cmd2,8)|SHIFT_IT(cmd3,0))
#define PACK4(cmd1,cmd2,cmd3,cmd4) (SHIFT_IT(cmd1,24)|SHIFT_IT(cmd2,16)|SHIFT_IT(cmd3,8)|SHIFT_IT(cmd4,0))

Result (good):

 0x12 = 0x12 => 0x12     ( 0x00000012 )
 0x12 0x34 = 0x1234 => 0x1234    ( 0x00001234 )
 0x12 0x34 0x56 = 0x123456 => 0x123456   ( 0x00123456 )
 0x12 0x34 0x56 0x78 = 0x12345678 => 0x12345678  ( 0x12345678 )
=========================================================
 0x1 = 0x1 => 0x1        ( 0x00000001 )
 0x1 0xFFFFFFFF = 0x1FF => 0x1FF         ( 0x000001FF )
 0x1 0xFFFFFFFF 0x8 = 0x1FF08 => 0x1FF08         ( 0x0001FF08 )
 0x1 0xFFFFFFFF 0x8 0xFFFFFFF8 = 0x1FF08F8 => 0x1FF08F8  ( 0x01FF08F8 )
=========================================================
 0x12 0x34 0x56 0x78 = 0x12345678 => 0x12345678  ( 0x12345678 )
 0x12 0x34 0x56 0x78 = 0x12345678 => 0x12345678  ( 0x12345678 )
 0x12 0x34 0x56 0x78 = 0x12345678 => 0x12345678  ( 0x12345678 )
 0x12 0x34 0x56 0x78 = 0x12345678 => 0x12345678  ( 0x12345678 )
=========================================================
 0x1 0x34 0x56 0x78 = 0x1345678 => 0x1345678     ( 0x01345678 )
 0x12 0xFFFFFFFF 0x56 0x78 = 0x12FF5678 => 0x12FF5678    ( 0x12FF5678 )
 0x12 0x34 0x8 0x78 = 0x12340878 => 0x12340878   ( 0x12340878 )
 0x12 0x34 0x56 0xFFFFFFF8 = 0x123456F8 => 0x123456F8    ( 0x123456F8 )
=========================================================
 0xFFFFFFFF 0x34 0x56 0x78 = 0xFF345678 => 0xFF345678    ( 0xFF345678 )
 0x12 0xFFFFFFFF 0x56 0x78 = 0x12FF5678 => 0x12FF5678    ( 0x12FF5678 )
 0x12 0x34 0xFFFFFFFF 0x78 = 0x1234FF78 => 0x1234FF78    ( 0x1234FF78 )
 0x12 0x34 0x56 0xFFFFFFFF = 0x123456FF => 0x123456FF    ( 0x123456FF )
=========================================================
 0xFFFFFFFF 0x34 0x56 0x78 = 0xFF345678 => 0xFF345678    ( 0xFF345678 )
 0x12 0xFFFFFFFF 0x56 0x78 = 0x12FF5678 => 0x12FF5678    ( 0x12FF5678 )
 0x12 0x34 0xFFFFFFFF 0x78 = 0x1234FF78 => 0x1234FF78    ( 0x1234FF78 )
 0x12 0x34 0x56 0xFFFFFFFF = 0x123456FF => 0x123456FF    ( 0x123456FF )
=========================================================
 0xFFFFFFFF 0x34 0x56 0x78 = 0xFF345678 => 0xFF345678    ( 0xFF345678 )
 0x12 0xFFFFFFFF 0x56 0x78 = 0x12FF5678 => 0x12FF5678    ( 0x12FF5678 )
 0x12 0x34 0xFFFFFFFF 0x78 = 0x1234FF78 => 0x1234FF78    ( 0x1234FF78 )
 0x12 0x34 0x56 0xFFFFFFFF = 0x123456FF => 0x123456FF    ( 0x123456FF )

Anyway, if I find better way (maybe with variadic macros, to have just one PACK with variable number of commands) I will add it here too.

(BTW: Thanks all to help me find the way, I was really deep locked before)

answered on Stack Overflow Mar 6, 2017 by

gilhad • edited Mar 6, 2017 by

gilhad

What you probably should do is to write a function, to make the code type safe and generic. Depending on byte order (endianess) etc, this function might look a bit different. Something along the lines of:

#include <stdint.h>
#include <string.h>

uint32_t pack (size_t size, uint8_t bytes[size])
{
  uint8_t tmp[sizeof(uint32_t)] = {0};
  memcpy(tmp, bytes, size);

  return tmp[0] <<  0 |
         tmp[1] <<  8 |
         tmp[2] << 16 |
         tmp[3] << 24 ; 
}

Macros are hard to read, understand and maintain, so I wouldn't recommend them. But if you insist on using them, you could do some evil macro-magic with variadic macros:

(assuming 32 bit CPU)

#include <stdint.h>
#include <stdio.h>
#include <inttypes.h>

#define SHIFT(cmd, bits) (((uint32_t)(cmd)) << (bits))

#define PACK1(c1,...)          ( SHIFT(c1, 0) )
#define PACK2(c1,c2,...)       ( SHIFT(c1, 8) | SHIFT(c2, 0) )
#define PACK3(c1,c2,c3,...)    ( SHIFT(c1,16) | SHIFT(c2, 8) | SHIFT(c3,0) )
#define PACK4(c1,c2,c3,c4,...) ( SHIFT(c1,24) | SHIFT(c2,16) | SHIFT(c3,8) | SHIFT(c4,0) )

#define PACK_VARIABLE(c1,c2,c3,c4,N,...) PACK##N (c1, c2, c3, c4, __VA_ARGS__)
#define PACK(...) PACK_VARIABLE(__VA_ARGS__,4,3,2,1,0)

int main (void)
{
  printf("%" PRIX32 "\n", PACK(0xAA));
  printf("%" PRIX32 "\n", PACK(0xAA, 0xBB));
  printf("%" PRIX32 "\n", PACK(0xAA, 0xBB, 0xCC));
  printf("%" PRIX32 "\n", PACK(0xAA, 0xBB, 0xCC, 0xDD)); 
  return 0;  
}

Output:

AA
AABB
AABBCC
AABBCCDD

answered on Stack Overflow Mar 6, 2017 by

Lundin

User contributions licensed under CC BY-SA 3.0