MagneticSensorI2C uses incorrect bit mask calculation about arduino-foc HOT 4 OPEN

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!

from arduino-foc.

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.

from arduino-foc.

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?

from arduino-foc.

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

from arduino-foc.