-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
FW launch status reporting improvement #7068
FW launch status reporting improvement #7068
Conversation
Why is all those states needed, since only 2 will be used (launch detected and finish/abort? https://github.com/iNavFlight/inav/pull/7068/files#diff-11d5bd31d29e1a5d6ca52ed58ff0694afdf0559801dafb5fb63170245eb0f90f
I see more complexity by adding these, also the code is just doing the same. Perhaps you may want to add the launchStatus in the fixedWingLaunchData_t to be more consistent
I'm just saying |
The problem with the states used at the moment is they're transient. Launch Detected state lasts a loop/few loops and then is no longer the current state. You know you've moved beyond it perhaps by looking at the current state number but when you get to the end of the launch you end up at the idle state which resets the state number sequence back to 0 meaning there's no way to know how far the launch sequence went before finishing. Was it aborted immediately after detection or did it actually reach the end with a flying plane ? I wanted to use the change in this PR as a means of detecting if the plane was actually likely to be successfully flying at the end of the launch or if the launch was aborted before then meaning you couldn't be sure the plane was flying. Maybe there's a better way of doing this but as it is it's difficult to understand what happened at launch finish using the current state reporting. |
I see. But the navigation doesn't know what to do with such of states at this moment (Was it aborted immediately after detection or did it actually reach the end with a flying plane), it's just the same behaviour as isFixedWingLaunchFinishedOrAborted(), no distinction, so there will be no result or benefit for the users. On the other hand you add transient state in navigation.c if I'm not wrong. I don't know... |
Well it did occur to me that the idle state is the problem here, resetting the state sequence to 0. Should be possible to end launch and hold the current state so doesn't really need an idle state ? Once launch is finished it's not going to be used again during the flight so setting it to an idle state doesn't really achieve anything other than flag the launch is finished or aborted ... which could be achieved by another method that maintains the last current state. As to the suggestion to add the additional states ... seems that is a more complicated solution than adding a status flag (and yes I know people don't like flags but maybe sometimes they offer the more effective solution). |
Yes. By adding a state (let's name it 'FW_LAUNCH_STATE_FLYING') which is called on success event for `FW_LAUNCH_STATE_FINISH'. This can remain until the navigation will activate again the autolaunch . IDLE should remain as an initial state, we can rename it to match a state of 'waiting for activation'. I know it looks more complicated, but in the end, you have to deal with one step at a time, each step doing its thing.
... of course requires the implementation for handlers of each new state. |
I think adding the additional states should work because it preserves the state sequence to the end. It also maintains the cleaner structure which, I agree, is a much better than the old version. Not sure the Idle state would be used in this case though, given that Launch initialises to the |
Replaced status flag with additional states instead
Decided to remove the Idle state in the end since it seems totally redundant with the 2 new end states added. Also changed the Abort logic during the |
great. looking good👍 |
This is working as required so could be included in 4.0.0. Doesn't change anything from a user point of view but does improve the logic of the launch sequence making the launch status more obvious and easier to use where required. |
…h_status_reporting
Provides a more useful means of tracking the status of the fixed wing launch sequence with 2 additional end states added making it clearer if the launch resulted in flight or was aborted. The Idle state has been removed since it is now redundant, replaced by the 2 new additional states.