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.

Sign up for GitHub

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

Jump to bottom

Block auto-disarm when in Launch Detection for Catapault/Hand-Launched FW #12513

Closed
wants to merge 6 commits into from
Closed

Block auto-disarm when in Launch Detection for Catapault/Hand-Launched FW #12513

wants to merge 6 commits into from

Conversation

Kjkinney
Copy link
Contributor

@Kjkinney Kjkinney commented Jul 18, 2019 •
edited
Loading

Describe problem solved by the proposed pull request
Hand launched Fixed-Wings are unable to use auto-disarm due to it being based of the Land-Detector. Because the Launch-Detection is less sensitive to "Non-Landed" States than the Land-Detector, this will often lead to a situation where the motor is not spinning and the vehicle transitions from landed to non-landed and then back to landed, triggering the auto disarm. This occurs during ground handling after the vehicle is armed. Currently we leave auto-disarm disabled.

Test data / coverage
This has been tested in a lab on a pixracer flight controller mounted on a fixed wing vehicle while spoofing GPS. https://review.px4.io/plot_app?log=e6249163-72e1-4ae6-9aa0-b56c947659de

Describe your preferred solution
This issue is solved by having the Fixedwing Position Controller set a message on the position_controller_status uORB struct determining whether or not Launch-Detection is running. This message is then subscribed to by the commander, which then blocks all auto-disarming if it is running(true).

Describe possible alternatives
There was talk about using a variable already declared for this, but that is based off of Land-Detector which this is circumventing.

Additional context
Discussed in this weeks dev-call.

FYI @Antiheavy

@Antiheavy Antiheavy requested a review from MaEtUgR July 18, 2019 18:41
@Antiheavy
Copy link
Contributor

FYI/A @MaEtUgR , we discussed this during the Dev call. This PR helps make life easier for hand-launched fixed wings, now that auto-disarm is enabled by default: #12346

@Antiheavy Antiheavy mentioned this pull request Jul 19, 2019
Merged
@weekly-digest weekly-digest bot mentioned this pull request Jul 21, 2019
Closed
MaEtUgR
MaEtUgR reviewed Jul 22, 2019
View reviewed changes
Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

Could the logic be in a way that minimizes impact in commander?
I imagine the launch detector implementing _get_launchdetection_status(), the fixed wing position controller just feeding it into the status and the commander only checking the status (and not looking at the parameter and additional conditions itself). That should work if launch detection is turned on when arming and auto disarm should work again after a successful launch.

src/modules/commander/Commander.cpp Outdated
@@ -1317,6 +1322,10 @@ Commander::run()
int32_t airmode = 0;
int32_t rc_map_arm_switch = 0;

/* Catapault Launch Enabled */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* Catapault Launch Enabled */
/* Catapult Launch Enabled */

src/modules/commander/Commander.cpp Outdated
bool Commander::launch_detection_running()
{
bool updated;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/modules/commander/Commander.cpp Outdated
bool updated;

position_controller_status_s _controller_status{};

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/modules/commander/Commander.cpp Outdated

bool Commander::launch_detection_running()
{
bool updated;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool updated;
bool updated = false;

src/modules/commander/Commander.cpp Outdated
orb_check(_position_controller_sub, &updated);

if (updated) {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/modules/commander/Commander.cpp Outdated

}


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/modules/commander/Commander.hpp Outdated
@@ -196,6 +197,9 @@ class Commander : public ModuleBase<Commander>, public ModuleParams

void update_control_mode();


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/modules/commander/Commander.cpp Outdated
@@ -1251,6 +1254,8 @@ Commander::run()
uORB::Subscription system_power_sub{ORB_ID(system_power)};
uORB::Subscription vtol_vehicle_status_sub{ORB_ID(vtol_vehicle_status)};


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/modules/commander/Commander.cpp Outdated
@@ -1251,6 +1254,8 @@ Commander::run()
uORB::Subscription system_power_sub{ORB_ID(system_power)};
uORB::Subscription vtol_vehicle_status_sub{ORB_ID(vtol_vehicle_status)};



Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/modules/fw_pos_control_l1/FixedwingPositionControl.cpp Outdated

bool launch_detection_running;

if(pos_sp_curr.type == position_setpoint_s::SETPOINT_TYPE_TAKEOFF &&
Copy link
Member

Choose a reason for hiding this comment

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

style incorrect in this method

@weekly-digest weekly-digest bot mentioned this pull request Jul 28, 2019
Closed
Kjkinney added 3 commits July 30, 2019 10:22
@Kjkinney
This adds a launch detection uORB message used by commander to contro…
a840d9c 
…l auto-disarm

The uORB message is in the "position_control_status" struct whixh is published by the Fixed Wing Position Controller
@Kjkinney
Fix if statement
8c53b99
@Kjkinney
Adds suggested changes to the Auto-Disarm block.
868f4b8 
The launchdetector already had a way to determine if it was running or not implemented. This was then added to the Fixed wing position controller. All the parameter logic was also removed from commander. Commander now only checks the Poisition Controller status uORB message as well.
@Kjkinney Kjkinney force-pushed the pr-Autodisarm-CatapaultLaunch branch from bada010 to 868f4b8 Compare July 30, 2019 18:26
@Kjkinney
Copy link
Contributor Author

Re-based and implemented the suggested improvements. @MaEtUgR can you review it again?

@davids5 davids5 requested a review from RomanBapst August 7, 2019 15:08
@davids5 davids5 requested review from MaEtUgR and removed request for MaEtUgR August 7, 2019 15:09
@Antiheavy
Copy link
Contributor

We have tested this branch on the ground. We also have been extensively flight testing a slightly different version of this (built on v1.8.0) for the past month. It works quite well.

@weekly-digest weekly-digest bot mentioned this pull request Aug 11, 2019
Closed
RomanBapst
RomanBapst previously approved these changes Aug 21, 2019
View reviewed changes
Copy link
Contributor

@RomanBapst RomanBapst left a comment

Choose a reason for hiding this comment

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

I think the changes are ok. I think you will need to rebase this once more since there have been slight changes to the auto-disarm logic.
I'm ok to have this merged after and given that you've done extensive testing.

@RomanBapst
Copy link
Contributor

@Kjkinney Could you please resolve the conflict so that we can merge it?

@weekly-digest weekly-digest bot mentioned this pull request Aug 25, 2019
Closed
@Kjkinney
Copy link
Contributor Author

@RomanBapst will do

@weekly-digest weekly-digest bot mentioned this pull request Sep 1, 2019
Closed
@weekly-digest weekly-digest bot mentioned this pull request Sep 15, 2019
Closed
@dagar dagar self-requested a review November 25, 2019 22:48
@Kjkinney
Copy link
Contributor Author

Moving to new PR

@Kjkinney Kjkinney closed this Dec 23, 2019
@Kjkinney Kjkinney mentioned this pull request Dec 23, 2019
Open
@Kjkinney Kjkinney deleted the pr-Autodisarm-CatapaultLaunch branch December 26, 2019 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Antiheavy Antiheavy Antiheavy left review comments

@MaEtUgR MaEtUgR MaEtUgR left review comments

@RomanBapst RomanBapst RomanBapst left review comments

@dagar dagar Awaiting requested review from dagar

Assignees
No one assigned
Labels
Admin: Enhancement (improvement) 💡 Fixed Wing 🛩️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Kjkinney @Antiheavy @RomanBapst @MaEtUgR @julianoes