Remove MPC_USE_HTE and always use hover thrust for altitude control
#24751
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Note the separate module can still be not run but I have not seen a case where the hover thrust estimate is not useful for altitude control hence I don't expect anyone to have the parameter disabled or planning on not running the module. This parameter was used to experimentally introduce the hover thrust estimator. First it was just logging its status and you could opt in to use it: f9794e9 but soon after it became the default and you had to opt out of using it: a8063ac AFAIK we haven't seen problems requiring to disable it in the last 5 years and hence I suggest to remove the parameter to reduce the configuration space.
2 tasks
🔎 FLASH Analysispx4_fmu-v5x [Total VM Diff: -56 byte (-0 %)]px4_fmu-v6x [Total VM Diff: -136 byte (-0.01 %)]Updated: 2025-04-22T17:08:31 |
|
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: |
|
I flight tested this yesterday and all seemed well. |
|
No flaws found |
|
@dakejahl Thanks for testing! I had a quick glance and it looks good from my perspective with this pull request in mind: |
dakejahl
approved these changes
Apr 25, 2025
bresch
requested changes
May 2, 2025
| @@ -98,12 +97,12 @@ void MulticopterLandDetector::_update_topics() | |||
| _flag_control_climb_rate_enabled = vehicle_control_mode.flag_control_climb_rate_enabled; | |||
| } | |||
|
|
|||
| if (_params.useHoverThrustEstimate) { | |||
| if (_hover_thrust_estimate_sub.updated()) { | |||
| hover_thrust_estimate_s hte; | |||
|
|
|||
| if (_hover_thrust_estimate_sub.update(&hte)) { | |||
There was a problem hiding this comment.
Suggested change
| if (_hover_thrust_estimate_sub.update(&hte)) { | |
| if (_hover_thrust_estimate_sub.copy(&hte)) { |
Since you already have the updated above.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solved Problem
When we discussed #24710 in the dev call and agreed once again we need to reduce the configuration space, I realized I would never change this parameter to 0 or advise someone to do so. It was introduced beginning 2020 to experimentally introduce the hover thrust estimator but was soon after enabled by default and I have not seen anyone turn it off.
References:
Hover thrust estimator introduction: #13981
Addition of the parameter (and many improvements): #14182
Enabling use of the estimate the default: #14419
Solution
Note the separate
mc_hover_thrust_estimatormodule can still be not run but I have not seen a case where the hover thrust estimate is not useful for altitude control hence I don't expect anyone to have the parameter disabled or planning on not running the module.Please review the cases where the hover thrust is not available or gets invalid. My intention is to keep on using the last valid estimate like for the stick rescaling in Stabilized mode. This is in my eyes better than jumping back to the hover thrust parameter. I left the safety logic to make landing detection checks more strict with invalid hover thrust.
Changelog Entry
Test coverage
I have not tested this pr. But can do so if we decide to go forward with it.