Hi there -- sorry to bug you with a newbie question, but I'm a hardware design engineer, so software sometimes leaves me a bit confused.

Can you take a quick glance at the following example functions and tell me if the casts (highlighted in yellow) are (a) correctly done and (b) necessary, or would the compiler do them anyway?

Similarly, are the parentheses (highlighted in green) necessary? Even if they aren't necessary, are they still a good idea for ensuring things happen as I want them to happen?

www.clivemaxfield.com/area51/do-not-delete/casting...

Thanks as always for your sage advice :-)

Is this C or C++ (I'm assuming C)?

Is this a 16bit or 32bit platform? (i.e. what is the size of the int)

So, for example, if I try to compile that code for gcc MSP430, I get an error:

https://godbolt.org/z/TY2y9x

But, if you change the compiler to a 32bit platform, then it compiles fine. Check out my link above and play with it. If you haven't seen godbolt.org before, it is really an amazing utility to help with answering questions like this AND compiling the code across various compilers and targets.

Also note the compiler options I selected: "-O2 -Wall -Werror -Wextra".  This turns on many warnings that the gcc compiler may find, and furthermore turns those warnings into compile errors. Very much recommended.

FWIW: Casting is generally considered a sign of a design issue and should be considered a code smell. However, in our embedded world it is frequently needed. Therefore that general best practice advice is less applicable.

Also, the question at its core is regarding integer promotion rules in the C language: see here for some nap time reading:

https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules#:~:text=The%20rank%20of%20any%20unsigned,signed%20char%20and%20unsigned%20char%20.

Best regards,

Matthew

https://covemountainsoftware.com/services/source-code-review/

Sorry -- I should have said -- it's my pathetic attempt at C.

The target processor dev board is a Seeeduino XIAO, which has a 32-bit Arm Cortex-M0+ (and which is awesome for the price of only $4.90)

But I just tried compiling it for the 8-bit Arduino Uno, which has 16-bit ints, and it compiled just fine there also (I'm using the Arduino IDE).

I haven't used the Arduino tools in many years. Check the project's compiler settings. It likely compiled fine due to the lack of enabled warnings as errors. 

uint32_t BuildColor (int red, int green, int blue) 
{ 
   return ( (uint32_t) (red << 16) ) | ( (uint32_t) (green << 8) ) | ( (uint32_t) blue );
}

Given the use of 'stdint' types (uint32_t, etc) in most of the code, perhaps change the above to (I'm assuming the colors are actually limited to uint8_t ):

uint32_t BuildColor (uint8_t red, uint8_t green, uint8_t blue)
{
 return ((uint32_t)red << 16) | (  ((uint32_t)green << 8) ) | ( (uint32_t) blue );
}

Notice that I still needed to cast 'red', etc, from uint8 to uint32 to avoid the warning/error on shifting 'red' beyond its native size. Without that cast, I get (with MSP430 gcc):

source>: In function 'BuildColor':
<source>:25:14: error: left shift count >= width of type [-Werror=shift-count-overflow]
  return (red << 16) | (  ((uint32_t)green << 8) ) | ( (uint32_t) blue );
              ^
cc1: all warnings being treated as errors
Compiler returned: 1

Given that it is only a warning, which I have moved to 'error' status as a matter of best practices, the compiler would have still generated code that is likely correct and works fine.

Or, better yet, define your color types in some manner:

https://godbolt.org/z/NYyWYH

That would perhaps make it easier to change the types in the future if needed. Check that link to note where I removed casts too...

Best regards,

Matthew

https://covemountainsoftware.com/services/source-code-review/

OK, in the following statement, tmpRed has been declared as a uint32_t. Also, I've modified the GetRed() function to return a uint8_t.

tmpRed =   ( (  GetRed(startColor) * (numSteps - currentStep) ) + (  GetRed(endColor) * currentStep) ) / numSteps;

I multiply the value returned from GetRed(), which can generate a result that's bigger than 8-bits. Do I need to cast the value returned from GetRed() before performing the multiplication, or will the compiler do thsi automatically because it knows the result is going to be assigned to a uint32_t ?

The compiler will do it automatically. This topic is about integer promotion rules and implicit type conversions, which are involved, but for the most part follow some reasonable natural expectations. 

Some useful reading:

https://en.cppreference.com/w/c/language/conversion

https://stackoverflow.com/questions/46073295/implicit-type-promotion-rules

https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules#:~:text=The%20rank%20of%20any%20unsigned,signed%20char%20and%20unsigned%20char%20.

=======

FWIW, In my example, I kept 'tmpRed' as uint32 because I didn't want to fully analyze the algorithm to determine the output range. If the algorithm allows for results greater than 255, then those bits will be lost when passed to BuildColor(..), so it probably would have been fine for it to be uint8. But, it depends on the goal. Maybe overflow is expected/desired, or maybe it should be clipped. If clipped, then you would want uint32 for those tmp variables, and then some additional code to clip the value to 255.

Good point, but the way the algorithm works is that the worst case will be to have GetRed() always return values of 255. In this case, we will have an intermediate value of:

(255 * (numSteps - currentStep)) + (255 * currentStep)

Which is the same as 255 * numSteps

At the end we divide by Numsteps, so the final result in tmpRed will always be <= 255

Hello Max,

Though I’m still an amateur, I’ll share my opinion based on what I’ve picked up from self-study. Don’t take what I say on faith, but run some experiments with your compiler to see what it outputs. I recommend turning on the optimizer and dumping human-readable assembly (-S flag is you’re using GCC or Clang).

1.  It appears that each single color is represented by a single byte. I recommend changing all the ints to uint8_t.
2. The casting in the Get* functions looks alright to me, but in the BuildColor function you should cast and then shift, e.g. ((uint32_t)red << 16). The cast operation has higher precedence than the shift so you don’t need additional parenthesis here.
3. According to the rules for C operator precedence, bit-wise shifting will occurs before bit-wise OR, so the green parentheses in BuildColor are not strictly necessary. I do like them, however, as they make the code clearer.
4. In the CrossFadeColor function, you shouldn’t need the green parenthesis around each cast when calling BuildColor in the return statement. Also, rather than casting in the function call, you could make each tmp* variable a uint8_t type and doing the cast on the right side of the assignment.

Though not part of your request, I have to ideas for changes to the code which I thought I might share. 

• If your compiler cannot inline the Get* functions called inside CrossFadeColor, you may consider turning each Get* into a macro to avoid function call overhead on what is essentially bit-shifting/masking operations.
• Instead of using a uint32_t to pack the colors together, have you considered using a struct?
  struct RGBColor {
      uint8_t red;
      uint8_t green;
      uint8_t blue;
  };
  this way you could skip the shifting and masking and simply write function calls like this.
  uint8_t GetRed(const struct RGBColor* color) {
      return color->red;
  }

I hope this helps!

Hi there, this helps a lot -- thanks so much -- Max

Hello Max,

First off I would say never mix the bit number explicit defs like uint32_t with int in the same module.

Secondly GetGreen etc are declared unhelpfully - what you have will work but if a function returns a uint8_t why not declare it as such ?

In BuildColour you set yourself another trap, red green and blue are obviously unsigned - so why not declare them as what they are ? The casts should then be like:

(uint32_t) red << 16, does << bind tighter than (cast) ? 

why take the chance:

((uint32_t)red << 16)

In the last function, if tmpRed is a uint32_t then declare it as such (you did) and write BuildColor to accept it as it is.

So I think pretty much all of your casts should really be coded away by making things match. The only area of doubt is in BuildColor where you are shifting uint8_t elements into a uint32_t. 

Hope this helps.

MK

Hi Mike -- do you have a minute to discuss this (256 970 1906)?

That's a good point -- you suggest:

((uint32_t)red << 16)

But wouldn't it be better to use:

((uint32_t) red) << 16

Max

Woops ! Yes !

MK

Hi, MaxMaxField!

Glad to read from you again!

Your doubts are not only the doubts of many of us as they also end revealing much useful information ( see below ).

I fully agree with Matthew, and I thank him for the valuable links.

And I add that parenthesis should always be used. Never rely on compiler precedence rules is one of the mantras of embedded software development.

Cheers!

Well, your example contains these:

(uint32_t) (red << 16)

where red is int. What you told the compiler was 'shift the signed int red by 16 bits then convert the result to a 32-bit unsigned quantity'. If on your platform int happens to be 16 bits, then that is not good, for 2 reasons: a) you are shifting by the width of the operand, which is the dreaded Undefined Behaviour and b) even if the compiler does not do anything nasty, shifting a 16-bit word to the left by 16 bits results in a constant 0.

The undefined behaviour might even be triggered by the green channel:

green << 8

because if green >= 128, then the shifting will set the top bit of the 16-bit word and that is UB by the standard.

What you wanted to tell the compiler was 'convert the signed int red to a 32-bit unsigned and then shift it by 16 bits:

(uint32_t) red << 16

Then, for the bitwise OR you need other parenthesis:

((uint32_t) red << 16) | ((uint32_t) green << 8) | (uint32_t) blue;

No need to cast the result back to int, that will be done by the compiler automatically (the return statement automatically casts the returned value to the return type of the function). Which, by the way, on a 16-bit platform would mean that you lose the red channel, as your 24-bit composite colour would be truncated down to a 16-bit int.

If you are working on a 32-bit platform, then none of the casting is necessary, as your composite colour will always fit on 24 bits, which is always positive so no problems with shifting and or-ing or anything. 

However, it would generally be a good practice to declare the types:

typedef uint8_t colour_channel_t;

typedef uint32_t composite_colour_t;

(with some more sensitive names, of course) and explicitly cast channel colours to composite before the shifting and the or-ing. the  because that would give platform independence for your code. It is more typing (once) but generates no extra code and works no matter what platform you have.

Awesome -- thanks so much for taking the time to explain all of this -- Max