Shifting for Bitmasking Fails

My understanding is, that when you shift 1's out the left of a variable, and then shift back right, zeros will return in their place.

Working on a bit of a github library for the MCP9808 driver that appears to be broken, I've narrowed the problem down to this statement:

uint8_t regs[SIZEOF_REGS]; 
uint16_t tAmbient;
tAmbient  = ( ( regs[8] << 11 ) + ( regs[9] << 3 ) ) >> 3;

From the datasheet, it confirms that the objective of that line is to combine two bytes into an uint16_t and mask the three uppermost bits (to zeros).

BUT... it doesn't work. I still get ones in the upper three bits.

I think that the value of this expression,

( ( regs[8] << 11 ) + ( regs[9] << 3 ) )

since it's not typecast to anything, gets the default type of int. Then when it's shifted right, the sign bit is propagated to the right.

You can test this by typecasting that expression to uint16_t before shifting the whole thing right by 3, and seeing if it does what you expect.

To avoid having to worry about all that, you might consider shifting the reg[8] to the left by 8 bits, OR'ing with reg[9], and AND'ing the whole thing with a mask that clears the most significant bits. I think it would run faster, too.

I seriously doubt that Christensen's library is 'broken'.

MCP9808 library

More likely the problem is elsewhere.

Can you give me an idea where the problem might be?

Here is a test with 1.6.8:

uint8_t test_regs[2]; 
uint16_t test_tAmbient;

test_regs[0] = 0xFF;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = ( ( (uint16_t)test_regs[0] << 11 ) + ( (uint16_t)test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

Output:

FFFF
1FFF
FFFF

Why don't you use something that does not need explicit casting to some unsigned type?

tAmbient = ((regs[8] << 8) | regs[9]) & 0x1FFF;ortAmbient = ((regs[8] & 0x1F) << 8) | regs[9];

jecottrell:
Can you give me an idea where the problem might be?

have you thought about using simple union?

union Data{
  uint8_t regs[2];
  uint16_t reading;
};

Data data;

void setup() 
{
  Serial.begin(9600);
  data.regs[0] = 0xff;  // low byte
  data.regs[1] = 0xff;  // high bute
  data.reading &= 0x1FFF;  // set three most significant bits to zero
  Serial.println(data.reading, HEX);
}

void loop() 
{

}

I can make it work, my goal is to find and understand why it doesn't. As OldSteve said...

OldSteve:
I seriously doubt that Christensen's library is 'broken'.

More likely the problem is elsewhere.

Uncasted expressions are treated as signed ints.

Signed fields are filled with the sign bit on arithmetic right shifts.
Unsigned fields are filled with zeros on arithmetic right shifts.

That is exactly what your inefficient masking produces.

Where do you see a problem with that?

John,

The compiler expands the byte values to 16-bits before performing the shifts. I haven't examined the assembly code, but the optimizer might be fooling with the shifts also. Does this give the expected result?

test_tAmbient  = ((uint16_t) ( test_regs[0] << 11 ) + (uint8_t)( test_regs[1] << 3 ) ) >> 3;

Thank you for this explanation. That's what I was looking for. I will try to find more to read to be able to apply it.

Whandall:
Uncasted expressions are treated as signed ints.

Signed fields are filled with the sign bit on arithmetic right shifts.
Unsigned fields are filled with zeros on arithmetic right shifts.

But, looking closer and trying to understand your explanation, why doesn't the first uncast test return a 0x1FFF? "Signed fields are filled with the sign bit on arithmetic right shifts." The sign bit would be '0' in the case of 0x7F, no? When is the sign bit "checked", before the left shift or before the right shift? My tests are appearing to indicate "right shift of signed values is undefined."

test_regs[0] = 0x7F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = ( ( (uint16_t)test_regs[0] << 11 ) + ( (uint16_t)test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
7FFF
1FFF
7FFF

So I did the following test see the behavior depending on what the "boundary bit" was after the left shift.

Jim, it looks like your early cast to an uint8_t, removes the upper three bits of the LSB.

test_regs[0] = 0x7F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = ((uint16_t) ( test_regs[0] << 11 ) + (uint8_t)( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
Serial.println();

test_regs[0] = 0xFF;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = ((uint16_t) ( test_regs[0] << 11 ) + (uint8_t)( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
Serial.println();

test_regs[0] = 0x1F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = ((uint16_t) ( test_regs[0] << 11 ) + (uint8_t)( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
Serial.println();

test_regs[0] = 0x0F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = ((uint16_t) ( test_regs[0] << 11 ) + (uint8_t)( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
7FFF
1FFF
1F1F

FFFF
1FFF
1F1F

1FFF
1FFF
1F1F

FFF
FFF
F1F

Here's a better test of the boundary bits:

uint8_t test_regs[2]; 
uint16_t test_tAmbient;

test_regs[0] = 0x0F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
Serial.println();

test_regs[0] = 0x1F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
Serial.println();

test_regs[0] = 0x2F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
Serial.println();

test_regs[0] = 0x3F;
test_regs[1] = 0xFF;

test_tAmbient  = ( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);

test_tAmbient  = (uint16_t)( ( test_regs[0] << 11 ) + ( test_regs[1] << 3 ) ) >> 3;
Serial.println(test_tAmbient, HEX);
FFF
FFF

1FFF
1FFF

2FFF
FFF

3FFF
1FFF

So.... I guess it's a core difference. :-[

Test was:

Val - simple shift and add to combine two uint8_t into an uint16_t
Chr - Christensen's shift scheme
Cst - Christensen's shift scheme with the cast to uint16_t

AVR (1.6.8 & 1.6.4)

Val: FFF
Chr: FFF
Cst: FFF

Val: 1FFF
Chr: FFFF
Cst: 1FFF

Val: 2FFF
Chr: FFF
Cst: FFF

Val: 3FFF
Chr: FFFF
Cst: 1FFF

SAMD21 (1.6.8)

Val: FFF
Chr: FFF
Cst: FFF

Val: 1FFF
Chr: 1FFF
Cst: 1FFF

Val: 2FFF
Chr: 2FFF
Cst: FFF

Val: 3FFF
Chr: 3FFF
Cst: 1FFF

Solved!

The library's author, Jack Christensen has helped with the issue I posted to the github library. He speculated it wasn't a core issue but the ARM's native type of int32_t as opposed to the AVR's int16_t. A simple cast of int16_t fixed everything up.

The cast of uint16_t was just a red herring.

All this behaviour and how to fix it was explained in #1.

Whandall:
All this behaviour and how to fix it was explained in #1.

Is this the fix?

tmd3:
To avoid having to worry about all that, you might consider shifting the reg[8] to the left by 8 bits, OR'ing with reg[9], and AND'ing the whole thing with a mask that clears the most significant bits. I think it would run faster, too.

Is this the correct form?

(( ( test_regs[0] << 8 ) |  test_regs[1] ) & 0x1FFF ) >> 3;

jecottrell:
Is this the correct form?

(( ( test_regs[0] << 8 ) |  test_regs[1] ) & 0x1FFF ) >> 3;

No. It is
(( test_regs[0] << 8 ) |  test_regs[1] ) & 0x1FFF;or (even faster)

((test_regs[0] & 0x1F) << 8) | test_regs[1];

This is the original code that works on AVRs and not on ARMs:

( ( regs[0] << 11 ) + ( regs[1] << 3 ) ) >> 3;

tmd3's (and your recommendation):

(( test_regs[0] << 8 ) |  test_regs[1] ) & 0x1FFF;

Doesn't achieve the same end, I don't think. The goal isn't merely to mask the 3 most significant bits. It is, to take a signed 13 bit int that is embedded between bits 0 and 12, convert it to an int16_t and subtract it from 256 if it is negative. jchristensen achieved all of that with the single line... IF the native type is int16_t as in the AVR. When compiling for the ARM, the line has to be cast from the native int32_t to int16_t for the code to work.

jecottrell:
From the datasheet, it confirms that the objective of that line is to combine two bytes into an uint16_t and mask the three uppermost bits (to zeros).

jecottrell:
The goal isn't merely to mask the 3 most significant bits. It is, to take a signed 13 bit int that is embedded between bits 0 and 12, convert it to an int16_t and subtract it from 256 if it is negative.

You should make up your mind.

Last night after working on it most of the day yesterday, that was what I thought to be the total purpose of that line. However, since then I've learned it does significantly more.

I apologize for the confusion.