MagneticSensorI2C uses incorrect bit mask calculation

[runger1101001]

https://github.com/simplefoc/Arduino-FOC/blame/6fafa35b73e9c5ba6f5d9259640a5ed328dc2750/src/sensors/MagneticSensorI2C.cpp#L40

  lsb_mask = (uint8_t)( (2 << lsb_used) - 1 );
  msb_mask = (uint8_t)( (2 << bits_used_msb) - 1 );

Why are we shifting 2 here? It does not seem correct.

Some examples: 2 is 00000010b in binary

lets say lsb_used is 6, i.e. we would want bits 7..2 of the LSB, with bits 0 and 1 not used. But this calculation would give us:
00000010b << 6 = 10000000b
10000000b -1 = 01111111b and we get bits 6..0 instead

lets say msb_used is 4, i.e. we would want bits 3..0 of the MSB, but the calculation gives us:
00000010b << 4 = 00100000b
00100000b -1 = 00011111b and we get bits 4..0 instead

This is just wrong. It was probably never noticed for two reasons. Firstly, the present calculation, though incorrect, still gives the correct mask for the LSB if all 8 bits are used. Secondly, although the MSB mask is wrong, it appears the AS5600 actually always returns 0 for the incorrectly included bits, and so no one has ever really noticed this...

The correct calculation should be:

lsb_mask = (uint8_t)( ( (0x1 << lsb_used) - 1 )  << (8-lsb_used) );
msb_mask = (uint8_t)( (0x1 << bits_used_msb) - 1 );

[askuric]

Oh wow!
This is a big one.
I don't remember what was my thinking any more. :)
This was written at the very early stages of simplefoc :D
The interesting part is that it has never been noticed though!

[askuric]

Hey Richard,

I've looked into it a bit and here is the structure of AS5600 :

datasheet page 19

And here is the AS5048B

datasheet page 25

So according to these datasheets if I understand correctly it seems like both MSB and LSB bits are read from 0 to number of bits. So for example if we have the lsb_used =6 we really need to read form 0..5, not from 7..2.

So in that case the mask should be

lsb_mask = (uint8_t)( (1 << lsb_used) - 1 );

I think that the reason why I made the mistake here is because I was trying to exploit the relationship that if we want to select last 3 bits, the bit mask corresponds to 2^3-1 =7 = 0b111. Probably I thought that 2^3 = 2<<3, which is not true. :D

So I guess that the reason why this worked for AS5600, where we have 8 bits in the LSB is because the 2<<8 = 0. When you shift outside the range the c code returns 0.

So I think that we should have

lsb_mask = (uint8_t)( (0b1 << lsb_used) - 1 );
msb_mask = (uint8_t)( (0b1 << bits_used_msb) - 1 );

And this could explain a bit why we did not see this before. Because these sensors return 0 outside msb and lsb returned. So the mas was wrong, but the error was not too big, it only added leading zeros. And these lading zeros were corrected by these lines.

Arduino-FOC/src/sensors/MagneticSensorI2C.cpp

Lines 114 to 115 in 6fafa35

	readValue = ( readArray[1] & lsb_mask );
	readValue += ( ( readArray[0] & msb_mask ) << lsb_used );

[runger1101001]

And this could explain a bit why we did not see this before. Because these sensors return 0 outside msb and lsb returned. So the mas was wrong, but the error was not too big, it only added leading zeros. And these lading zeros were corrected by these lines.

Yes. I agree with this analysis... all in all the API is a bit confusing... in the I2CSettngs we provide a start_pos and number of bits, but the layout of these bits could be different in different sensors.

Looking at the MT6701, another sensor supporting I2C, for example, we have this kind of layout:

So here the lsb is not right-aligned (to 0) but rather left-aligned on bit 7 and "contiguous" with the MSB if we think of the memory as a "string of bits".

So the same logic can't cover the MT6701 and the AS5048A, unfortunately.

Since we already have a PR to add MT6701 support (#398) I think we'd better come up with a fully general API that can cover all three sensors?

[askuric]

I agree completely!
It's too complicated for what it is and not flexible enough.