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

Autotest: make RTL and Land wait a little more verbose, don't use raw reboot on fly_battery_failsafe #14882

Merged
merged 4 commits into from Aug 29, 2020

Conversation

khancyr
Copy link
Contributor

@khancyr khancyr commented Jul 22, 2020

also correct flip test that wasn't resetting the attitude correctly

@khancyr
Copy link
Contributor Author

khancyr commented Aug 6, 2020

@peterbarker some fixs for the autotest. Could have a look at them. nothing worring.
I am thinking to change the wait_land to something more robust (checking hit ground message and LANDED_STATE) as currently we are only guessing what the land altitude will be....

Tools/autotest/arducopter.py Outdated Show resolved Hide resolved
Tools/autotest/arducopter.py Outdated Show resolved Hide resolved
Tools/autotest/arducopter.py Outdated Show resolved Hide resolved
@khancyr khancyr force-pushed the test_rtl branch 2 times, most recently from 713dc40 to b0928d6 Compare August 11, 2020 10:53
@khancyr
Copy link
Contributor Author

khancyr commented Aug 11, 2020

comments addressed and ready to go

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

There are several good changes in here.

It's a bit of a pity they're all mixed in as it means I'm reviewing perfectly good changes multiple times, and they're being held up for other stuff :-)

One thing you may not have been aware of - we've had CI failures because of too much log output. Something to bare in mind.

except Exception as e:
ex = e

self.set_parameter('BATT_LOW_VOLT', 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use a context here rather than assuming this list of parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done like that so I did it the same way.
I can use the context

@khancyr
Copy link
Contributor Author

khancyr commented Aug 12, 2020

I have split the PR into multiple to not hold the ready ones

peterbarker
peterbarker previously approved these changes Aug 13, 2020
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

I've tweaked the commit messages.

@peterbarker
Copy link
Contributor

Looks like the check for altitude needs to accept that the vehicle might start off on the ground.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

There still appears to be a new failure in the heli test in this PR.

@peterbarker
Copy link
Contributor

I've rebased this and taken out the expectation of the statustext. That can reappear later if you can get the heli test passing.

@peterbarker peterbarker merged commit 084ec2b into ArduPilot:master Aug 29, 2020
@peterbarker
Copy link
Contributor

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants