Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to first order altitude hold gradient calculation #11063

Closed
wants to merge 10 commits into from
Closed

Fix to first order altitude hold gradient calculation #11063

wants to merge 10 commits into from

Conversation

almaaro
Copy link
Contributor

@almaaro almaaro commented Dec 17, 2018

Test data / coverage
After pr: The altitude setpoint (black) is performing the first order hold seemingly well. (There are some weird spikes here and there, don't know why. Any ideas? However, they don't seem to affect the tecs altitude setpoint.) Tested with at least 20 flight.
image

Before pr: the altitude setpoint will change transiently if the current waypoint is within the acceptance radius of the next one.
image

Describe problem solved by the proposed pull request
nimeton
Before pr: Upon arriving to point A (which is within the acceptance radius of waypoint 1) we calculate the altitude gradient between waypoints 1 and 2. However, if waypoint 1 is within the acceptance radius of waypoint 2, the altitude setpoint will change transiently.
After pr: The altitude control will follow the red line between the actual starting point of the descent (A) and C, which is at the start of waypoint 2 acceptance radius.

Describe your preferred solution
Instead of using the lat, lon and altitude of the previous waypoint (in this case 1), use the lat, lon and altitude of the vehicle at the beginning of the altitude change.

@almaaro almaaro changed the title Fix foh altitude distance Fix to first order altitude hold gradient calculation Dec 17, 2018
@dagar dagar self-requested a review December 18, 2018 17:58
@dagar
Copy link
Member

dagar commented Dec 18, 2018

FYI - I've been wanting to get this logic out of navigator and into the FW position controller. MC is already ignoring it. #8883

@dagar
Copy link
Member

dagar commented Jan 8, 2019

This looks good, I just want to be sure _foh_calculation_start_altitude is always initialized properly.

Any thoughts on simply moving the entire FOH logic to the FW position controller? It removes most of the special checks and makes it usable for non-mission navigation states like RTL.

/* Save the distance between the current sp and the previous one */
if (pos_sp_triplet->current.valid && pos_sp_triplet->previous.valid) {
/* Save the distance between the current sp and our current position */
if (pos_sp_triplet->current.valid) {

_distance_current_previous = get_distance_to_next_waypoint(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop "previous" from _distance_current_previous to reflect the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think _altitude_foh_distance would be good? I think it would describe its use better.

@almaaro
Copy link
Contributor Author

almaaro commented Jan 8, 2019

Any thoughts on simply moving the entire FOH logic to the FW position controller? It removes most of the special checks and makes it usable for non-mission navigation states like RTL.

The altitudes of the current and previous waypoint and distance could be recorded in position_setpoint_triplet_poll() whenever the position setpoint triplet is updated? After that, the _pos_sp_triplet.current's altitude could be adjusted in control_position(...) (the original altitude would ofc be saved)

@Antiheavy
Copy link
Contributor

Seems like an okay idea. Could help in some situations.

@dagar
Copy link
Member

dagar commented Oct 27, 2019

Please reopen. See #11099 (comment)

@dagar dagar closed this Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants