Style question for sending 16bit values over UART.

[Obi_Kwiet]

I'm dusting off an old project, and I decided I wanted to address the few compiler warnings I had. I'm working in C.

At one point I need to pass an array of 16bit values to another micro-controller over uart. The driver from the TX micro takes a pointer to an array of uint8_t. My solution was to simply set the uint8_t array pointer equal to the first element pointer of the 16bit array, and feed the driver that, like so.

Code: [Select]

    uint16_t LEDArray[LED_COLUMNS];
    char *ptrLEDArray;
    ptrLEDArray = LEDArray;
This works fine, except I get a compiler warning about assignment from incompatible pointer type. Should this warning be ignored, or is there a better way to do this? I can think of some ways, but they all involve writing a loop that runs through the array, which is pretty lame. Is my approach bad practice for reasons I don't understand?

[ataradov]

Your approach is fine, don't overthink it, just do a cast: "ptrLEDArray = (char *)LEDArray;".

[newbrain]

As ataradov said.

But, let's overthink it a bit! 
Converting a pointer to whatever to a pointer to char is one of the few pointer conversions explicitly permitted in the C standard, in chapter 6.3.2.3 Pointers, clause 7 of C99 draft:

A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the resulting pointer is not correctly aligned57) for the pointed-to type, the behavior is undefined. Otherwise, when converted back again, the result shall compare equal to the original pointer. When a pointer to an object is converted to a pointer to a character type, the result points to the lowest addressed byte of the object. Successive increments of the result, up to the size of the object, yield pointers to the remaining bytes of the object.

Specifically, the type uint8_t is usually defined as unsigned char, so the above should hold.
If the array address is cast to a (char *), though, a conforming compiler might still complain as char and unsigned char are not the same type (clause 15 and footnote 35 in chapter 6.2.5 Types), even where char is unsigned by default or due to target architecture.
I would then suggest to use uint8_t* or unsigned char* in the cast.

One reason unsigned char is commonly used as a way of passing byte sequences is that it is the only type for which the standard explicitly guarantee that all its bits concur to forming the value (so one won't lose information when accessing it), see 6.2.6.1 cl. 3 and 6.2.6.2 cl. 1.

[Doctorandus_P]

My prefered method is to mae a union in which the data can either be accessed as an array of integers or an array of bytes.

It is bad practice to ignore warnings. Once you have too many warnings you want to ignore, you do not see the single other / important warning anymore.

Be carefull though that methods like this can cause problems with endianness. It is safer to write some small support functions to explicitly serialise your data and extract high and low byte of the integer in a well defined order, for example:

uint8_t high = big >>8;
uint8_t low = big & 0xff;   // This may need a cast to suppres "assignment to smaller datatype"

[Cicero]

My prefered method is to mae a union in which the data can either be accessed as an array of integers or an array of bytes.

It is bad practice to ignore warnings. Once you have too many warnings you want to ignore, you do not see the single other / important warning anymore.

Be carefull though that methods like this can cause problems with endianness. It is safer to write some small support functions to explicitly serialise your data and extract high and low byte of the integer in a well defined order, for example:

uint8_t high = big >>8;
uint8_t low = big & 0xff;   // This may need a cast to suppres "assignment to smaller datatype"

Came here to say this...

[Obi_Kwiet]

Good call on the cast. I don't do a ton of software, so I didn't think about casting the pointer. I figured that I was setting one memory address type equal to another, but I guess the cast should just tell the compiler that I meant to do that.

ataradov, if I am understanding you correctly, it sounds like uint8_t is the only data that C guarantees the storage format for? (Other than endianness.) I guess you just need to be aware of your architecture before using this method.

I don't see any good reason to worry about byte order from the perspective of the other micro-controller. If the other micro is opposite endian, you'll still have to write a function to convert the bit order as well, so you may as well figure that out and write it all in one place. (And I think I did.) This way you do a single operation instead of executing multiple instructions on every array element and creating a duplicate array.

[newbrain]

My prefered method is to mae a union in which the data can either be accessed as an array of integers or an array of bytes.

I, for one, welcome our over-overthinkers.
But this is one of the few cases where a cast is benign and also sanctioned by the standard.

It is bad practice to ignore warnings. Once you have too many warnings you want to ignore, you do not see the single other / important warning anymore.

QFT.
And silencing them with casts is often not a good thing.

Good call on the cast. I don't do a ton of software, so I didn't think about casting the pointer. I figured that I was setting one memory address type equal to another, but I guess the cast should just tell the compiler that I meant to do that.

Yes, that's the purpose in this case.

ataradov, if I am understanding you correctly, it sounds like uint8_t is the only data that C guarantees the storage format for? (Other than endianness.) I guess you just need to be aware of your architecture before using this method.

I think you mean me, not ataradov, who gave a perfectly good answer and did not indulge in going into the details rabbit hole.  .
And yes, that's my reading of the standard.
In practice, I'd be hard pressed to find a counter example in modern, reasonable architectures, where (signed or unsigned) integers have padding bits (I'm sure someone will come up with one!).

[ataradov]

ataradov, if I am understanding you correctly, it sounds like uint8_t is the only data that C guarantees the storage format for? (Other than endianness.) I guess you just need to be aware of your architecture before using this method.

Yes, but I typically design all my protocols to be little-endian. This way all my stuff is compatible with sane architectures. For others you would have to do support functions and all that jazz. But there is really no need to think about until you actually do a port to BE architecture. And it may be never.

So you are not violating anything. Your protocol is rigidly defined. It just happens to be defined in a way that works out of the box on LE architectures.

[T3sl4co1l]

Mind that when you do something like,

Code: [Select]

low = (*big_p) & 0xff;
high = (*big_p) >> 8;
big_p += sizeof(array[0]);

assuming sizeof(array[0]) is 2 (so you aren't skipping bytes), the compiler will emit code to access the first byte, then the second byte, then increment the pointer.  In subsequent optimization steps (assuming more than -O0), it should realize that these steps are redundant (e.g., loading a 16-bit value into a pair of 8-bit registers, saving 'low'; loading them over again, shifting them over by a whole register width, then saving 'high'..), and optimize it down to sensible code.  In particular, for something like AVR, it has to increment the pointer and load memory, which should reduce to a pair of lds reg, [reg+] instructions.  Which includes the increment right there, so it's done.

This is why it's always a good idea to inspect the assembled output, to make sure you're doing what you think you're doing.

Instead of explicitly incrementing the pointer, indexing the array in a loop should reduce to very similar results.

Such code is always portable to other platforms, at some cost to memory or efficiency.  For example, a 32-bit machine will have an int array with a natural width of 4 bytes, but as long as you're using the same portable logic, you're only writing and reading the lower 16.  Memory is wasted, but access is not inefficient, in that it stays word-aligned.  (You usually pay a price for misaligned access: x86 memory fetch requires an entire bus cycle whether you're reading a single byte or the full bus; ARM doesn't allow it at all and so the compiler must generate fetch, mask and bitshift instructions.  In both cases, the faster code will load a word into a register, and chop it up from there.)  Likewise for big-endian systems, the pointer might have to stutter-step (e.g., 6809), costing execution time and code space, but remaining correct.

There can be no such thing as truly portable code; you can optimize your code structure to suit some platforms, and you can use #defines to target multiple platforms, but only as many as you have tested (and have driver support for -- in an embedded context, this is the probably the bigger challenge).

Tim

[Siwastaja]

So you are not violating anything. Your protocol is rigidly defined. It just happens to be defined in a way that works out of the box on LE architectures.

And, happens to be easier to write, easier to read and understand, less error prone, more ecologically sustainable, and so on.

Indeed - I do the same.

[newbrain]

Mind that when you do something like,

Code: [Select]

low = (*big_p) & 0xff;
high = (*big_p) >> 8;
big_p += sizeof(array[0]);


Something is off with that code:
Either big_p is a pointer to an uint8_t, then high will always be 0, or to something larger than an uint8_t type, and the += is skipping at least one array element.

Maybe you meant something as:

Code: [Select]

low = (*big_p++) & 0xff;
high = (*big_p++) >> 8;


[legacy]

OT:
for the Power9 Blackbird mainboard we (at DTB) have recently decided to rewrite the whole firmware in order to force the CPU into BE since the factory default is LE.

This seems a silly detail but it caused *a lot* of troubles with the software.
So BE vs LE is not a thing that you have to underestimate when you write software.

[SiliconWizard]

If the underlying send function uses byte transfers (meaning alignment is no issue), my preferred way of doing this is to declare a pointer to void as the function parameter. (void *)
Then you can pass it any pointer you want without the need of an explicit cast.

[Obi_Kwiet]

I think you mean me, not ataradov, who gave a perfectly good answer and did not indulge in going into the details rabbit hole.  .
And yes, that's my reading of the standard.
In practice, I'd be hard pressed to find a counter example in modern, reasonable architectures, where (signed or unsigned) integers have padding bits (I'm sure someone will come up with one!).

Yeah, I meant you, my bad! Thanks for the help. I'm happy with this solution, but I'll keep the various pitfalls in mind for the future.

[T3sl4co1l]

Something is off with that code:
Either big_p is a pointer to an uint8_t, then high will always be 0, or to something larger than an uint8_t type, and the += is skipping at least one array element.

Yes, let me clarify that:

Code: [Select]

int array[len];
uint8_t low, high;
int* big_p = array;
...
low = (*big_p) & 0xff;
high = (*big_p) >> 8;
big_p += sizeof(array[0]);


Maybe you meant something as:

Code: [Select]

low = (*big_p++) & 0xff;
high = (*big_p++) >> 8;


That's not what I meant, because on the next access, that won't align to the next element of "array", assuming a variable width array.  The alternative is to simply use array[index] in the expressions, and let the compiler figure it out -- which if your loop is straightforward and optimization is on (I forget which levels will yield this result), will end up using the same load-and-pointer-increment instructions as intended.

Tim

[nctnico]

Your approach is fine, don't overthink it, just do a cast: "ptrLEDArray = (char *)LEDArray;".

Very bad advice! The pointer to the character array may not be word aligned so the CPU may throw an exception or just read the wrong value (and if not now then it may happen later when someone . The C compiler throws a warning for a good reason! If you want to do this at least use memcpy() to copy the data into the memory. If you are sure endianess is never a problem then this is sufficient.

[ataradov]

Very bad advice! The pointer to the character array may not be word aligned so the CPU may throw an exception or just read the wrong value

We are casting down to bytes and use it as a byte array.  There are no alignment issues here.

[nctnico]

Very bad advice! The pointer to the character array may not be word aligned so the CPU may throw an exception or just read the wrong value

We are casting down to bytes and use it as a byte array.  There are no alignment issues here.

You are right. I misread that part. It is just that you often see people casting arrays or structures over byte arrays and run into trouble.

[newbrain]

Code: [Select]

int array[len];
uint8_t low, high;
int* big_p = array;
...
low = (*big_p) & 0xff;
high = (*big_p) >> 8;
big_p += sizeof(array[0]);


I still think you meant a different thing, i.e., seeing the declarations:

Code: [Select]

// ...
big_p++;
Incrementing big_p, a pointer to int, of sizeof(int), probably 4 in a 32 bit arch, means that 3 elements of the array are skipped, after processing half of the first one...not that it's wrong in itself, but, humor me, it looks very odd in the context.

[T3sl4co1l]

Right, the pointer math is done for you when you use a non-byte-typed pointer...

Call that big_p++ then!

Tim