Minor pid.c refactoring #12722
Minor pid.c refactoring #12722
Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks, @ledvinap. I agree about the desirability of keeping it readable and simple. Though I'm confused why we would multiply the preTpaD value by '22' for logging purposes? I can't quite understand the explanatory comment. The value being logged is simply the pre-TPA D value. Why multiply that logged value by 22? To me that makes no sense. Maybe multiply by 10 for precision perhaps... but 22? Very tempted to change 22 to 10. |
src/main/flight/pid.c
Outdated
| @@ -245,7 +245,7 @@ void pgResetFn_pidProfiles(pidProfile_t *pidProfiles) | |||
|
|
|||
| // Scale factors to make best use of range with D_LPF debugging, aiming for max +/-16K as debug values are 16 bit | |||
| #define D_LPF_RAW_SCALE 25 | |||
| #define D_LPF_FILT_SCALE 22 | |||
| #define DEBUG_D_PRE_TPA_SCALE 10 | |||
There was a problem hiding this comment.
Makes no sense to log a pre-TPA D value that is multiplied by 22, when the following, normal, D value is not multiplied by anything.
A multiply by 10 makes the logged values much easier to compare, numerically, and does enhance precision of the logged value.
There was a problem hiding this comment.
Does this need coordination with the blackbox viewer?
There was a problem hiding this comment.
bbe:
$ grep -Iirn D_LPF ./js
./js/flightlog_fields_presenter.js:846: 'D_LPF' : {
./js/flightlog_fields_presenter.js:847: 'debug[all]':'D-Term [D_LPF]',
./js/flightlog_fielddefs.js:337: "D_LPF",
upon skim-read, i do not see any SCALE nor static 22 associated.
may need to add translation (flightlog_parser.js:358), but unsure 🤷♂️
There was a problem hiding this comment.
Does this need coordination with the blackbox viewer?
No, it is easier to compare with DTerm when the multiplier is 10.
BlackBox viewer needs quite a lot of updating :-(
There was a problem hiding this comment.
@ctzsnooze You're giving me mixed signals here 😀 Do we need to update the corresponding debug channel in the blackbox viewer or not (to make this perfectly comparable to the D-term)?
There was a problem hiding this comment.
No, this change improves the blackbox scaling so that the D_LPF Debug is easier to interpret.
There are many other debugs which require attention, including the D_LPF debug. The D_LPF debug from memory requires attention in some of its other channels.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
AUTOMERGE: (FAIL)
|
This comment was marked as outdated.
This comment was marked as outdated.
03c5473 to
1f91ab5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@nerdCopter Thanks, I removed any two blank lines in sequence and replaced with one. |
This comment has been minimized.
This comment has been minimized.
Remove unnecessary comment Change multiplier to factor of 10
d9d220e to
6a8966a
Compare
* Minor pid.c refactoring * Only calculate unfiltered gyro delta for that debug Remove unnecessary comment Change multiplier to factor of 10 * remove some archaic comments * clarify name of D_LPF_PRE_TPA_SCALE, use inverse in define * Remove some blank lines * undo conversion to division thanks -ffast-math
* Minor pid.c refactoring * Only calculate unfiltered gyro delta for that debug Remove unnecessary comment Change multiplier to factor of 10 * remove some archaic comments * clarify name of D_LPF_PRE_TPA_SCALE, use inverse in define * Remove some blank lines * undo conversion to division thanks -ffast-math
* Minor pid.c refactoring * Only calculate unfiltered gyro delta for that debug Remove unnecessary comment Change multiplier to factor of 10 * remove some archaic comments * clarify name of D_LPF_PRE_TPA_SCALE, use inverse in define * Remove some blank lines * undo conversion to division thanks -ffast-math
Small changes, perhaps a little more efficient.