Arduino - Convert 4 x uint8 to 1 x uint32

[kolbep]

Hi All.
So I am Interfacing a PZEM16 Modbus Power Meter Module to an Arduino nano,
and so far I have got it working ok. I am just having problems converting the 8 Bit Register Returns into 32 Bit Integers.
It works fine at low current, but as soon as the current increases over a few amps, I start getting a display of 40000 + Amps.

Here is the Relevant Section of code where it receives all the registers from the module

Code: [Select]

   while (mySerial.available()) {
      int countr = countr + 1;
      buffer[countr] = mySerial.read();
      containsdata = 1;
    };

And here is when I try to convert it to 32 Bit Integers, to make it easier for Display on the Serial, LCD, and logging to file :

Code: [Select]

   if (containsdata) {
      uint16_t rvoltage = (buffer[4] << 8) | buffer[5];
      uint32_t rcurrent = (buffer[6] << 8) | buffer[7] | (buffer[8] << 24) | (buffer[9] << 16);
      uint32_t rcurrent = (buffer[6] << 8) | buffer[7];
      uint32_t rpower = (buffer[10] << 8) | buffer[11] | (buffer[12] << 24) | (buffer[13] << 16);
      uint32_t renergy = (buffer[14] << 8) | buffer[15] | (buffer[16] << 24) | (buffer[17] << 16);
      uint32_t rfreq = (buffer[18] << 8) | buffer[19];
      uint32_t rpf = (buffer[20] << 8) | buffer[21];

      float pvoltage = float(rvoltage) / 10;
      float pcurrent = float(rcurrent) / 1000;
      float ppower = float(rpower) / 10;
      float penergy = float(renergy);
      float pfreq = float(rfreq) / 10;
      float ppf = float(rpf) / 100;

      Serial.print("-----------------------------------");
      Serial.println(rcurrent);

      Serial.print(pvoltage, 1); Serial.print(F("V "));
      Serial.print(pcurrent, 3); Serial.print(F("A "));
      Serial.print(ppower, 1); Serial.print(F("W "));
      Serial.print(penergy, 1);  Serial.print(F("E "));
      Serial.print(pfreq, 1); Serial.print(F("hz "));
      Serial.print(ppf, 2); Serial.print (F("pf "));

      Serial.println();



From what I understand (From the Attached Register Listing),
Element 1 2 and 3 in the Array are irrelevant, they are just the Slave Address, etc. {They are not shown in this table, but from the rest of the Datasheet it makes sense}

that Element 5 is the Low Byte for Voltage, Element 4 is the High Byte for Voltage.
And for current, Element 7 Is Low Byte, Element 6 is the High Byte, Element 9 is the High Bytes Low, and Element 8 is the High Bytes High.

Even as I am typing this out, I am confused.

Please can somebody check this code, and the Image of Register Listings, and help me out.

Thanks

[RoGeorge]

- identify the indian-ness type for your uint32 representation
- use "union" to easily read the same memory location(s) either as uint8 or as uint32
- never start talking with "so", "so" is for conclusion, not for explaining a situation

[kolbep]

I think I have just made some headway.
In my Global Variable Declaration I had

Code: [Select]

byte buffer[25];

changed it to

Code: [Select]

unsigned int buffer[25];

and now it seems to be better, more accurately tracking the load

[rstofer]

I sure wonder about

Code: [Select]

while (mySerial.available()) {
      int countr = countr + 1;
      buffer[countr] = mySerial.read();
      containsdata = 1;
    };

countr is not initialized to zero nor is it static.  countr will be allocated on the stack and the stack contains random values.  I'm not sure how it would work out.

Since you have it working, maybe this doesn't matter.

[sokoloff]

I think I have just made some headway.
In my Global Variable Declaration I had

Code: [Select]

byte buffer[25];

changed it to

Code: [Select]

unsigned int buffer[25];

and now it seems to be better, more accurately tracking the load

Good.

In general, you want to post a complete reproduction case when asking for help, rather than eliding the code and only posting the "relevant" portion (which in this case was a necessary but not sufficient subset).

[Renate]

Never start talking with "so", "so" is for conclusion, not for explaining a situation.

So what.

Generally I'd take Modbus in on  a uint8_t buffer.
If you're using "switch" or "coil" there could be an odd number of bytes.
OTOH, if in a specific case it's all registers then uint16_t could be easier.
(I still wouldn't do it.)

As you've shown already, Modbus is high byte, low byte order.
It seems that usage of multiple words by implementers tends to be low word (16bit), high word.

I think that I'll pick up one of these.
I guess that you can only set address by putting it on a standalone wire and writing the address register there?

[IDEngineer]

use "union" to easily read the same memory location(s) either as uint8 or as uint32

Be very careful with this technique. It relies on having an absolute understanding of how your compiler of choice handles structures/unions, and in particular how it handles packing. That can vary from compiler to compiler, and even revision to revision of the same compiler. Don't ask what it cost me to learn that.

For some background on the topic, you can start with: http://www.catb.org/esr/structure-packing

To the OP: I strongly suggest taking the time to better understand your data, and then manually write safe routines to rearrange it as desired. You assume your compiler does, and later will continue to, handle such conversions at your peril.

[SiliconWizard]

A note - knowing the endianness of your target, and provided that it's the same as your data, you can safely "read" from or "write" to a buffer of uint8_t as a uint32_t... as long as there is no alignment problem! Many small MCUs don't support unaligned accesses, so you can only do this if the data is properly aligned. So, that's a first potential trap. The second one is of course if the endianness is not the same, then obviously you can't do that, or you'd have to reverse the bytes, which you might as well do directly.

If you're using structs to "map" data, you need to be careful about alignment as well. As said above, first make sure the various fields are "packed" (meaning not padded), compilers usually have attributes for that. An older, but usually supported by most compilers, way of enforcing this is to use "#pragma pack(...)" directives. But exactly as with the above, you could still encounter unaligned access problems.

Consider this struct for instance (GCC attributes):

Code: [Select]

struct __attribute__((__packed__, aligned(4)))
{
    int32_t A;
    int16_t B;
    int32_t C;
};

If the target is 32-bit and doesn't support unaligned accesses, you'll have a problem. Accessing A will be OK. Accessing B will be OK. Accessing C will not.

[RoGeorge]

use "union" to easily read the same memory location(s) either as uint8 or as uint32

Be very careful with this technique. It relies on having an absolute understanding of how your compiler of choice handles structures/unions, and in particular how it handles packing. That can vary from compiler to compiler, and even revision to revision of the same compiler. Don't ask what it cost me to learn that.

For some background on the topic, you can start with: http://www.catb.org/esr/structure-packing

Not sure where to look.  That is a link to more than 5 thousand words about structures, nothing about unions.  In fact, the unions are mentioned as much safer, yet not discussed there.

I was thinking about a conversion like in the following example.  The advantage is that there are no additional operations, and can be used for either ui32 to ui8[4] or ui8[4] to ui32.

Code: [Select]

...
union ui32or4xui8 {
  uint32_t as32bitsTogether;
  uint8_t[4] as8bitsByte;
};
...

int main() {
...
  union ui32or4xui8 myNumber;
...

  /* write ui32, read each ui8 */
  myNumber.as32bitsTogether = 0xDEADBEEF;

  printf("%x", myNumber.as8bitsByte[0]);
  printf("%x", myNumber.as8bitsByte[1]);
  printf("%x", myNumber.as8bitsByte[2]);
  printf("%x", myNumber.as8bitsByte[3]);
...

  /* write each ui8, read as ui32 */
  myNumber.as8bitsByte[0] = 0xB0;
  myNumber.as8bitsByte[1] = 0xB1;
  myNumber.as8bitsByte[2] = 0xB2;
  myNumber.as8bitsByte[3] = 0xB3;

  printf("%x", myNumber.as32bitsTogether);
...
}

AFAIK, all union members are guaranteed to start at the same address, and an uint8 vector is never padded between the elements.

Can you please give an example of architecture/compiler/settings where the above union is not safe (assuming we know the endianness)?

[SiliconWizard]

AFAIK, all union members are guaranteed to start at the same address, and an uint8 vector is never padded between the elements.

Although that is a reasonable assumption, the real problem is not so much with the start address. There still isn't any formal guarantee that writing to an union member will be reflected as is if reading another union member afterwards.

Standard C99 says:

" With one exception, if the value of a member of a union object is used when the most
recent  store  to  the  object  was  to  a  different  member, the  behavior  is
implementation-defined.

One special guarantee is made in order to simplify the use of
unions: If a union contains several structures that share a common initial sequence (see
below), and if the union object currently contains one of these structures, it is permitted to
inspect  the  common  initial  part  of  any of them  anywhere  that  a  declaration  of  the
completed type of the union is visible."

In your example, the union contains two members that are themselves not structures. So the behavior is actually implementation-defined as per the standard.
Whereas it will do as you expect with many C compilers for many different targets, there is just no guarantee. It's implementation-defined, meaning that by nature it's not portable code.
Now for programming specific MCU targets with a specific compiler, you may rely on the implementation doing that correctly as long as you checked it did. Just be aware that it's not, by definition, portable.
(And yes, we may argue that any implementation that would not guarantee this would be pretty twisted, but the standard still considers this possible.)

[RoGeorge]

I thought that type conversion through a union is a trick, but it even has a name "union type punning".   
https://stackoverflow.com/questions/25664848/unions-and-type-punning

Some are saying it's working fine.  Some don't like it.

Seems like the software world is still arguing about union type punning, but coming from the hardware world, I see no reason why it shouldn't work for a simple 32 <-> 4x8 bits (again, apart from reasons like endianness, compiler optimisations that can be fixed by using volatile, or obvious causes like "manually" writing consecutive uint8 values in an array will not be atomic operations from the viewpoint of a 32 bits read, etc).

I know the "I see no reason" is not a proof for "there is no reason", but still feels like an urban legend.

Does anybody have an example where union type punning of uint32 and uint8[4] behave different than expected?

[newbrain]

Some are saying it's working fine.  Some don't like it.

Does anybody have an example where union type punning of uint32 and uint8[4] behave different than expected?

Under a quite relaxed set of constraints, that specific example of type punning is perfectly fine.
The only condition to verify is that sizeof(uint32_t) == 4.

Just to make sure, let's see how I derive this conclusion, from the C99 standard:

First of all, we are guaranteed that there are no padding bits or bytes:
Both uint8_t and uint32_t are exact-width integer types.

As a typedef, uint8_t it cannot be smaller than the smaller integer type, char, and is unsigned so there's no alternative for it to be anything else that unsigned char (7.18.1.1), with size 1 by definition.

Chapter 6.2.6.1 (Representation of types - General) states in §4:

Values stored in non-bit-field objects of any other object type consist of n × CHAR_BIT
bits, where n is the size of an object of that type, in bytes. The value may be copied into
an object of type unsigned char [n] (e.g., by memcpy); the resulting set of bytes is
called the object representation of the value.

This is a guarantee that any object value can be represented in a sequence of unsigned chars. It does not guarantee, in general, that any value of the sequence corresponds to a valid object (trap representations), but in our case:
- There is no padding
- All the bit combination in an uint32_t (sized 4) are then a valid value, as a consequence of 6.2.6.2 Integer types, §4
So no trap representation is possible.

Similar consideration can be made in other cases, as an example, if instead of uint32_t we consider int32_t, we must add another constraint:
The implementation must use two's complement integer representation.
If this is not verified (one's complement or sign magnitude) the bit pattern for negative 0 might be a trap representation.
As both the representation and the trap status of negative 0 are implementation-defined, they must be documented by the compiler.

Now, endianness is a different matter...but still must be documented, as is part of the implementation-defined behaviour (J.3.13 Architecture).

EtA: It must be said that if type punning is done to "optimize" the code, it is mostly a wasted effort.
Much better use explicit shifts: if they match endiannes, they are optimized away, if they do not, type punning would introduce a bug.

[IDEngineer]

First of all, we are guaranteed that there are no padding bits or bytes:
Both uint8_t and uint32_t are exact-width integer types.

The latter statement is true. The former statement may NOT be true, which is why you must be careful. 32 bit values may be required by the processor architecture to be aligned on a word or dword boundary. If you lead with a char, there could be padding to align an immediately following word or dword.

Note you can always read or write multibyte entities as bytes. It's going the other way that may be a problem (reading multiple individual bytes as a word, dword, etc.). If your base address is not aligned for the size of read/write you're requesting you can get anything from no effect, to additional cycles as the CPU silently accommodates the misalignment, to a fault as noted in that article I cited. When I used to write Assembly language for the x86 family we had to pay careful attention to data alignment to maximize performance for exactly this reason.

Now, endianness is a different matter...but still must be documented, as is part of the implementation-defined behaviour (J.3.13 Architecture).

So far I've seen people only mention endianness at the byte level. However, be careful with some sensors which can (optionally, if you're lucky!) reverse the BIT order within their output bytes. The most recent time I saw this was in an accelerometer+gyro MEMS chip; the data could be reordered in almost any way, with reversal within each byte as well as reordering of the bytes themselves in the 16-bit values. I suspect this was done to assist with implementing FFT's since those can involve bit-level reversal, and that's not an operation commonly found in non-DSP cores so offering it in the hardware could yield a performance boost on general purpose CPU's.

My bottom line for stuff like this: Write the code and step through it with the debugger, line by line, examining all of the data at each step to make positively certain you know what the compiler AND the CPU are doing. Then heavily document it in the source so you, or someone else, know where the potential trouble spots are and how they are (presently) handled for which compiler and which underlying hardware. The language specs are delightful but there's no guarantee that a given version of a given compiler for a given hardware target actually implemented something strictly to the specs. I trust no one and verify everything. YMMV!

[newbrain]

First of all, we are guaranteed that there are no padding bits or bytes:
Both uint8_t and uint32_t are exact-width integer types.

The latter statement is true. The former statement may NOT be true, which is why you must be careful. 32 bit values may be required by the processor architecture to be aligned on a word or dword boundary. If you lead with a char, there could be padding to align an immediately following word or dword.

I respectfully disagree:

0. Our uint32_t has size 4 - note that I explicitly put this as a precondition.

1. Char hasn't got any padding bits, per 6.2.6.2.

2. uint8_t cannot be anything else than unsigned char: cannot be smaller, as it is both exact-width and a typedef, and cannot be larger as the minimum range for unsigned char is 0-255.

3. An union must be correctly aligned for both its member types - we are not considering a generic uint8_t[4] - and there can't be padding at the beginning, per 6.7.2.1 Structure and union specifiers, §13.

4. All the representations in an exact-width 32 bit unsigned int of size 4 must be valid (I don't think I have to show why).

All of the the above yields:
- No padding is possible in an uint32_t when its size is 4.
- There are no trap representations possible.
- Alignment is guaranteed.
- I can read the uint32_t as a sequence of 4 uint8_t (always guaranteed, no need for an union).
- I can read the sequence of 4 uint8_t as an uint32_t, without fear of misalignments or trap representations

[GeorgeOfTheJungle]

- use "union" to easily read the same memory location(s) either as uint8 or as uint32

Why not use a ptr to the uint32_t, that you can always cast as needed?

Code: [Select]

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

int main (void) {
    uint32_t a= 0x03020100;
    uint8_t* b= (uint8_t*) &a;

    printf("%d %d %d %d\n", b[0], b[1], b[2], b[3]);
}

-> 0 1 2 3

Code: [Select]

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

int main (void) {
    uint32_t a= 0x03020100;
    #define aptr ((uint8_t*) &a)

    printf("%d %d %d %d\n", aptr[0], aptr[1], aptr[2], aptr[3]);
}
-> 0 1 2 3

[GeorgeOfTheJungle]

It works fine at low current, but as soon as the current increases over a few amps, I start getting a display of 40000 + Amps.

"A few amps" is less than 10?

I'd printf the bytes of that uint32_t (see the msg above) and compare what you get with these values in mA:


   1A= 0x000003e8
   2A= 0x000007d0
   3A= 0x00000bb8
   4A= 0x00000fa0
   5A= 0x00001388
   6A= 0x00001770
   7A= 0x00001b58
   8A= 0x00001f40
   9A= 0x00002328
40e3A= 0x02625a00

[IDEngineer]

Alignment is guaranteed.

Consider this case, one of many easy examples (addresses are offsets from the base of the struct/union, which is itself presumed to be aligned). We have a char followed by a dword in a struct/union:

Offset 0x00: char
Offset 0x01: LSB of dword
Offset 0x02: next byte of dword
Offset 0x03: next byte of dword
Offset 0x04: MSB of dword

Accessing the char is guaranteed to be aligned, yes. It's a single byte. Accessing the dword as a 32 bit entity... not so much. It's on an odd address. The char caused it to be offset from the base of the (overall aligned) struct/union.

Options to permit aligned access for BOTH members include:

1) Insert a pad byte at 0x01 (works for architectures where a 32 bit access need only be aligned on an even address, e.g. address LSb = 0)
2) Insert three pad bytes at 0x01 (works for architecture where a 32 bit access is required to be on a modulo-4 address, e.g. two address LSb's = 0)
3) Author rearranges the order of the struct/union to place the dword at offset 0x00 (compiler dares not do this on its own since other code may presume the source's explicit ordering)

Note that options 1 and 2 may end up being repeated if the struct/union has more members which also end up misaligned due to ordering. It is possible to write a compiler that could automatically optimize ordering, but that had better be an explicit opt-in or else the cast-based accessing being discussed in this thread would fail horribly - AND the compiler may not even be able to warn about it depending upon how the cast-based accessing was expressed in the source.

[sokoloff]

Under a quite relaxed set of constraints, that specific example of type punning is perfectly fine.
The only condition to verify is that sizeof(uint32_t) == 4.

I think you also need to verify that CHAR_BITS is 8 (which is required by POSIX and as a practical matter, it's been decades since a platform had a CHAR_BITS of 9, but that would still allow for sizeof(uint32_t) == 4 and yet almost assuredly guarantee that a uint8_t[4] would not be the same bits as a uint32_t) The spec only guarantees that CHAR_BITS is >= 8.

[newbrain]

Under a quite relaxed set of constraints, that specific example of type punning is perfectly fine.
The only condition to verify is that sizeof(uint32_t) == 4.

I think you also need to verify that CHAR_BITS is 8 (which is required by POSIX and as a practical matter, it's been decades since a platform had a CHAR_BITS of 9, but that would still allow for sizeof(uint32_t) == 4 and yet almost asesuredly guarantee that a uint8_t[4] would not be the same bits as a uint32_t) The spec only guarantees that CHAR_BITS is >= 8.

Remember, we have in this case an uint8_t.
According to standard, this is a typedef so it does not change the characteristics of the underlying type.
It is also an exact-width integer type.
It cannot be smaller than unsigned char as (unsigned) char is the smallest integer.
It cannot be larger than unsigned char as 8 bits is the minimum size for unsigned char.
The two statements above are guaranteed by "5.2.4.2.1 Sizes of integer types <limits.h>", §1:

— number of bits for smallest object that is not a bit-field (byte)
   CHAR_BIT                    8

There are no objects smaller than CHAR_BIT (bit-fields are off-topic in this discussion)
So, if we have uint8_t at all, CHAR_BIT==8.

Having sizeof(uint32_t)==4 is only the last of a series of preconditions, but using exact-width integers automatically verifies them: they are optional types, if they are there they must behave as stated.
 If we had used generic (unsigned) ints and chars they would not hold.

@IDE_engineer: no. Again: it's a union and there's no padding at the beginning. The members of an union begin at the same address. If one is typacasting odd structures into other objects all bets are of course off.

"cast-based"?
This is not cast based. Ths is type punning and uses a union. No cast operators in sight.

EtA: Standard chapter for minimum object size, it's CHAR_BIT not BITS.

[newbrain]

It is possible to write a compiler that could automatically optimize ordering

Of course, but not in a standard compliant mode.
See "6.5.8 Relational operators", §5:

...If the objects pointed to are members of the same aggregate object, pointers to structure members declared later compare greater than pointers to members declared earlier in the structure...

[donotdespisethesnake]

Always funny to see people justifying why writing bad code is a good idea. 

In this case, it's not like there is only one way to do it. The alternative method is always guaranteed to be correct with any compiler on any platform, and usually compiles to something with similar performance. It borders on insanity not to use it.

My job is basically is fixing other people's bad code, so I have seen a lot of it.

[GeorgeOfTheJungle]

In this case, it's not like there is only one way to do it. The alternative method is always guaranteed to be correct

Which way is that?

[newbrain]

Always funny to see people justifying why writing bad code is a good idea. 

If you are referring to my explanation, I might have made my purpose and stance clearer.

I already stated that this us of type punning is wasted time, and I would never advocate (or use) it.
My elucubrations were just the answer to RoGeorge doubt, to show that, yes, in this very specific case the standard is backing you (IMO, but I haven't seen a valid rebuttal, yet).
But let the phase of the moon change, and undefined behaviour will rear its ugly head (in some, more benign, cases just implementation defined behaviour).

Do I find this code "bad"? Yes.
Do I think it's standard compliant? Yes. In C99 at least, C11 and later change some fine points in the wording.

The alternative method is always guaranteed to be correct with any compiler on any platform, and usually compiles to something with similar performance. It borders on insanity not to use it.

Could not agree more. As I also demonstrated.

I am quite sure I've never written this kind of code. I make use of unions very sparingly in any case.

My job is basically is fixing other people's bad code, so I have seen a lot of it.

Eh...having been in the same position I'm glad Italian has wonderfully rich vocabulary for swearing.

Which way is that?

Use shifts.

[GeorgeOfTheJungle]

Use shifts: https://gcc.godbolt.org/z/DSxgPJ

gcc.godbolt.org !!

[grizewald]

I sure wonder about

Code: [Select]

while (mySerial.available()) {
      int countr = countr + 1;
      buffer[countr] = mySerial.read();
      containsdata = 1;
    };

countr is not initialized to zero nor is it static.  countr will be allocated on the stack and the stack contains random values.  I'm not sure how it would work out.

Since you have it working, maybe this doesn't matter.

Maybe it doesn't, but declaring 'countr' inside the while loop is just wrong.