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

fw_att_control: move ecl/attitude_fw into fw_att_control #14284

Merged
merged 1 commit into from Mar 15, 2020

Conversation

dagar
Copy link
Member

@dagar dagar commented Mar 2, 2020

Let's merge this back into PX4/Firmware. A large proportion of the code simply goes away if the work is handled in place.

ECL side (required first): PX4/PX4-ECL#765

First step is to copy in place as is, then we can incrementally refactor easily with the submodule gone.

@dagar
Copy link
Member Author

dagar commented Mar 2, 2020

@sfuhrer @RomanBapst once this is in let's refactor it in the style of mc_att_control/AttitudeControl and mc_rate_control/RateControl with unit tests.

sfuhrer
sfuhrer previously approved these changes Mar 3, 2020
Copy link
Contributor

@sfuhrer sfuhrer left a comment

Choose a reason for hiding this comment

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

I totally agree with moving it out of ecl, thanks for initializing that @dagar .

I made a quick pass on the code and sitl tested it with a plane and a standard vtol, looks right. I could also give it a quick spin on a real vehicle (VTOL) if you think that's already necessary at this stage.

There is no 1.11 release branch yet right? It that's planned in the (very) near future I guess we should wait with the FW refactoring we start with this, or how do you feel about it?

@@ -0,0 +1,145 @@
/****************************************************************************
*
* Copyright (c) 2013 Estimation and Control Library (ECL). All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

@dagar is it okay to keep the wrong header for now? Or should we at least change them, plus maybe also the file names (remove the "ecl" from them?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fine. They'll also be refactored out of existence at some point.

@LorenzMeier
Copy link
Member

What is keeping this PR from being merged? It concerns me if things just stop for a week. This makes the whole team really slow. We really need to be working on maximum 3-4 things at any given time, but get those things done and merged quickly.

@dagar
Copy link
Member Author

dagar commented Mar 10, 2020

What is keeping this PR from being merged?

In this particular case we got blocked by the yaw estimator (EKF-GSF) merging into PX4/ecl master before the required change here (PX4/PX4-ECL#765).

@dagar
Copy link
Member Author

dagar commented Mar 15, 2020

Other ECL change is out of the way, rebasing this PR to get it in.

dagar added a commit to PX4/PX4-ECL that referenced this pull request Mar 15, 2020
 - this was never maintained as a separate standalone project
 - PX4/PX4-Autopilot#14284
@dagar dagar force-pushed the pr-fw_attitude_control_ecl branch from 65f1a13 to fab55a2 Compare March 15, 2020 17:37
@dagar dagar merged commit 1a9452e into master Mar 15, 2020
@dagar dagar deleted the pr-fw_attitude_control_ecl branch March 15, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants