AVR-GCC: bug or my fault?

Go To Last Post
16 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

AVR-GCC 4.3.3 (WINAVR):

#include 

uint8_t data( void )
{
  return PINB;
}


uint32_t wrong( void )
{
  return (uint32_t)(data() << 8);
}


uint32_t right( void )
{
  return (uint16_t)(data() << 8);
}

Please can anybody enlighten me, why the function "wrong()" gives me the wrong result.
It make a sign extension, but all variables are unsigned. :?: :?:

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This is easy.

uint32_t wrong( void )
{
//  return (uint32_t)(data() << 8);
//    (data() << 8) is evaluated with no sign extension
//                  but the result is a 'standard' C type
//                  i.e. an int
//                  so when you cast the 'int' to uint32_t
//                  you get the sign extension.
//                  e.g. 0x80u -> 0x8000u -> 0xFFFF8000
//
  return (uint32_t)(data()) << 8;  // this should be fine
}

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The result of data() is promoted to a signed int (because that is the minimum type for each operand). At this step no sign extension happens because the original type is unsigned. Because data() is a (promoted) signed int, the result of "data() << 8" is a signed int too. The unwanted sign extension happens then at the step from signed int to uint32_t.

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Welcome to integer promotions.

In wrong(), uint8_t is first converted to int, which is int16_t, then the shift is performed (on the signed int), and only after that the result (which is int16_t) is converted to uint32_t. This is where the surprising sign extension happens.

In the second case, when you cast the implicit int16_t into uint16_t, no sign extension happens, and uint16_t into the return type uint32_t is purely unsigned hence no problemo.

You might want to

return (((uint32_t)data) << 8);

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you for the explanation.

So in conclusion, whenever an implizit conversion was made, it was done to signed, regardless of the signedness of the operands.
Really crazy and a pitfall.

I faced this problem, if I tried to read out a 24bit ADC.
On the upper and the lower byte no sign extension occur.
Only on the middle byte I got the wrong reading.

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

danni wrote:
So in conclusion, whenever an implizit conversion was made, it was done to signed, regardless of the signedness of the operands.
Really crazy and a pitfall.

This comes from the same philosophy as "int is the natural width of the given machine". It results in the simplest possible program, at the cost of potential confusion. But that's C - a bunch of ad-hoc rules, usually in the "hack it up with the least effort" spirit, codified by ANSI/ISO ex-post.

But, if you think about it, this works most of the time as expected, unless you try to cast the result into a longer unsigned type. There is a reasoning in the Derek Jones book why sign extension in that case is a good thing, but I forgot it already and have not the pdf at hand now. If you want to chew on the standard, the applicable verse is 6.3.1.3 #2 (the procedure described there is not explicitly called sign extension, but the effect is the same).

danni wrote:
I faced this problem, if I tried to read out a 24bit ADC.
On the upper and the lower byte no sign extension occur.
Only on the middle byte I got the wrong reading.

If you have this particular snippet at hand, I would like to see it, thanks.

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
So in conclusion, whenever an implizit conversion was made, it was done to signed, regardless of the signedness of the operands.
Not quite. It is promoted to an int if all possible values of the original type can be expressed by a signed int (which going from an 8 bit int to a 16 bit signed int is always the case), otherwise it is promoted to a unsigned int.

If you want to preserve the signedness, then do as wek said above and make it an explicit cast rather than an implicit one.

Regards,
Steve A.

The Board helps those that help themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

wek wrote:
If you have this particular snippet at hand, I would like to see it, thanks.

The working code:

uint32_t ADS1241_read_adc( void )
{
  uint32_t val;

  xBUS_CS0 = 0;
  bus_spi( 0x01 );                              // read command
  _delay_us( READ_DELAY );
  val = (uint32_t)bus_spi( 0 ) << 16;           // high byte
  val |= (uint16_t)bus_spi( 0 ) << 8;           // mid byte
  val |= bus_spi( 0 );                          // low byte
  xBUS_CS0 = 1;
  if( val & 0x00800000 )                        // sign extension:
    val |=  0xFF000000;                         // 24 bit -> 32 bit
  return val;
}

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your code looks wrong to me. I will assume that bus_spi() returns an uint8_t.

  val |= (uint16_t)bus_spi( 0 ) << 8;           // mid byte 
// assuming that bus_spi(0) is returning 0x89
// (uint16_t)bus_spi( 0 ) => 0x0089
// 0x0089 << 8 => 0x8900
// val |= 0x8900 => val |= 0xFFFF8900

As suggested earlier, you can get your desired result with either of these:

  val |= (uint32_t)bus_spi( 0 ) << 8;           // mid byte 
  val |= (uint16_t)(bus_spi( 0 ) << 8);         // mid byte 

I would guess that both solutions will generate no more code than your version if the optimiser is working well.
But this is fairly irrelevant. The important point is to follow the semantics of a language correctly.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:
Your code looks wrong to me. I will assume that bus_spi() returns an uint8_t.

  val |= (uint16_t)bus_spi( 0 ) << 8;           // mid byte 
// assuming that bus_spi(0) is returning 0x89
// (uint16_t)bus_spi( 0 ) => 0x0089
// 0x0089 << 8 => 0x8900
// val |= 0x8900 => val |= 0xFFFF8900


No, David, Peter's code is correct. There is no sign extension in this case, as the conversion is from a smaller-width unsigned (result of the shift is uint16_t) to a larger width one (uint32_t, which can hold all possible values of uint16_t).

Yeah, those pesky integer promotion rules... ;-)

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Next time, I had better compile and simulate some code before I spout off!

I would still be happier with the more conventional casts. It does not cost much typing. Nor gives you brain-ache.

I would possibly code a 24-bit ADC:

uint32_t ADS1241_read_adc( void )
{
  uint32_t val;
  uint8_t bytes;
  xBUS_CS0 = 0;
  bus_spi( 0x01 );                   // read command
  _delay_us( READ_DELAY );
  for (val = 0L, bytes = 3; bytes--; ) {
      val <<= 8;
      val |= bus_spi(0);
  }
  return val;
}

No casts. Adjustable for the # bytes. Not the most efficient.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

No casts. Adjustable for the # bytes. Not the most efficient.


I usually do that kind of thing with a union. Purists object to that as well, but the "load" sequence is easily adjustable for MSB/LSB first.

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hmmmm, will face exactly same issues with 24-bit Analog Devices I2C-interfaced ADC.

Wish there is a way to "tag" this thread for later reference. Yes, I can make a local "printable" copy but that will not have the benefit of later added wisdom!

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Jim,

The rules about sign-extension, casting etc can all get a little fiddly.

IMHO, a simple loop and shift is intuitive. And you just rearrange the loop for LSB first or MSB first.

The important point is to declare your spi() as uint8_t. Or if necessary cast in one place.

danni was seeking an efficient solution, but this is seldom necessary. Compilers are often more clever than humans.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I had the same kind of problem (writing RGB to a colour display), I was a bit disapointed that a >>8 and >>16 to a char (all unsigned) actually made a ror and not just loaded the needed byte! So I made a note so when I need to speed it up the plotdot will be made with a union or in ASM.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:

danni was seeking an efficient solution, but this is seldom necessary.

No, it was the first straightforward attempt, not very effective.

The loop approach contain less pitfalls and is more effective (less code):

uint32_t ADS1241_read_val( void )
{
  uint32_t val = 0xFFFFFF00;

  xBUS_CS0 = 0;
  bus_spi( 1 );
  _delay_us( 20 );
  do{
    val <<= 8;
    val |= bus_spi( 0 );
  }while( val & 0x80000000 );
  xBUS_CS0 = 1;
  if( val & 0x00800000 )
    val |=  0xFF000000;
  return val;
}

Peter