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

Delayed safehome #6655

Merged
merged 13 commits into from
Apr 18, 2021
Merged

Delayed safehome #6655

merged 13 commits into from
Apr 18, 2021

Conversation

tonyyng
Copy link
Contributor

@tonyyng tonyyng commented Mar 1, 2021

The following changes are in Safehomes v2:

  • The safehome does not replace the arming location until it is needed. Also, if you cancel RTH or recover from RX failsafe, the original arming location is restored as the home location.
  • New safehome_usage_mode option:
    • OFF - Safehomes are not used
    • RTH (default) - Safehomes will be used for both manual RTH and RTH RX failsafe
    • RTH_FS - Safehomes will be used only for RX failsafe
  • The Safehome usage mode can be set using the OSD menu
  • New OSD message when RTH to safehome is in progress: "DIVERTING TO SAFEHOME"

By not replacing the arming location with the safehome until it is needed, it fixes the problems where the OSD elements involving home (distance, direction, etc) are those of the safehome instead of the arming location.

Also there are some potential safety checks that use "distance_to_home" that won't work as expected when the arming point has been replaced by the safehome.

Resolves these issues:
#6541 Safehome only use in FS state
#6617 Display telemetry from arming location, not safehome

docs/Settings.md Outdated Show resolved Hide resolved
@avsaase
Copy link
Member

avsaase commented Mar 26, 2021

I tried this today with default setting and noticed that once the plane reached the safe home position the system messages displayed "diverting to safehome" and "loitering around home". I think it would be better to create one message "loitering around safehome" (if that fits on the osd, haven't checked).

@tonyyng
Copy link
Contributor Author

tonyyng commented Mar 27, 2021

I tried this today with default setting and noticed that once the plane reached the safe home position the system messages displayed "diverting to safehome" and "loitering around home". I think it would be better to create one message "loitering around safehome" (if that fits on the osd, haven't checked).

That is a good suggestion. I've made the changes. Can you give it another try? It's still not flying season here.

@digitalentity
Copy link
Member

Can you please rebase so we can merge this?

@digitalentity digitalentity added this to the 3.0 milestone Apr 18, 2021
@tonyyng
Copy link
Contributor Author

tonyyng commented Apr 18, 2021

Can you please rebase so we can merge this?

I don't know how to do this. Is it documented somewhere?

@avsaase
Copy link
Member

avsaase commented Apr 18, 2021

Can you please rebase so we can merge this?

I don't know how to do this. Is it documented somewhere?

The easiest is probably to merge the current master branch into your branch and resolve all the conflicts.

@tonyyng
Copy link
Contributor Author

tonyyng commented Apr 18, 2021

Can you please rebase so we can merge this?

I don't know how to do this. Is it documented somewhere?

The easiest is probably to merge the current master branch into your branch and resolve all the conflicts.

So:

* delayed_safehome
  master
  release_2.6.0
  release_2.6.1
  safe_home_test_2.6.1
  safehome_off_warning
tony@ubinav:~/inav$ git checkout delayed_safehome
Switched to branch 'delayed_safehome'
Your branch is up to date with 'origin/delayed_safehome'.
tony@ubinav:~/inav$ git merge master  <- is this right?

@avsaase
Copy link
Member

avsaase commented Apr 18, 2021

Yes, that should be correct.

@avsaase
Copy link
Member

avsaase commented Apr 18, 2021

@tonyyng I have resolved the merge conflicts. Is it okay if I push them to your branch?

@tonyyng
Copy link
Contributor Author

tonyyng commented Apr 18, 2021

@tonyyng I have resolved the merge conflicts. Is it okay if I push them to your branch?

Sure

# Conflicts:
#	docs/Settings.md
#	src/main/fc/settings.yaml
#	src/main/navigation/navigation.c
@CertainBot
Copy link

Great job @TonyYing, very useful PR! Not a big deal but I found the options a bit misleading but you might as well have some other considerations in mind of course. To me 'OFF' is ok, 'RTH' I undertsand only as the manual RTH and RTH_FS I see as both RTH + FS. Wouldn't be better RTH_FS (default, all) and FS_ONLY just for FS?

@tonyyng
Copy link
Contributor Author

tonyyng commented May 14, 2021

Great job @TonyYing, very useful PR! Not a big deal but I found the options a bit misleading but you might as well have some other considerations in mind of course. To me 'OFF' is ok, 'RTH' I undertsand only as the manual RTH and RTH_FS I see as both RTH + FS. Wouldn't be better RTH_FS (default, all) and FS_ONLY just for FS?

Maybe, but as long as the behavior is documented accurately, I don't think it makes a difference, especially now that the changes have been merged.

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.

4 participants