Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MagneticSensorI2C uses incorrect bit mask calculation #402

Open
runger1101001 opened this issue May 21, 2024 · 4 comments · Fixed by #412
Open

MagneticSensorI2C uses incorrect bit mask calculation #402

runger1101001 opened this issue May 21, 2024 · 4 comments · Fixed by #412
Assignees
Labels
bug Something isn't working
Milestone

Comments

@runger1101001
Copy link
Member

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 );
@runger1101001 runger1101001 added the bug Something isn't working label May 21, 2024
@askuric
Copy link
Member

askuric commented May 22, 2024

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
Copy link
Member

askuric commented May 22, 2024

Hey Richard,

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

And here is the AS5048B
image
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.

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

@runger1101001
Copy link
Member Author

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:

image

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
Copy link
Member

askuric commented May 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants