search

simplefoc / Arduino-FOC

Arduino FOC for BLDC and Stepper motors - Arduino Based Field Oriented Control Algorithm Library
https://docs.simplefoc.com
MIT License
1.94k stars 510 forks source link

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

Open ys1374 opened 1 month ago

ys1374 commented 1 month ago

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

runger1101001 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 (https://github.com/simplefoc/Arduino-FOC/issues/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 commented 1 month ago

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.