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

Copter: split init() into ok_to_enter and enter() #7402

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
6 participants
@peterbarker
Copy link
Contributor

commented Dec 13, 2017

This change allows the base mode object to do some checks for the derived classes.

It also ensures we either don't play with the controllers or switch modes.

I'm looking for suggestions on gcs().send_text(...) in a const function!

@peterbarker peterbarker added the WIP label Dec 13, 2017

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 2 times, most recently from 520fa1d to f9db925 Dec 13, 2017

@OXINARF OXINARF added the Copter label Dec 14, 2017

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 2 times, most recently from e5427ba to 0a066a1 Dec 14, 2017

@OXINARF
Copy link
Member

left a comment

@peterbarker I'm wishing to hurt you after looking at this PR 😝 How many doesn't the base implementation do this already did I write and have now deleted? 😄 Please do squash last two commits with commit number 2 🙂

#endif

if (requires_GPS()) {
if (!(_copter.position_ok() || _copter.motors->armed())) {

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

Isn't this equivalent to if (!_copter.position_ok() && !_copter.motors->armed()) or am I seeing this wrong? If it is equivalent, then I think it is wrong, it's not OK to enter when it doesn't have position and it is armed.

I'm not very comfortable with this check though - maybe it's just the names. If the mode requires GPS, then position_ok will only guarantee that a relative position is available, not an absolute position.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

I was musing with Randy on this yesterday, actually. requires_GPS() is a very bad name which we should fix at some stage.

I was musing on requires_velocity3f(), for example, for Drift mode. Randy suggested just requires_relative_position() and requires_absolute_position()

AFAIK these checks are still equivalent to what we have - it just makes it clearer where we need improvement.

I will fix the logic by breaking out a subroutine.

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

I think that we should add as many as necessary methods. If Drift only requires 3D velocity then that's what we should check.

// reject switching to auto mode if landed with motors armed but first command is not a takeoff (reduce chance of flips)
if (motors->armed() && ap.land_complete && !mission.starts_with_takeoff_cmd()) {
gcs().send_text(MAV_SEVERITY_CRITICAL, "Auto: Missing Takeoff Cmd");
// gcs().send_text(MAV_SEVERITY_CRITICAL, "Auto: Missing Takeoff Cmd");

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

I think we need to leave this in, even if const needs to be removed.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

Can we come up with some other /awfully clever/ plan?

One option would be to pass in a const char *& as we do in some places for arming.

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

I would love to give a better option but I can't think of one. Honestly passing the variable to fill seems worse to me than the method not being const, but I don't really care much - as long as this message stays in, I'm fine with whatever solution.

if ((_copter.position_ok() && mission.num_commands() > 1) || ignore_checks) {
_mode = Auto_Loiter;

if ((_copter.position_ok() && mission.num_commands() > 1) || _copter.motors->armed()) {

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member
  • I think it would be good to separate the first check to just be if (motors->armed() && mission.num_commands <= 1) { return false; }
  • Isn't motors->... enough? That's in the line below. Also, shouldn't it be !armed? This could be removed though if the point above is done.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

I was hoping to avoid rewriting this logic :-) I'll do so..

@@ -43,10 +49,6 @@ bool Copter::ModeAuto::init(bool ignore_checks)

// start/resume the mission (based on MIS_RESTART parameter)
mission.start_or_resume();

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

Wrong indentation above.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

Yep, I was aware of that, but didn't want to churn the code unnecessarily.

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

Maybe a final commit fixing the indentations? They shouldn't be left like that.

@@ -26,11 +33,6 @@ bool Copter::ModeBrake::init(bool ignore_checks)
}

_timeout_ms = 0;

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

Wrong indentation above.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

Yep, again, trying to avoid code churn.

pos_control->set_alt_target_to_current_alt();
pos_control->set_desired_velocity_z(inertial_nav.get_velocity_z());
}

return true;

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

Shouldn't this call base implementation?

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

Yes, fixed.

}

// only do position hold if starting autotune from LOITER or POSHOLD
use_poshold = (_copter.control_mode == LOITER || _copter.control_mode == POSHOLD);
have_position = false;

return success;
// initialize vertical speeds and leash lengths

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

I'm not sure if this has any impact, but before this was run prior to backup_gains_and_initialise/load_intra_test_gains.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

I don't believe there's an impact.

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

Besides the previously identified order change, this has a new behavior in case mode == SUCCESS: the code below looks harmless to me, but wasn't run before, so either @rmackay9 or @lthall should look at it before merging.

This comment has been minimized.

Copy link
@lthall

lthall Jan 11, 2018

Contributor

I think this is ok.

break;

case SUCCESS:
// we have completed a tune and the pilot wishes to test the new gains in the current flight mode
// so simply apply tuning gains (i.e. do not change flight mode)
load_tuned_gains();
Log_Write_Event(DATA_AUTOTUNE_PILOT_TESTING);
break;
return;

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

Humm, this won't allow the two lines below to run. I think that isn't good as those should be initialized too.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

True. Not sure what I was thinking there.

@@ -563,7 +563,8 @@ class ModeBrake : public Mode {
Copter::Mode(copter)
{ }

bool init(bool ignore_checks) override;
bool ok_to_enter() const override;

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

This has no implementation, should be removed.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

Will do.

}else{
return false;
}
// insert missing initialisations here

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 15, 2017

Member

Add a TODO: in beginning of comment? Easier to look up.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 15, 2017

Author Contributor

Yep

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from 0a066a1 to 18a4ac0 Dec 15, 2017

@amilcarlucas
Copy link
Contributor

left a comment

I do not get that comment

bool Copter::Mode::ok_to_enter_gps_checks() const
{
if (requires_GPS()) {
// this mode doesn't require position information

This comment has been minimized.

Copy link
@amilcarlucas

amilcarlucas Dec 16, 2017

Contributor

I do not understand this comment

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 16, 2017

Author Contributor

Fixed.

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from 18a4ac0 to 3950764 Dec 16, 2017

// this mode doesn't require position information
return true;
}
if (!_copter.motors->armed()) {

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

Sorry for insisting, but isn't motors-> enough? That _copter already annoys me (copter is a global object already, we didn't need a reference in modes), but even worse is inconsistent code. In the new ``ok_to_enter` it uses just motors, so can we do it here too?

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 25, 2017

Author Contributor

Found two to fix.

{
if ((_copter.position_ok() && mission.num_commands() > 1) || ignore_checks) {
_mode = Auto_Loiter;
if (mission.num_commands() < 2) {

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

It's not a big issue, but this is a change to previous behaviour. Before if you didn't have 2 commands you could enter Auto when disarmed. My recommendation would be to put the code as follows:

if (motors->armed()) {
  if (mission.num_commands() < 2) {
    return false;
  }
  if ((!ok_to_enter_check_takeoff_cmd()) {
   return false;
 }
}
[...]

And remove the armed check inside ok_to_enter_check_takeoff_cmd.

This comment has been minimized.

Copy link
@peterbarker

peterbarker Dec 25, 2017

Author Contributor

OK.

}

// only do position hold if starting autotune from LOITER or POSHOLD
use_poshold = (_copter.control_mode == LOITER || _copter.control_mode == POSHOLD);
have_position = false;

return success;
// initialize vertical speeds and leash lengths

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

Besides the previously identified order change, this has a new behavior in case mode == SUCCESS: the code below looks harmless to me, but wasn't run before, so either @rmackay9 or @lthall should look at it before merging.

@@ -653,7 +662,7 @@ class ModeGuidedNoGPS : public ModeGuided {
ModeGuidedNoGPS(Copter &copter) :
Copter::ModeGuided(copter) { }

bool init(bool ignore_checks) override;
void enter() override;
void run() override;

bool requires_GPS() const override { return true; }

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 18, 2017

Member

I'm not sure you got my comment here. I wasn't referring to the method name, but the fact that it is returning true in a method that doesn't require GPS.

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@OXINARF

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

@peterbarker

It doesn't require GPS, but does require positioning (position_ok()).
requiresGPS should be requiresLocalPosition()
(or requiresRelativePosition()?), and we need a new requiresGPS() when you
really do need latitude/longitude (i.e. a Global Position!)

No, it doesn't, that's what I'm saying. It only accepts attitude commands, doesn't need position for anything.

If you like. So long as the indentation is consistent within the function
it doesn't bother me - just something to fix if the function gets reworked
IMO.

Sorry, I'm sensitive 😄 It does bother me so I appreciate the patch 🙂

The reason I was so keen on const is that it provides some sort of
guarantee that your ok_to_enter() isn't creating the same bug we had for
the heli checks, where state was changed but no mode change occured.

Yep, I like when we can const methods too! It's like using override, it might be useless most of the time, but it avoids doing bugs later.

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from 3950764 to af9e7ce Dec 25, 2017

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2017

@OXINARF Points addressed except for the reindenting of the enter function in autotest; reindenting that would make it harder for @lthall or @rmackay9 to look at that change.

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from af9e7ce to 264a7c8 Dec 25, 2017

@OXINARF
Copy link
Member

left a comment

@peterbarker OK, looks almost good to merge. Let's try to get @rmackay9/@lthall's approval soon.

@@ -27,7 +32,7 @@ bool ModeSmartRTL::_enter()
smart_rtl_state = SmartRTL_WaitForPathCleanup;
_reached_destination = false;

return true;
ModeSmartRTL::enter();

This comment has been minimized.

Copy link
@OXINARF

OXINARF Dec 27, 2017

Member

Mode::enter()

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 5 times, most recently from 4aca0d2 to 90da1fb Dec 27, 2017

@peterbarker peterbarker removed the WIP label Dec 28, 2017

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from 90da1fb to 67f49d2 Dec 28, 2017

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 2 times, most recently from 6bf816b to 163acc8 Jan 10, 2018

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from ec74f70 to 8c4df95 Feb 26, 2018

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 2 times, most recently from d298332 to e90be91 Mar 16, 2018

@amilcarlucas

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2018

This one is starting to bitrot. And the symptoms are ... merge conflicts.

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 2 times, most recently from 2b7a168 to c63b623 Apr 6, 2018

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 5 times, most recently from 7e1a420 to 1f07492 Jun 4, 2018

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

I've updated this to use snprintf in place of asprintf, which makes it like the rest of our core. tridge rightly pointed out that allocating memory in the hope someone else will free it isn't a great look.

if (_mode_rtl.enter()) {
_submode = Auto_RTL;
char reason[MAVLINK_MSG_STATUSTEXT_FIELD_TEXT_LEN] = {};
if (!_mode_rtl.ok_to_enter(reason, sizeof(reason))) {

This comment has been minimized.

Copy link
@khancyr

khancyr Jun 7, 2018

Contributor

shouldn't this be sizeof(reason-1) as snprintf will append the \0 at the end ? otherwise we risk an overflow

This comment has been minimized.

Copy link
@peterbarker

peterbarker Jul 30, 2018

Author Contributor

That's not typical of this sort of interface.

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from 1f07492 to 11dc6e7 Jun 12, 2018

@amilcarlucas

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

No merge conflicts here :)

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch 3 times, most recently from e8f7b5a to 442ac43 Jul 26, 2018

@peterbarker peterbarker force-pushed the peterbarker:ok-to-enter branch from 442ac43 to bab8168 Oct 24, 2018

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

This will need to be redone from scratch if we want to do it.

Some of the changes have been done in other ways, some are PR'd elsewhere in other ways,

@magicrub

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

This reminds me of the plane onion!

@peterbarker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.