Infineon / TLI493D-W2BW

MIT License
3 stars 3 forks source link

concatResults function is wrong for temperature data #2

Closed Riesi closed 1 year ago

Riesi commented 1 year ago

Hello! When using the concatResults code as a reference I found that the code path for the temperature conversion looks quite wrong. https://github.com/Infineon/TLI493D-W2BW/blob/master/src/Tli493d.cpp#L567-L574

upperByte contains Bit 11-4 and is not ambiguous lowerByte only contains Bit 3 and 2 with two layouts which both don't work with the given code Layout A) Bit 3&2 are in the LSBs so XXXX XX32 Layout B) They are in the MSBs still 32XX XXXX

The bit mask is 0xC0 = 0b1100 0000

1) value = (uint16_t)upperByte << 8;
2) value |= ((uint16_t)lowerByte & 0xC0) << 6;
3) value |= 0x06; //append bit 1 and 0
4) value >>= 4; 

1) value's upper byte gets set to upperByte. 2) Masking lowerByte with 0xC0 will only keep the two MSB resulting in 0000 0000 or 3200 0000 depending on the given layout. Casting this to 16Bit and shifting 6 to the left results in either a value of 0 or 0032 0000 0000 0000. ORing this with value will do nothing or collide upperByte bits with lowerByte bits, which seems wrong. 3) ORing 0x06 = 0b0000 0110 to value 4) Right shifting of value by 4 makes step 3) irrelevant

I assume the correct version assuming lowerByte contains the temperature bits in its LSBs (layout A) should look like this:

value = (uint16_t)upperByte << 8;
value |= ((uint16_t)lowerByte & 0x03) << 6;
value >>= 4; 

Assuming layout B:

value = (uint16_t)upperByte << 8;
value |= ((uint16_t)lowerByte & 0xC0) ;
value >>= 4; 

I would do a PR, but I am not sure what your layout is, so I wanted to discuss this first.

Kind regards, Riesi

9Volts9er commented 1 year ago

Hi Riesi,

sorry for the late reply. You are totally right, I've already fixed this with a commit ~2 weeks ago. I just forgot to reply to this issue, I'm very sorry.

The problem was the same in the "parent" library code, I've fixed it there as well: TLE493D-3DMagnetic-Sensor

Thank you very much for reporting this. I will close this issue for now, if there is anything else, feel free to reopen it.