0

I'm not very sure of my code and want to improve it.

I receive some datas from SPI (8 bits communication) and store it into a buffer of 8 bits so. To use it, I want to use 32 bits word. I know my first code will work but I'm not sure about the second one, can anyone confirm it ?

uint8_t *regData[5];

spi_xfer(fd, "\x24\xFF\xFF\xFF\xCC", 5, regData, 5);
uint32_t regVal;
regVal = (regData[0]);
regVal += (uint32_t)(regData[1]) << 8; 
regVal += (uint32_t)(regData[2]) << 16; 
regVal += (uint32_t)(regData[3]) << 24;

The second one :

uint8_t *regData[5];

spi_xfer(fd, "\x24\xFF\xFF\xFF\xCC", 5, regData, 5);
uint32_t regVal;
regVal = (regData[0]) | (uint32_t)(regData[1]) << 8 | (uint32_t)(regData[2]) << 16 | (uint32_t)(regData[3]) << 24;

Thanks a lot for your help !

Brieuc

B. Delbart
  • 41
  • 5
  • For code that is already working, you might want to ask in https://codereview.stackexchange.com/ instead. Aside from that, you should write tests to make sure code is working. – user694733 Oct 31 '17 at 08:53
  • Side note: `uint8_t *regData[5];` looks wrong on both versions. It looks like you are serializing array of pointers. But it's unclear, as we don't know what `spi_xfer` does. – user694733 Oct 31 '17 at 08:55

1 Answers1

1
uint8_t *regData[5];

The regData[] is an array of pointers. If this is intended, to retrieve the value stored at the pointer in the array you need to dereference the pointer.

regVal = *(regData[0]);

Otherwise the operation will assign the address stored at regData[0] to regVal, rather than the value stored at the address.

S. Whittaker
  • 118
  • 9
  • Is that not easier to use the code like this ? (uint8_t regData[5]; spi_xfer(fd, "\x24\xFF\xFF\xFF\xCC", 5, regData, 5); uint32_t regVal; regVal = (regData[0]) | (uint32_t)(regData[1]) << 8 | (uint32_t)(regData[2]) << 16 | (uint32_t)(regData[3]) << 24;) Thanks a lot for your answers ! – B. Delbart Oct 31 '17 at 09:04
  • The `or` operation is valid. I would suggest wrapping each condition in brackets, e.g. `((uint32_t)(regData[x]) << 8)`. – S. Whittaker Oct 31 '17 at 09:07