check for floating point error due to insufficient precision

[ATILIUS-REGULUS]

At BLDCMotor::velocityOpenloop(float target_velocity) was an error due to insufficient precision with the float value shaft_angle.
If a motor run with 0.5 rad/s on a F446RE, the motor stopped after about 14 minutes in the open_loop_velocity_example, when shaft_angle reached a value of 512.00.

[askuric]

Hey Jurgen,
Thanks for exposing this problem and offering a solution.
Since the problem is due to the float32 precision and is due most probably to the fact that in the line:

shaft_angle += target_velocity*Ts

the combination target_velocity*Ts drops under the precision of the float32 after some time.
Now I think that this will almost never happen for Arduino MCU because it cannot achieve such a low sample time Ts.

I am thinking that maybe just having the shaft angle as double variable would be enough to resolve this issue, even though Arduino UNO (atmega328) doesn't have it.

I'll look into it a bit and let you know, because I would like to keep track of the actual shaft angle and not initialize it to 0 during the course of motion control. This will maybe change in the future releases of the library though.

[ATILIUS-REGULUS]

Hello Antun!
Please note that the angle of a shaft, which rotates around a center, measured in radiant is between 0 and 2 * pi.
When the angle reaches 2 * pi, it is the same value as 0 or 4pi, i.e. the angle repeats, or angle = angle mod (2 * pi).
If you want to keep track of how many rounds the shaft have turned, we could use another int varibable "revolutions" maybe and keep the shaft angle between 0 and 2pi.

The reason why I used (_2PI * 4) as a limit and not (_2PI) is, because I had the same problem with 360° spinning servos before.
In the firmware of the manufacturer, the position of the servo was between 0 and 4*360° or so, in order to be able to keep track of the spinning at least for some revolutions.

Anyways, I think that it would be the best, if the shaft angle should stay between 0 and 2*pi and the number of turns should be counted in a int32 varibable.
This way, no error with the float shaft_angle will occure and int32 of revolutions should also cause no problems.
The next days, I will try to program that and send you another pull request, and you can check, if you like it.

Cheers,
Juergen

[ATILIUS-REGULUS]

Hello Antun!
After a deeper look into the library, I found out that angle values occur in most of the source code file, i.e. a larger amount of changes needs to be done.
Also found another general problem with the angle at Encoder.cpp - float Encoder::getAngle () :
"return natural_direction * _2PI * (pulse_counter) / ((float) cpr);".

The coding implies, that the value pulse_counter is incremented all the time and the Angle is calculated by this value.
This will work only for some minutes for a typical bldc motor with around 5000 RPM and a typical encoder with around 4096 CPR, as pulse_counter has 32 bit size, 12 bits are needed per second (4096 CPR), more than 12 bits are needed per minute (5000 RPM), which leave less than 8 bits total, which is less than 256 minutes until an error will occur with the calculation.
Some encoders have even more than 12 bits and some motors run over 10000 RPM, so the error occurs earlier.

Therefore, I think it will be a good idea, to keep the angle between 0 <= angle < 2*pi, and to save the rotations in a separate variable.

Currently I have stated to implement the new rotations variable, but need some more days to finish the programming.
When I am finished, I need to check, if I can update this pull request or if I need to withdraw this pull request and open another one.

[askuric]

Hey Jurgen,

You are right, I am aware of this actually. This is surely a problem, but in a context of this library it was an ok solution. But this has to be changed!

Now, I would suggest you to update the code for your application but not to waste too much time on the pull-request.
The reason why is that instead of introducing a new variable for counting rotations and another for 0-2PI angle, I would rather change the Sensor.h interface and introduce the function getFOCAngle() or something like that that will always return the angle in between 0 and 2PI and use the getAngle() for position control.
Maybe both solutions will be needed to make this run right, I don't know at this moment.

Finally, since this code change requires core changes in the library, and a lot of testing for all types of sensors and drivers. I am not sure I feel comfortable a pull-request without in depth analysis. I hope you understand what I mean.
Unfortunately, I will not be able to spend some time on this issue for few more weeks, so I did not give you much to work with :D
Sorry about that.

If you have time to deal with this I will be happy to see your changes and what works for you, but introducing this to the Simple FOC library might take a bit longer :D

[ATILIUS-REGULUS]

Dear Antun!

Thank you for your feedback, which I understand and have similar thoughts about.

The main problem is the small number of data types in the Arduino environment: https://learn.sparkfun.com/tutorials/data-types-in-arduino/all, which don't have 64-bit data types, i.e. no double and no long long.

Of course, I understand that you'd like to keep a total angle that includes all revolutions, but with a hardware setup that requires 12 bits per second for the encoder and 12 bits (or more) for the motor speed, the left over bits (<= 8) for the minutes are really small.
Some users who run a bldc motor for more than 4 hours (sometimes a lot less) will definitely run into issues with the motor's behavior and don't know the reason, just like I did with the small gimbal motor.

I also understand that core changes in the library will create a lot of effort in testing all types of sensors and drivers.
Unfortunately, I don't have the sensors and drivers to test all of this, otherwise I would.
For my example of the open loop mode, my solution is quite simple as I don't need the total angle including all revolutions.
Any major changes I would only do just to help you, not for myself (since I don't even have an encoder right now) but I understand you don't have the time to check all of this out.

To cut a long story short, I would suggest that I stop making any changes until you decide how to resolve this issue as there is no point making all the changes and no one has time to review and approve the pull request.
My idea would be to change all sensors / encoders so that the pulse_counter (or a similar counter depending on the sensor type) only counts one revolution and keeps the angle within a modulo range of one revolution. Each time the sensor overflows after one revolution, a separate revolution variable is incremented / decremented.
When you find the time to dig deeper into this issue and make a decision on how to resolve it, drop me an email and I'll be happy to support you.

Cheers,
Juergen

[Polyphe]

Hi Everybody,
From my point of view, and that would be maybe the simplest, I think that you should do a getElectricalAngle(); and a getMechanicalAngle(); , and only use the Electrical angle in the FOC loop 0-2PI, and use the Mechanical angle for the position with the maximum accuracy and/or reset this one before reaches the 32 bits.
What would simplify the modification of the code, and waste of time.

Yours thoughts ? (I can do a mistake of understanding too :) )

Best regards

[askuric]

Hey guys,

Yes, I'll revive the pull-request as soon as we updated the internal interfaces and the API. I think it will help me a lot to discuss it with you guys.

@Polyphe the electrical angle is a property of the motor. And in order for the sensor to know it it would need to know the number of pole pairs of the motor. This is feasible, but I would like to avoid it.

Resetting the counter is a good idea and it is inline with what Jurgen has proposed. And we will definitely go deeper into this topic in the next few releases.

What I would like to do, in combination with this change is to add a possibility to combine more position sensors:

• one for foc the other for motion control (ex. encoder + imu)
• one for foc in low speeds the other for high speeds (ex. magnetic + hall)

[Polyphe]

Hi @askuric,
Sorry, but I don't understand what you mean : "And in order for the sensor to know it it would need to know the number of pole pairs of the motor", anyhow, we have to put the number of pole pair through the main program so that the sensors use it.
In fact, I meant in the function of loopFoc() to replace :

shaft_angle = shaftAngle(); --> remove
// set the phase voltage - FOC heart function :)
setPhaseVoltage(voltage_q, voltage_d, _electricalAngle(shaft_angle,pole_pairs)); --> put setPhaseVoltage(voltage_q, voltage_d, getElectricalAngle());
Where this new function "getElectricalAngle()" (in each sensor, magnetic, hall, and so on) come from all the sensors that also use pair pole (mandatory).

What do you think about ?

I think @ATILIUS-REGULUS are totally right about of this issue, because when we reach the limits, the motor misbehaves for the above reasons, as you know.

Best regards.

[askuric]

Hey @Polyphe
What i mean is that getElectricalAngle() would need to provide the pole pairs number to the sensor. The one who has the pole pairs number is the BLDC motor at the moment. And it is not really the important information for the sensor and i would like to keep it that way.
The motor would be able to communicate this value to the sensor but honestly I would not like for this kind of transfer to hapen. Especially since you are not reallyintroducing any new information there.

We are not arguing that this is a problem. I am aware of this and we will fix it. And it is good that you guys point it out so that we don't into it faster. But there is a lot of small things like this one in this library, small checks that have not been handled for the sake of the readability/clarity/simplicity. This library never really had the optimality and long term robustness as it's primary goal. Im sure you've already seen these kinds of trade-off made in code.
But recently this has started to change and we are putting a lot more effort into this. So I promise you that we will implement this better and we will put you in a loop when we start. Now in short term, if you wish to propose a solution, i would be happy to discuss it and if it passes all the sensor tests I'll be happy to incorporate it.

[ATILIUS-REGULUS]

Hello everybody!
Actually the problem is even worse, as a float has 32 bits but the fraction part has only 23 bits, which means, that a bldc motor with 5000 RPM and an encoder with 4096 CPR will cause Encoder::getAngle errors within less than a minute!
Since this a general problem, maybe we should discuss this better within the forum and not within github comments for pull requests, so will open a new post about it at the simpleFOCcommunity: https://community.simplefoc.com/t/errors-with-motor-angle-insufficient-precision-of-float/312.

Cheers,
Juergen

[askuric]

Hey guys,
I'll merge this pull request now and we will discuss the issue later when we actually implement the full support.
At the moment I'll resolve the issue for the open loop, but this remains unsolved issue.

Jurgen, I'm going to adapt the code a bit, but the idea is the same. So thanks once again.

[carbonadam]

I am guessing this is somthing that I am also running into. After about 5 minutes of running in velocity mode with any of the as5047 sensors. The motor eventually slows down and draws quite a lot of current. The slower I run velocity the longer it spins but eventually it will always stall.

[ATILIUS-REGULUS]

Hello carbonadam!
In my case the motor stop and didn't move anymore, so this could be a different problem.
Maybe you can check, at which value of shaft_angle the problem occurs and if the problem occurs always at the same value if you restart the board, in order to check if this is an error due to insufficient precision of the float value.

[runger1101001]

I am guessing this is somthing that I am also running into. After about 5 minutes of running in velocity mode with any of the as5047 sensors. The motor eventually slows down and draws quite a lot of current. The slower I run velocity the longer it spins but eventually it will always stall.

Hey Adam, my guess is that you're running into the precision bug we have with magnetic sensors. Depending on how fast it's spinning, motors stop after a few minutes.
See https://github.com/simplefoc/Ardunio-FOC-drivers/tree/master/src/utilities
and https://github.com/simplefoc/Ardunio-FOC-drivers/blob/master/src/encoders/as5048a/PreciseMagneticSensorAS5048A.h

That code fixes the problem for my AS5048A sensors. I will adapt it to the AS5047, because the diagnostic and magnitude registers are different, but in the meantime it should actually work as is for reading the angles.