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

delete control_state and cleanup vehicle_attitude #7882

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Conversation

dagar
Copy link
Member

@dagar dagar commented Aug 30, 2017

Continuation of #7773. The rebase was becoming too difficult. I can split this if necessary.

@@ -166,6 +168,7 @@ class MulticopterAttitudeControl
struct battery_status_s _battery_status; /**< battery status */
struct sensor_gyro_s _sensor_gyro; /**< gyro data before thermal correctons and ekf bias estimates are applied */
struct sensor_correction_s _sensor_correction; /**< sensor thermal corrections */
struct sensor_corrected_s _sensor_corrected; /**< sensor in-run bias corrections */
Copy link
Member

Choose a reason for hiding this comment

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

@dagar sensor_correction and sensor_corrected is a terrible choice - could we change to sensor_bias instead? Because that is what it really is.

Copy link
Member Author

Choose a reason for hiding this comment

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

That came in after what I originally did, but it sounds fine to me. There are a few users of the bias correct accel though.
I understand why we're doing it, but this entire sensor reconstruction in the rate controller feels awkward. I'll do the bias rename for now, but I'd like to discuss other options later on.

LorenzMeier
LorenzMeier previously approved these changes Aug 31, 2017
@LorenzMeier
Copy link
Member

@PX4TestFlights Please re-test. Thanks!

@santiago3dr
Copy link

couple flights with pixracer (v4)
https://logs.px4.io/plot_app?log=029a9c70-a993-4a2c-a9a8-2f7552eefd5f
https://logs.px4.io/plot_app?log=e231541f-ac9b-4764-b58d-8654db510203

good flights; steady flights; no notable difference from current master

@Avysuarez
Copy link

Avysuarez commented Aug 31, 2017

flight with pixhawk 1 (V2)
https://review.px4.io/plot_app?log=1ce0ca12-3572-4a91-bbfd-3be441829cb8

flight with pixhawk mini (V3)
https://review.px4.io/plot_app?log=505b613a-3be9-472f-85a0-b5cfeb83088b

good flights, no issues

@dannyfpv
Copy link

today flight with pix (v4-pro)
https://review.px4.io/plot_app?log=ac90f677-e46e-4527-8acf-39e1a483c8ca
flight = good
mission= Minor issues, acceptable
rtl= great
altitude=good
loiter= great

@Avysuarez
Copy link

@dagar
Copy link
Member Author

dagar commented Sep 1, 2017

Thanks guys, @dannyfpv could you expand on the minor mission issues?

@sanderux let's verify this doesn't break your VTOL setup.

I think we're close to merging!

@sanderux
Copy link
Member

sanderux commented Sep 1, 2017

@dagar i will try to fly this tomorrow, can you rebase?

@dagar
Copy link
Member Author

dagar commented Sep 1, 2017

Done

@dagar
Copy link
Member Author

dagar commented Sep 5, 2017

Rebased yet again. Did you get a chance to test this @sanderux?

@sanderux
Copy link
Member

sanderux commented Sep 5, 2017

Sorry, no, while in the field we had all but a usb cable. in the next few days i will.

LorenzMeier
LorenzMeier previously approved these changes Sep 11, 2017
@dagar
Copy link
Member Author

dagar commented Sep 16, 2017

Rebased again.

@r0gelion
Copy link
Contributor

@dagar Do you need @PX4TestFlights to test this again?

@dagar
Copy link
Member Author

dagar commented Sep 18, 2017

I think we're mostly good on testing other than VTOL without an airspeed sensor.

@LorenzMeier
Copy link
Member

@dagar would you mind rebasing again?

Copy link
Member

@sanderux sanderux left a comment

Choose a reason for hiding this comment

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

@dagar dagar merged commit b475529 into master Sep 21, 2017
@dagar dagar deleted the pr-att-bias-cleanup3 branch September 21, 2017 20:24
@mhkabir
Copy link
Member

mhkabir commented Sep 22, 2017

Finally! Woohoo!

@nicolaerosia
Copy link
Contributor

@dagar could you please summarize how this impacts sensor drivers? The diff is pretty big and is all in one commit.

@dagar
Copy link
Member Author

dagar commented Sep 24, 2017

@nicolaerosia this was mainly a topic cleanup. There was a significant amount of duplicated data being published by the estimators in a topic called control_state.

I apologize for the enormous diff. This dragged out for a long time across multiple people and rebasing became unmanageable.

It shouldn't have any impact on sensor drivers.

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.

10 participants