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

Use filtered throttle for battery voltage compensation #9055

Merged
merged 2 commits into from
Apr 10, 2018

Conversation

nanthony21
Copy link
Contributor

@nanthony21 nanthony21 commented Mar 11, 2018

When an internal resistance for the battery has not been selected the firmware uses throttle and the BAT_V_LOAD_DROP parameter to compensate for the voltage drop due to load on the battery. The battery voltage has a delayed response to a change in load and it is further filtered to get rid of high-frequency noise. However the throttle value that is used in the calculation can change immediately, this means that when the throttle changes suddenly the compensation will be way off until the filtered voltage has had time to stabilize.

This issue can be seen at 18:24.75 in this log:
https://review.px4.io/plot_app?log=53b9938b-ad16-453a-b6f5-e0d5637eba66

When the quadcopter is switched from manual mode to position mode the throttle jumps from 100% to 6%. This causes the battery estimation to drop from ~40% down to 0%. It then slowly recovers back to 40% as the voltage rises.

This pull request replaces the battery voltage drop compensation's raw throttle value with a value that has been filtered at the same time constant as the battery voltage. This prevents the compensation from overshooting during sudden changes of throttle.

@dagar
Copy link
Member

dagar commented Mar 11, 2018

CI failure is a minor code style issue. If you install astyle locally you can run make check_format.

@nanthony21 nanthony21 changed the title User filtered throttle for battery voltage compensation Use filtered throttle for battery voltage compensation Mar 11, 2018
@nanthony21
Copy link
Contributor Author

Should be fixed now.

@nanthony21
Copy link
Contributor Author

@dagar I'm not quite sure what this next CI error is.

@nanthony21
Copy link
Contributor Author

@MaEtUgR I know you've worked a bit on battery stuff. Can you take a look at this?

@lamping7
Copy link
Member

CI error (intermittent land detector issue) can be disregarded as it is not related.

@dagar
Copy link
Member

dagar commented Mar 15, 2018

Note for later, but I think we should have Battery actually subscribe and gather the data it needs. The usage across different parts of the system has become a little inconsistent.

@dagar dagar requested a review from MaEtUgR March 15, 2018 17:25
@dagar
Copy link
Member

dagar commented Mar 15, 2018

@MaEtUgR could you take a quick look?

Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

The idea makes sense and the implementation looks good to me.

I would also make the sketchy guess that the estimation improves but more detailed tests would be nice.

@nanthony21
Copy link
Contributor Author

I haven't been able to do a proper flight test but I have these two propless bench tests to compare the current implementation to the one in this PR.

The battery voltage drop parameter isn't set quite right so you can see that the battery estimate changes depending on the throttle setting. However, in the second log you can see that a sudden change in throttle is no longer able drop the battery estimate before the battery voltage has had time to stabilize.

Current implementation: https://review.px4.io/plot_app?log=05a9f3b6-190f-482b-aaa6-9a21677ab2bb
Pull Request: https://review.px4.io/plot_app?log=91d31adf-d754-44e1-8d0d-4a9cc91e20aa

Once I am able to do a proper flight test the benefits of the this pull request should be easier to see.

@nanthony21
Copy link
Contributor Author

@dagar @MaEtUgR Here is a log from a flight today using this PR. As you can see the remaining battery estimate is much more stable than in the log from my original post.

https://review.px4.io/plot_app?log=20ea78d4-aa8c-4fe9-bceb-daa0f286683c

@MaEtUgR MaEtUgR merged commit 0c2b9dc into PX4:master Apr 10, 2018
@MaEtUgR
Copy link
Member

MaEtUgR commented Apr 10, 2018

@nanthony21 Thanks for the logs, it looks good. I can see in the log that you have current sensing in your setup and I would suggest to you to read https://docs.px4.io/en/config/battery.html and use the current based compensation and fusion with current integration (if you have a fixed size battery) for best results.

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.

5 participants