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

Adds is_armable property. #430

Merged
merged 2 commits into from
Nov 10, 2015
Merged

Adds is_armable property. #430

merged 2 commits into from
Nov 10, 2015

Conversation

tcr3dr
Copy link
Contributor

@tcr3dr tcr3dr commented Nov 7, 2015

Questions:

  • Should we throw if armed is set and we are not is_armamble?

See #416.

@tcr3dr tcr3dr changed the title Adds is_armable property. [WIP] Adds is_armable property. Nov 7, 2015
@tcr3dr tcr3dr force-pushed the tcr-addarm branch 2 times, most recently from ac045d5 to 966d581 Compare November 7, 2015 01:23
# check that mode is not INITIALSING
# check that we have a GPS fix
# check that EKF pre-arm is complete
return self.mode != 'INITIALISING' and self.gps_0.fix_type > 1 and self._ekf_predposhorizabs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think arming requires a 3D Fix (on Copter at least) based on various comments around the place. Should be checked. And yes, I know my tests look at <2 ... I do wonder though if perhaps the 3D fix happens very quickly after 2d fix and I've just got lucky.

@hamishwillee
Copy link
Contributor

Should we throw if armed is set and we are not is_armamble?

I think so. They really do need to make this check because there is no other way to check on ekf warmup. That means that not checking is a programmatic error, which is where you want to throw (to force good programming practice). That said, if we didn't, I think everyone is going to copy our example code, so the same thing will happen by stealth.

Originally you were talking about publishing a reason so people could work out why things were failing. Decided not to do that? I guess we can publish the checks.

@tcr3dr
Copy link
Contributor Author

tcr3dr commented Nov 9, 2015

I don't want to delve too heavily into "feature development" around this, but to address the reason question, maybe this should be a different function:

(armable, reasons) = vehicle.arm_check()

Reasons can be a dict of checks that were performed, armable can be a simple boolean value.

In reality, we also do nothing to allow you to read the acknowledgement value which is how we should be doing this—vehicle.armed = True should throw if the acknowledgement fails, but we currently don't check for that.

Given that ACKs would be really nice to integrate but is a breaking change and not well thought out, I'm a bit of a loss on what to do here. arm_check() and infrastructure for checking arming? Implement ACKs? Implement a second, explicit mechanism .acknowledge() that throws if the command failed?

@hamishwillee
Copy link
Contributor

@tcr3dr As you say, we could go around this all day.

I guess what I really want is to be able to have a simple instruction that tells the system to arm that I can wait on until arming is confirmed to succeed. It should have (optional) debug output on what we are waiting on and a time timeout or fail retry timout.

So something like the below, where debug rate is the spacing between printing conditions you're waiting on to console (None for don't print), retry is how many times it will try resend the arm command after EKF warmup is complete.

arm(await=True, retry=3, debug_rate=None)

Which returns after arming is detected.

I see the method we have discussed as a primitive for this sort of functionality. Which is why I quite like the ability to get the reasons (though only the ones that are holding us up or we are waiting on).

Does that help? If not, lets keep it simple - deliver this exactly as currently specced, no throw, no checking, no reasons.

@tcr3dr
Copy link
Contributor Author

tcr3dr commented Nov 10, 2015

Considering how much work this involves, and that we can do it in a backward-compatible way, I'm going to classify this as "enhancement" and go ahead with is_armable as we've specced. I'd love to develop a more coherent response to awaiting data and acknowledgement (particularly in Python) but that could take some time. No time to lose!

@tcr3dr tcr3dr changed the title [WIP] Adds is_armable property. Adds is_armable property. Nov 10, 2015
@hamishwillee
Copy link
Contributor

@tcr3dr OK. I've added the docs for this, and updated the examples which have a takeoff to use it. Merge at will!

tcr3dr added a commit that referenced this pull request Nov 10, 2015
@tcr3dr tcr3dr merged commit dc35975 into master Nov 10, 2015
@tcr3dr tcr3dr deleted the tcr-addarm branch November 10, 2015 03:35
@tcr3dr tcr3dr mentioned this pull request Nov 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants