A bug in THROTTLE_ANGLE_CORRECTION

[Eric]

Code: Select all

 
  #if defined(THROTTLE_ANGLE_CORRECTION)
    cosZ = EstG.V16.Z / (ACC_1G * 100.0f);                                                        // cos(angleZ) * 100 
    throttleAngleCorrection = THROTTLE_ANGLE_CORRECTION * constrain(100 - cosZ, 0, 100) >>3;  // 16 bit ok: 200*150 = 30000  
  #endif

Should be

Code: Select all

 
  #if defined(THROTTLE_ANGLE_CORRECTION)
    cosZ = EstG.V16.Z * 100 / ACC_1G;                                                        // cos(angleZ) * 100, mul 100 should be enough
    throttleAngleCorrection = THROTTLE_ANGLE_CORRECTION * constrain(100 - cosZ, 0, 100) >>3;  // 16 bit ok: 200*150 = 30000  
  #endif

Or cosZ will always be 0.

ACC_1G * 100.0f is a large number, or you have to cast EstG.V16.Z to float to get the correct result.

[Alexinparis]

Right, there is something wrong here: /100 instead of *100
this error was introduced unintentionally by myself

but EstG.V16.Z * 100 / ACC_1G is still not correct:
because EstG.V16.Z * 100 can overflow as the result of 16bitx16bit will result in a 16bit temp var (at least on 8bit proc)

EstG.V16.Z * 100.0f / ACC_1G should be correct
or better for speed:
(int32_t)EstG.V16.Z * 100 / ACC_1G
or even better:
mul(EstG.V16.Z , 100) / ACC_1G

[mgros]

at last!!!

[sismeiro]

Hi,

This is a very nice feature to have but I didn't try it myself yet. I hope this is the last bug found in this implementation. I will update my MultiWii branch with this correction.

Any videos of this feature? I would like to see that how it behaves before trying it myself because 2.3 is so good maintaining the same flying height as it is.

Regards,
Luis Sismeiro

[Crashpilot1000]

Well, when tilting a copter the thrust of the props is not completely used for lifting any more. So the throttle - uncompensated - vertical speed will result in a hight loss. If your copter doesn't suffer from that simple law of physics and has a stellar 100+Hz Baro/ACC code at work you have no problem, so what could videos do to improve your already perfect result? Maybe a painting?

[sismeiro]

If your copter doesn't suffer from that simple law of physics and has a stellar 100+Hz Baro/ACC code at work you have no problem, so what could videos do to improve your already perfect result? Maybe a painting?

I am sorry if you didn't like my question. I am just curious to know if there are any known videos with this feature, just that. There are many other videos about MultiWii features that are helpful. It's becoming more and more difficult to have educated exchange of opinions in forums, everybody is becoming extremist about something that it's said or written.

[Crashpilot1000]

Whatever. BTW having throttleAngleCorrection in GPS PH is not a good idea - perhaps it's no issue with a 5Hz-10Hz GPS loop - just saying.

[EOSBandi]

Whatever. BTW having throttleAngleCorrection in GPS PH is not a good idea - perhaps it's no issue with a 5Hz-10Hz GPS loop - just saying.

THA helps on altitude hold, and I think it won't interfere with GPS PH on any way....

[ZaksterBlue]

Hi gurus, not a programming expert so wasn't sure how to make Alex's suggested change of "cosZ = mul(EstG.V16.Z , 100) / ACC_1G" work. Once I replaced the line in IMU.CPP and enabled AngleCorrection in the config.h, 2.3 wouldn't compile with the following messages:

IMU.cpp: In function 'void getEstimatedAttitude()':
IMU.cpp:243: error: 'union t_fp_vector' has no member named 'V16'
IMU.cpp:243: error: 'mul' was not declared in this scope

[timecop]

Right, there is something wrong here: /100 instead of *100
this error was introduced unintentionally by myself

but EstG.V16.Z * 100 / ACC_1G is still not correct:
because EstG.V16.Z * 100 can overflow as the result of 16bitx16bit will result in a 16bit temp var (at least on 8bit proc)

EstG.V16.Z * 100.0f / ACC_1G should be correct
or better for speed:
(int32_t)EstG.V16.Z * 100 / ACC_1G
or even better:
mul(EstG.V16.Z , 100) / ACC_1G

What's the non-smallangle approximation version of this stuff?
cosf( of what ) ?

[Alexinparis]

Hi gurus, not a programming expert so wasn't sure how to make Alex's suggested change of "cosZ = mul(EstG.V16.Z , 100) / ACC_1G" work. Once I replaced the line in IMU.CPP and enabled AngleCorrection in the config.h, 2.3 wouldn't compile with the following messages:

IMU.cpp: In function 'void getEstimatedAttitude()':
IMU.cpp:243: error: 'union t_fp_vector' has no member named 'V16'
IMU.cpp:243: error: 'mul' was not declared in this scope

mul(,) function and new vector struct are not in release 2.3
the code here is based on the current dev code.
I will upload an intermediate dev release to ease the test.

[-ralf-]

I will upload an intermediate dev release to ease the test.

Hi Alex,

happy new year

Can you integrate the S.Bus-Patch by "Plüschi" in the intermediate dev?
http://www.multiwii.com/forum/viewtopic.php?f=8&t=4425

Thanks .....

[Alexinparis]

Right, there is something wrong here: /100 instead of *100
this error was introduced unintentionally by myself

but EstG.V16.Z * 100 / ACC_1G is still not correct:
because EstG.V16.Z * 100 can overflow as the result of 16bitx16bit will result in a 16bit temp var (at least on 8bit proc)

EstG.V16.Z * 100.0f / ACC_1G should be correct
or better for speed:
(int32_t)EstG.V16.Z * 100 / ACC_1G
or even better:
mul(EstG.V16.Z , 100) / ACC_1G

What's the non-smallangle approximation version of this stuff?
cosf( of what ) ?

I don't think it's currently an approximation.
it's rather a definition: cos(alpha) = Z part / magnitude
and magnitude is supposed to be ACC_1G without additional acceleration
if you want to be more accurate: magnitude = sqrt(X^2+Y^2+Z^2)

[-ralf-]

Hi Alex,

we wrote at the same time .....
http://www.multiwii.com/forum/viewtopic.php?p=45197#p45197