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 mission check for init mission #22846

Merged
merged 10 commits into from
Mar 8, 2024
Merged

Conversation

KonradRudin
Copy link
Contributor

Solved Problem

The mission feasibility checks could return a wrong result on startup for the loaded mission on the storage.

Solution

  • Only run the mission feasibility check after the geofence has loaded all the data.
  • Only run mission feasibility check after a global position topic has been published.
  • Some Bugfixes

Changelog Entry

For release notes:

Bugfix Correclty check mission feasilbity on startup

Alternatives

We really should decouple the feasibility check from the mission execution.

Test coverage

  • SITL test:
    • Test case 1: Define a valid mission and store it on the vehicle. Stop the simulation. Start it again, with a different location far away.
    • Test case 2: Define a mission and a geofence, where the mission violates the geofence. Stop the simulation. Start it again.
    • Test results: Prior in both cases, the vehicle would arm then immediately shut down again since the reevaluation of the mission feasibility returned the expected result. However, with the PR, the vehicle is not even armed, as it properly determines the mission feasibility at startup.

BE AWARE OF

  • What should be happening if we don't have a global position? (i guess we anyways can't fly the mission, so it should be fine).

@KonradRudin KonradRudin force-pushed the fix_mission_check_for_init_mission branch from f4d4709 to 4636069 Compare March 6, 2024 13:30
@sfuhrer sfuhrer added the bug label Mar 6, 2024
@sfuhrer
Copy link
Contributor

sfuhrer commented Mar 6, 2024

Okay I've checked the state about requirement when to run the feasibility checker and when not (what is now in canRunMissionFeasibility()).
With current main the mission feasibility checks return false if ho valid Home is set. And it is not possible to set a Home position manually because of it's rejected if local position is invalid..
With the changes as proposed here the behavior is different in that it will in the "no GPS" case always upload the mission instead of rejecting it, but then it will check the feasibility of it once it gets a GPS lock. Seems better to me, and it's still safe as you won't be able to execute an invalid mission in both ways.

It would be nice to support planning missions offline or with a vehicle without GPS lock and still run the checks that are possible (e.g. by assuming that the vehicle position is at the planned Home), but this is for me a new feature request.

sfuhrer
sfuhrer previously approved these changes Mar 6, 2024
Copy link
Contributor

@sfuhrer sfuhrer left a comment

Choose a reason for hiding this comment

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

All good findings, and for me the right solutions. As you say probably not the last bugs in the logic but certainly a stab in the right direction.

@jongell
Copy link
Contributor

jongell commented Mar 8, 2024

Tested with VTOL:

Works with:
-Existing Takeoff in the mission, but missing landing pattern
-1st Waypoint too far

Found one corner case:

  1. MIS_DIST_1WP = 10000
  2. Upload normal VTOL Mission with Take off and Land Pattern within 10000m
  3. Change MIS_DIST_1WP to 1m
  4. Start Mission without reuploading it via Action in AMC

Here the Vehicle first arms, switches to Mission and then triggers the no valid mission failsafe, switches to return, waits and disarms

@KonradRudin
Copy link
Contributor Author

Tested with VTOL:

Works with: -Existing Takeoff in the mission, but missing landing pattern -1st Waypoint too far

Found one corner case:

  1. MIS_DIST_1WP = 10000
  2. Upload normal VTOL Mission with Take off and Land Pattern within 10000m
  3. Change MIS_DIST_1WP to 1m
  4. Start Mission without reuploading it via Action in AMC

Here the Vehicle first arms, switches to Mission and then triggers the no valid mission failsafe, switches to return, waits and disarms

Thanks for testing. Yeah, the check is not rerun on parameters change that is why the commander thinks the mission is feasible and switches to the mission. On mission activation however, the check is performed again, and thus, at this point it know that it is infeasible and shuts down again. I think we should address this later when we properly fix/refactor the check. I will only make a small change to make sure that the mission is rechecked on activation (currently depending on timing, you could change the parameter such that the mission is infeasible and still fly the mission). @sfuhrer for visibility

@sfuhrer sfuhrer merged commit 04099ed into main Mar 8, 2024
92 checks passed
@sfuhrer sfuhrer deleted the fix_mission_check_for_init_mission branch March 8, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants