Coding error, can anyone confirm?
I tried to understand Arduplane's code. In calc_nav_roll (Attitude.pde) there's a command:
nav_roll = g.pidNavRoll.get_pid(bearing_error, dTnav, nav_gain_scaler);I understand that dTnav is the time interval between calls of PID controller. This command is executed each time calc_nav_roll is called so the time interval between calc_nav_roll's calls also dTnav. dTnav is updated in medium_loop at the frequency of about 10 Hz, so dTnav is about 100 ms. Looking at the main loop, the fast_loop is called at the frequency of about 50 Hz, the time interval between calls is delta_ms_fast_loop which is about 20 ms. The fast_loop calls update_current_flight_mode() that means the time interval between calls also 20 ms. In most cases of update_current_flight_mode() it calls cal_nav_roll, so the time interval between calls is only 20 ms. There is a big difference between 20 ms and 100 ms. In my understanding in get_pid() calls instead of dTnav it should be delta_ms_fast_loop. Is this a coding error? Can anyone confirm or clarify this?
yep, you're right. Jon Challinger noticed this recently as well. I've been thinking about how to fix it.
It's easy to just fix the deltat of course, but that will change the meaning of the I and D terms in everyones existing configurations (ie. the HDG2RLL_I and HDG2RLL_D values), scaling them by a factor of 5x. That could change how well a plane flies by quite a lot. I'm always reluctant to break existing tuning, but in this case we may have to, with a warning in the release notes.
I'm also thinking about how to ensure this sort of bug doesn't happen again. I think the best way would be to make the PID object compute the deltat itself, rather than passing it in.
Changing this even with a large disclaimer will IMHO cause quite a few crashes since not everyone is reading the release notes. Even if, it's easy to forget that when loading old parameter files (I'm not using MP so I have no clue how this is handled). If the effect is really just a scaling issue then I'd rather change the variable name, apply the scaling and put a big fat disclaimer in the code (e.g., somewhere in the parameter section when these PIF parameters are scaled appropriately). Just my 5 cents..
The path of least user pain ( ie none ) would be to do both the following:
(a) if the I&D terms are at their default, reset them to whatever the new defaults need to be ( eg 5x) at the same time as you make the code treat them differently. ( will require a coordinated update of planner and firmware , like the mavlink 1.0 update/s.... I suspect )
(b) if the I&D terms are *not* at their default, popup a warning to the user, tell them about the 5x scaling issue, and ask them if they want to ( 1) have a once-off scaling factor applied for them or (2) reset the values to their default/s or (3) leave the values alone - not recommended.
I think this time you have to break existing tuning because dTnav is not exactly 5x of delta_ms_fast_loop, only is about so the corrected code will operate more precisely. BTW it's not easy to build a general PID object that can compute the deltat itself because outside there's also no mechanism to know, only the programmer knows.
Another thing I don't understand is that when looking at calc_nav_pitch(). There is a command
nav_pitch = -g.pidNavPitchAirspeed.get_pid(airspeed_error, dTnav); (2 parameters call)
However, get_pid definition is
PID::get_pid(int32_t error, uint16_t dt, float scaler) (3 parameters definition)
How can it be like this? Can anyone explain me?