splitting preflight and land disarm times into 2 parameters #12608

catch-twenty-two · 2019-08-02T16:13:27Z

Describe problem solved by the proposed pull request
This splits preflight takeoff (vehicle has never taken off after being armed) and landing disarm times into 2 different parameters.

Parameter description is as follows:

PARAM_DEFINE_FLOAT(COM_DISARM_LAND, 5.0f);

Time-out for auto disarm after landing
A non-zero, positive value specifies the time-out period in seconds after which the vehicle will be
automatically disarmed in case a landing situation has been detected during this period.
A negative value means that automatic disarming triggered by landing detection is disabled.

PARAM_DEFINE_FLOAT(COM_DISARM_PRFLT, 2.0f);

Time-out for auto disarm before the first take off
A non-zero, positive value specifies the time-out period in seconds after which the vehicle will be
automatically disarmed when the vehicle has never taken off.
A negative value means that automatic disarming triggered by a pre-takeoff timeout is disabled.

Test data / coverage
Was tested in the simulator

dagar · 2019-08-02T16:34:01Z

Minor code style failure in CI. http://ci.px4.io:8080/blue/organizations/jenkins/PX4%2FFirmware/detail/PR-12608/1/pipeline

catch-twenty-two · 2019-08-02T17:05:42Z

Thx, should be fixed

RomanBapst · 2019-08-03T11:11:48Z

Just to give a bit more background information: In some use cases like filming it's desirable to have the drone idling on the ground ready to takeoff any second. It's very annoying if the drone keeps disarming, hence the addition of this parameter.
It can make the difference between getting the perfect shot and missing it.

RomanBapst · 2019-08-03T11:12:37Z

@hamishwillee Pinging you here just to make sure we don't miss this in the docs.

RomanBapst · 2019-08-03T11:14:14Z

@hamishwillee I assume this will need changes in QGC as well:

hamishwillee · 2019-08-04T22:33:43Z

src/modules/commander/commander_params.c

+PARAM_DEFINE_FLOAT(COM_DISARM_LAND, 5.0f);
+
+/**
+ * Time-out for auto disarm before the first take off


Probably "... before takeoff". Because someone can land and use this again after disarming/rearming.

Thought I somewhat prefer "Time-out for auto disarm if too slow to takeoff"

Why did you change the default value of 2 seconds?

Yep, better 2s for disarm after landing, 10s for disarm before takeoff.

hamishwillee · 2019-08-04T22:37:25Z

src/modules/commander/commander_params.c

+
+/**
+ * Time-out for auto disarm before the first take off
+ *


@RomanBapst What happens if they specify 0 - this case is not covered in doc.

hamishwillee · 2019-08-04T22:38:53Z

src/modules/commander/commander_params.c

+/**
+ * Time-out for auto disarm before the first take off
+ *
+ * A non-zero, positive value specifies the time-out period in seconds after which the vehicle will be


A non-zero, positive value specifies the time after arming, in seconds, within which the vehicle must take off (after which it will automatically disarm).

hamishwillee · 2019-08-04T23:13:28Z

src/modules/commander/commander_params.c

+ * @unit s
+ * @decimal 2
+ */
+PARAM_DEFINE_FLOAT(COM_DISARM_PRFLT,  2.0f);


@RomanBapst This seems way too short. IMO should be about 10 seconds (I think it was shortened to allow quick auto-disarm, but since they no longer need to match, 10 seconds is far more reasonable).

I agree with Hamish, we should not change the default value of 10 seconds

hamishwillee · 2019-08-04T23:25:08Z

Thanks for the heads up Roman! Docs PR created in PX4/PX4-user_guide#542 . There won't be any QGC docs changes (unless you add a safety page setting for the new param).

bresch

Looks good to me, I just don't understand why you changed the defaults.

bresch · 2019-08-05T07:12:14Z

src/modules/commander/commander_params.c

+PARAM_DEFINE_FLOAT(COM_DISARM_LAND, 5.0f);
+
+/**
+ * Time-out for auto disarm before the first take off


Why did you change the default value of 2 seconds?

bresch · 2019-08-05T07:12:58Z

src/modules/commander/commander_params.c

+ * @unit s
+ * @decimal 2
+ */
+PARAM_DEFINE_FLOAT(COM_DISARM_PRFLT,  2.0f);


I agree with Hamish, we should not change the default value of 10 seconds

catch-twenty-two · 2019-08-05T13:33:29Z

@bresch @hamishwillee I wasn't sure why the original values were chosen, I figured it was 10 and 2 because there wasn't an option to change the initial pre takeoff time to a desired value so compromises were made. I'll change them back to there original values, I should have pointed this decision out in my PR comments.

hamishwillee · 2019-08-06T00:25:06Z

I wasn't sure why the original values were chosen, I figured it was 10 and 2 because there wasn't an option to change the inital pre takeoff time to a desired value so a compromises were made.

@catch-twenty-two No worries. Thanks.

The times are a safety and usability compromise. For takeoff a new user (in particular) needs a bit of time after arming to take off - but not too long or they might wander over while the vehicle is armed. For landing you pretty much want disarming to happen immediately in most cases - I am not sure why 2s rather than 3 or 0.5, but it is pretty reasonable.

catch-twenty-two · 2019-08-08T13:17:32Z

@dagar it looks like the others have signed off on this, okay to merge?

RomanBapst · 2019-08-08T19:58:48Z

src/modules/commander/commander_params.c

+ * A non-zero, positive value specifies the time after arming, in seconds, within which the
+ * vehicle must take off (after which it will automatically disarm).
+ *
+ * A negative value means that automatic disarming triggered by a pre-takeoff timeout is disabled.


@hamishwillee @catch-twenty-two I believe it should be "A zero or negative value means that ..."

RomanBapst · 2019-08-08T19:59:51Z

@catch-twenty-two I think if you clarify on the parameter description that the auto disarm is disabled for value =0 then we can get this in.

Addressing parameter description syntax

catch-twenty-two self-assigned this Aug 2, 2019

dagar self-requested a review August 2, 2019 16:34

splitting preflight and land disarm times into 2 parameters

d8cec42

catch-twenty-two force-pushed the pr-spilt-pretakeoff-land-disarm-param-time branch from 7e42e9c to d8cec42 Compare August 2, 2019 16:41

Merge branch 'master' into pr-spilt-pretakeoff-land-disarm-param-time

59ea578

hamishwillee reviewed Aug 4, 2019

View reviewed changes

hamishwillee mentioned this pull request Aug 4, 2019

Clarify auto-disarm params PX4/PX4-user_guide#542

Merged

bresch requested changes Aug 5, 2019

View reviewed changes

updating pre-takeoff and landding values to defaults

1c4d081

catch-twenty-two force-pushed the pr-spilt-pretakeoff-land-disarm-param-time branch from 9464b36 to 1c4d081 Compare August 5, 2019 13:49

dagar previously approved these changes Aug 8, 2019

View reviewed changes

Merge branch 'master' into pr-spilt-pretakeoff-land-disarm-param-time

c76dcb2

bresch previously approved these changes Aug 8, 2019

View reviewed changes

RomanBapst reviewed Aug 8, 2019

View reviewed changes

Update commander_params.c

afb00c2

Addressing parameter description syntax

catch-twenty-two dismissed stale reviews from bresch and dagar via afb00c2 August 8, 2019 20:10

weekly-digest bot mentioned this pull request Aug 11, 2019

Weekly Digest (4 August, 2019 - 11 August, 2019) #12682

Closed

bresch approved these changes Aug 14, 2019

View reviewed changes

Merge branch 'master' into pr-spilt-pretakeoff-land-disarm-param-time

b39d703

bresch added Admin: Enhancement (improvement) 💡 Multirotor 🛸 labels Aug 14, 2019

bresch merged commit 9e3493b into master Aug 14, 2019

bresch deleted the pr-spilt-pretakeoff-land-disarm-param-time branch August 14, 2019 12:17

weekly-digest bot mentioned this pull request Aug 18, 2019

Weekly Digest (11 August, 2019 - 18 August, 2019) #12733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

splitting preflight and land disarm times into 2 parameters #12608

splitting preflight and land disarm times into 2 parameters #12608

catch-twenty-two commented Aug 2, 2019 •

edited

Loading

dagar commented Aug 2, 2019

catch-twenty-two commented Aug 2, 2019

RomanBapst commented Aug 3, 2019

RomanBapst commented Aug 3, 2019

RomanBapst commented Aug 3, 2019

hamishwillee Aug 4, 2019 •

edited

Loading

bresch Aug 5, 2019

hamishwillee Aug 5, 2019

hamishwillee Aug 4, 2019

hamishwillee Aug 4, 2019

hamishwillee Aug 4, 2019

bresch Aug 5, 2019

hamishwillee commented Aug 4, 2019

bresch left a comment

bresch Aug 5, 2019

bresch Aug 5, 2019

catch-twenty-two commented Aug 5, 2019 •

edited

Loading

hamishwillee commented Aug 6, 2019 •

edited

Loading

catch-twenty-two commented Aug 8, 2019

RomanBapst Aug 8, 2019

RomanBapst commented Aug 8, 2019

splitting preflight and land disarm times into 2 parameters #12608

splitting preflight and land disarm times into 2 parameters #12608

Conversation

catch-twenty-two commented Aug 2, 2019 • edited Loading

dagar commented Aug 2, 2019

catch-twenty-two commented Aug 2, 2019

RomanBapst commented Aug 3, 2019

RomanBapst commented Aug 3, 2019

RomanBapst commented Aug 3, 2019

hamishwillee Aug 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee commented Aug 4, 2019

bresch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catch-twenty-two commented Aug 5, 2019 • edited Loading

hamishwillee commented Aug 6, 2019 • edited Loading

catch-twenty-two commented Aug 8, 2019

Choose a reason for hiding this comment

RomanBapst commented Aug 8, 2019

catch-twenty-two commented Aug 2, 2019 •

edited

Loading

hamishwillee Aug 4, 2019 •

edited

Loading

catch-twenty-two commented Aug 5, 2019 •

edited

Loading

hamishwillee commented Aug 6, 2019 •

edited

Loading