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

Do not report battery capacity remaining for Sub #15096

Closed

Conversation

Williangalvani
Copy link
Contributor

Subs can easily do multiple flights on one battery, or start flights on a half-charged battery. In these cases integrating the battery current would give a false percentage.

This makes Sub always return -1 for battery capacity remaining. QGC interprets the "-1" as "Battery remaining energy not sent by autopilot" as defined in SYS_STATUS spec, and displays voltage instead.

This PR will make users monitor the battery voltage instead of the percentage reported.

@WickedShell
Copy link
Contributor

@Williangalvani I'm not a sub maintainer, so I'm not as invested, but I'm not sure if this is really what you want here. The three things that stick out to me:

  • You can't use the current failsafes nearly as easily if you don't get the percentage reported to you, yes you need to use a full battery for this
  • The GCS is still told how much capacity has been used, and it knows how much is available via the params, so I'm not sure you can assume you won't have a GCS which won't generate a percentage out of this anyways
  • You can actually reset the battery driver from the GCS over MAVLink to be at the right percentage. So if you are starting from a non fully charged battery it's actually possible to make it be correct, which would give you the best of both worlds.

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.

It does seem odd killing this for every sub out there. Surely someone, somewhere would find it useful.

@WickedShell what's the mechanism used to reset the battery? Do you know if QGC supports it?

ArduSub/GCS_Mavlink.h Outdated Show resolved Hide resolved
@peterbarker
Copy link
Contributor

@peterbarker https://github.com/ArduPilot/ardupilot/blob/master/libraries/GCS_MAVLink/GCS_Common.cpp#L3863

OK, so that's ardupilotmega.xml, so @DonLakeFlyer won't support it. Perhaps we should look at getting that one back into common.xml?

@DonLakeFlyer
Copy link
Contributor

QGC mainly uses BATTERY_STATUS and falls back to SYS_STATUS if that is all it gets. Don't know if that solves you problem or not.

@jaxxzer
Copy link
Member

jaxxzer commented Aug 20, 2020

This change is in line with the functionality (or lack thereof) that has been present in every ArduSub stable release. We have always populated the sys_status.current_remaining field with -1, no matter what.

ArduPilot's current estimation algorithm is unsuitable for ArduSub. We cannot assume that the battery is fully charged at boot; see #1434. It is extremely common to boot without a fully charged battery, for example performing multiple dives during the day on a single charge (most of our users have 18000mAh batteries).

The issue is that unless the battery is completely charged at boot, ArduPilot and QGC will mislead the user into believing there is more life in the battery than in reality. The risk+confusion and potential for accidents or loss is not worth supporting this feature. Battery voltage is enough information to meet the same end of determining when the battery is out of juice - and it's secure/reliable. QGC displays the battery telemetry with a capacity estimate if that number is reported, otherwise QGC will display the voltage. We want QGC to always display voltage information, so we never report a capacity remaining estimate.

image
image

You can't use the current failsafes nearly as easily if you don't get the percentage reported to you, yes you need to use a full battery for this

As above, the problems with this assumption outweigh the utility of the current failsafes.

The GCS is still told how much capacity has been used, and it knows how much is available via the params, so I'm not sure you can assume you won't have a GCS which won't generate a percentage out of this anyways

QGC is the only GCS recommended for ArduSub. @bluerobotics/software-team
maintains support for ArduSub in QGC. I don't know of any other GCS that supports ArduSub.

You can actually reset the battery driver from the GCS over MAVLink to be at the right percentage. So if you are starting from a non fully charged battery it's actually possible to make it be correct, which would give you the best of both worlds.

The root issue is still present in this situation. If the user neglects to adjust - accidents will happen.

@jaxxzer
Copy link
Member

jaxxzer commented Aug 20, 2020

We can reconsider when the capacity remaining algorithm accounts for the fact that the battery may not be fully charged at boot.

Subs can easily do multiple flights on one battery, or start flights on a
half-charged battery. This makes the user monitor the battery voltage instead
of the (possibly bad) percentage reported by Ardupilot.
@jaxxzer
Copy link
Member

jaxxzer commented Aug 26, 2020

ping @peterbarker to make sure things look ok for gcscommon

@tridge
Copy link
Contributor

tridge commented Aug 27, 2020

@Jaaaky we had this same issue in Matternet, and solved it by initialising the capacity using a table:
https://github.com/matternet/ardupilot/blob/mttr-rebase-4.0/libraries/AP_BattMonitor/AP_BattMonitor_Analog_Table.cpp
I did raise this as an option for including in mainline ArduPilot, but there wasn't much interest in it on the dev call at the time. The key issue was that the table is fairly specific to the type of battery.
I can see several methods though:

  • provide the table in a lua script
  • have a small set of params that defines the shape of the curve

When using the table it inits the capacity when disarmed, then after that calculates capacity based on integrated current, starting from the table value

@hendjoshsr71
Copy link
Member

@Williangalvani Can this one be closed? Looks like: #15406 is the same as this one and got merged?

@Williangalvani
Copy link
Contributor Author

@Williangalvani Can this one be closed? Looks like: #15406 is the same as this one and got merged?

That was merged into a Sub branch, we still need it to make its way to master. I'll try to pick this back up this month.

@hendjoshsr71
Copy link
Member

Opps didn't notice that part sorry!

@Williangalvani
Copy link
Contributor Author

closing in favor of #13712

@Williangalvani Williangalvani deleted the no-percentage-for-sub branch September 8, 2021 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants