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

The problem of AS5600 Sensor resolved Finally!!!!! #401

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

ys1374
Copy link

@ys1374 ys1374 commented May 21, 2024

The falling behind Problem of AS5600 Magnetic encoder is taking care of in this clone of SimpleFoc library, Enjoy!

@runger1101001
Copy link
Member

Hey,

We only merge PRs to the dev branch of the library, I have rebased your PR.

But as I wrote in the forum, these code changes do not really make sense to me:

Firstly, you change the handling of the msb and lsb masks is the constructor. I think your fix may actually be correct here, I can’t quite figure out at the moment why the code is shifting 0x2 instead of 0x1 like your version. It may be an error. It would not actually make a difference to the AS5600 LSB mask, but for the MSB it looks like an extra bit is being included that should not be.

But anyways it does not matter, since later on in the code you drop the use of the bit masks altogether:

while (wire->available() < 2); // Wait for 2 bytes to become available

// Read the two bytes
readArray[0] = wire->read(); // High byte
readArray[1] = wire->read(); // Low byte

// Combine the bytes into a single 16-bit value
readValue = (readArray[0] << 8) | readArray[1];

Not using the bit masks could cause problems for other I2C sensors, even if it is working for AS5600. The MagneticSensorI2C driver is not supposed to be specific to AS5600.

You add a check for wire->available() to the code - but I am not sure this makes a difference unless the sensor is misbehaving. When you call wire->requestFrom() the data is already exchanged and it is already known how many bytes are waiting in the wire buffer. Normally, if the sensor is working well, it should always be the two bytes that you requested.
If it were less than 2, the original code would get back -1 from wire->read() and give a wrong result.
If it were less than 2, your code would wait forever at this point.

And then changing the readValue calculation to remove the bit masks would also not explain any improvement in your results - at best, the result is the same as before, at worst you now get incorrect data from the extra MSB bits not supposed to be in the calculation…

What do you think is the important change that has made it work?

Also, keep in mind the MagneticSensorI2C is a general class, not specific to the AS5600... so I am not sure we would want to include AS5600 specific code in there, like the PswMagicCodeAS5600I2C() function.

Perhaps if you want to make an improved driver specific to the AS5600 you could put it in our SimpleFOC Drivers library? Its where we keep hardware specific code...

@runger1101001 runger1101001 changed the base branch from master to dev May 22, 2024 07:03
@runger1101001 runger1101001 added enhancement New feature or request invalid This doesn't seem right labels May 22, 2024
@runger1101001
Copy link
Member

Hey, unfortunately there is also another problem with the code, it won't compile on STM32, RP2040, SAMD or ESP32...
Which MCU were you using for testing?

I have to say at this point that we really do appreciate contributions, and we are grateful for the time you took to look into this, and for making the effort to contribute it back.
I realise my first message may sound a bit negative - I did not mean it this way, we really are grateful for every contribution. But of course we also have to make sure the contributed code works, and doesn't cause problems for other users who may not have the same hardware setup.

So for all the reasons listed above I don't think we can merge this PR as it is.

We have started work on a hardware-specific dedicated AS5600 driver here: https://github.com/simplefoc/Arduino-FOC-drivers/tree/master/src/encoders/as5600
It is not yet finished, but perhaps you would like to base your work on this one instead?

@ys1374
Copy link
Author

ys1374 commented May 23, 2024

Hey Richard,

Apologies for the delayed response. Your first reply to my message in the community scared me a bit 😅, as I am an amateur in this field, and this was my first contribution. I should have double-checked everything. It seems that I uploaded the wrong version I was working on—sorry about that. I was exhausted from working with this sensor, and it was late at night.

I've now uploaded the proper version, and I didn't change the library much. As I mentioned in the community, the problem is not from the sensor side. This particular sensor tries to pull as low current as possible, and I saw here (#402) that you mentioned "we would want bits 7..2", so you're simply ignoring the power mode bits. Figure 6 of the datasheet shows that if you don't define this, it defaults to 11, and in this mode, the polling time is 100ms, which made the library fail to continuously update the reading angle; just put this to 00 or 01 and that would resolve the problem.

As for my code, I was just trying to make it work, and you are much more familiar with the library than I ever will be. Please incorporate my code or your version of it into the library so that it doesn't ignore those two bits.

Capture

@ys1374
Copy link
Author

ys1374 commented May 23, 2024

Regarding the idea of an AS5600 driver, I appreciate the suggestion and will definitely work on it to save people time when dealing with this sensor. Thank you for mentioning it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants