-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Offboard lost actions with additional timeout #4655
Conversation
I like your architectural approach better than the extensions I made. What is the testing state of this - good to go from your side? |
When do you expect to test this? |
Today |
b06dd84
to
de03923
Compare
Fixed the obvious errors.. but testing is difficult, my parameters get reset somehow? (#4722) |
87816c3
to
f08db22
Compare
Rebased, tested, ready. |
* | ||
* @group Mission | ||
*/ | ||
PARAM_DEFINE_INT32(NAV_OBL_ACT, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this param defined in the navigator space if the implementation is in commander?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, well, ahm, because the others are :). You're right of course, I should already move them to the commander. Then we can also move the rest of the failsafe handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to have a param NAV_
if the action is decided in the navigator. In this case clearly all the logic is in commander, so I don't see why it should be COM_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I mean moving the handling and params over to the commander, in a later PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a pain to move the params later? I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Na I meant other ones, doesn't matter, will do later. Moved mine and rebased.
f08db22
to
d1b27ab
Compare
Fixed and merged |
Reimplemented offboard lost handling from #3404
@LorenzMeier before I go ahead with this, I'd like some feedback on where the states should switch. Since my earlier version you started to add failsafe handling in the navigator (e.g. here: https://github.com/PX4/Firmware/blob/master/src/modules/navigator/navigator_main.cpp#L570), and now we have 2 places where nav state decisions happen, a duplication of responsibilities. So in contrary of how RC and DL loss got implemented on master, the state switching here is all in state_machine_helper.