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

Vehicle: add support for altitude above terrain as fact for telemetry panel #10786

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

Davidsastresas
Copy link
Member

@Davidsastresas Davidsastresas commented Aug 24, 2023

Add support to have altitude above terrain as displayable telemetry. It queries terrain data when vehicle coordinate is changed to compute the altitude, but it has an elapsed timer to limit the time between those queries to 500ms minimum, because the terrain query system wasn't able to service them at higher rate, and 500ms is fine anyway for this information. This elapsed timer is not super elegant there, if anybody has a nicer idea for it I will be happy to work on it.

Last commit is unrelated, but I realized that function wasn't in the private slot declaration, and it belongs there. Same for first commit.

Thanks to https://araceuas.com for sponsoring this.

@Davidsastresas Davidsastresas force-pushed the master-PR/Alt_above_terrain branch from 509a421 to be95ad0 Compare August 24, 2023 22:27
@s-lisovenko
Copy link
Contributor

Very useful feature. Thanks!

@s-lisovenko
Copy link
Contributor

About the 500 ms timer, do you think it might be better to use a boolean variable that can be set to false when sending a request and to true in the handler slot. In this case, we can not send new requests until the current one is processed?

@s-lisovenko
Copy link
Contributor

Also regarding naming new facts, maybe altitudeAGL would be preferable? It is more consistent with avionics.

@Davidsastresas
Copy link
Member Author

Davidsastresas commented Sep 26, 2023

Thanks for taking a look at it!

About the boolean, it would not be as simple because if for some reason the request is not serviced we would have a deadlock. A "similar"idea is already implemented in the terrainquery implementation itself, so It would have been totally feasible and safe to not implement timer at all, terrainquery mechanism takes care of it. I just tested a bit and I realized the terrain query system wasn't able to provide updates at a higher rate than that, so we just save the minimal extra cpu cycles it would take.

About the fact name, I totally agree, will fix it this ASAP.

Thanks!

@DonLakeFlyer
Copy link
Contributor

Two things:

  • How do I know if the altitude above terrain fact has a valid value in it? Seems like this should be initialized to nan or something so you know whether an initial terrain query hasn't returned yet.
  • I think it might make sense to add an additional check on distance from previous coordinate query to limit even further the calls to the terrain server. I get paranoid about chewing laptop battery to do work which will just return the same value over and over again.

@Davidsastresas Davidsastresas force-pushed the master-PR/Alt_above_terrain branch from be95ad0 to 91523d9 Compare November 16, 2023 22:46
@Davidsastresas
Copy link
Member Author

Davidsastresas commented Nov 16, 2023

@DonLakeFlyer I agree with your suggestions. I rebased to master and addressed your comments.

I needed to check for altitude change as well, because checking only distance would not query new terrain altitude in the situation when a copter gains altitude in the same coordinate.

I left a minimum change of .5 meters in altitude and 2 meters in XY for a new query to be executed.

I left the commits separated for you to check the changes, but If you confirm it to be fine I will squash them before merging.

thanks!

@DonLakeFlyer
Copy link
Contributor

Looks good. Squash it and you can merge it.

@Davidsastresas Davidsastresas force-pushed the master-PR/Alt_above_terrain branch from 91523d9 to aaddeb6 Compare November 20, 2023 11:27
@Davidsastresas
Copy link
Member Author

Thanks, done!

@Davidsastresas Davidsastresas merged commit 22b3408 into master Nov 20, 2023
6 checks passed
@Davidsastresas Davidsastresas deleted the master-PR/Alt_above_terrain branch November 20, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants