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

Rover: add arm-once-after-prearms-pass functionality #29066

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

peterbarker
Copy link
Contributor

Add option to Rover to arm the vehicle once, once prearm (and arm!) checks have passed.

This is intended to be an alternative to having an ARMING_REQUIRE of 0, which can cause all sorts of fun which is best avoided.

Also adds an "REQUIRE_POSITION" as a DriveOptions setting for Rover, the same as Copter's "REQUIRE_POSITION" functionality.

Tested in SITL.

autotests coming, assuming this doesn't get insta-killed...

@IamPete1
Copy link
Member

IamPete1 commented Jan 14, 2025

Doesn't have the advantage of your get_last_prearm_checks_result but this does work:

-- Arm the vehicle one time once arming checks have passed
local function update()

    if arming:is_armed() then
        -- Kill script once armed
        return
    end

    if arming:pre_arm_checks() and arming:arm() then
        -- Kill the script once it works
        return
    end

    -- try again in 5 seconds
    return update, 5000
end

return update, 5000

@peterbarker peterbarker force-pushed the pr/rover-ARMING_REQUIRE-kinda branch from 7ee5d74 to 9e2a581 Compare January 14, 2025 23:22
@@ -694,6 +694,12 @@ const AP_Param::GroupInfo ParametersG2::var_info[] = {
// @Path: mode_circle.cpp
AP_SUBGROUPINFO(mode_circle, "CIRC", 57, ParametersG2, ModeCircle),

// @Param: DRIVE_OPTIONS
Copy link
Contributor

@rmackay9 rmackay9 Jan 15, 2025

Choose a reason for hiding this comment

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

I'd like to avoid this new parameter if possible. I think the user could achieve essentially the same thing by setting the INITIAL_MODE parameter to a mode that requires a position estimate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point here is to require that the vehicle has position in all modes. So the user can't arm the vehicle at all until it knows how to get home.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I definitely understand the need for this now and we have the same option in Copter. I just wonder why we put it into FLIGHT_OPTIONS instead of ARMING_OPTIONS as part of PR #27367

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I definitely understand the need for this now and we have the same option in Copter. I just wonder why we put it into FLIGHT_OPTIONS instead of ARMING_OPTIONS as part of PR #27367

I think we did discuss at DevCall - but I can't find any notes on it.

It probably should be in arming options. Should I move it? This will require some awkward code in Copter if we want people to start using the bit in ARMING_OPTIONS. But I think we should.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @peterbarker yes, I'm sorry but I agree that we should move it. It's new for 4.6 so I think we could get away without the parameter conversion if we backported it before the official release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.... actually, I think the problems revolved around Plane.

If we ever allow Plane to arm without position (@Ryanf55 ...) we would have issues around retaining the default for Plane. parameter defaults and bitmasks are a mess - if someone already has set ARMING_OPTION in their parameter storage then you can't have sensible default values on a per-vehicle basis (or indeed any non-zero bit default).

Copy link
Contributor

Choose a reason for hiding this comment

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

For copter and rover the default is zero (e.g. require GPS for arming is false) but I guess plane would like that to default to 1. It's sad to put the option in the wrong place because of this though. I understand the issue but.. it's hard to accept doing the wrong thing for a small technical reason

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good details, I need time to digest, but am swamped at work. If you aren't in a rush, I can discuss in 2 days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR to move the bit into AP_Arming: #29076

@rmackay9
Copy link
Contributor

rmackay9 commented Jan 15, 2025

Thanks for this, it would be great if we could eventually remove the arming-required =0 option

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WikiNeeded needs wiki update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants