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

Add script for monitoring battery resistance #27571

Merged

Conversation

peterbarker
Copy link
Contributor

Closes #1898

@tpwrules
Copy link
Contributor

tpwrules commented Jul 17, 2024

Can you add docs too? I assume these are returned in Ohms.

Does this actually solve the problem? In my experience internal resistance estimates are pretty bad (5-10x expected values) until after takeoff and the expected current is being consumed. It's also easy to imagine that if the current sensor is slightly different the estimates could be unrealistically low too.

I'm not sure how a pre-arm check, especially after a fresh boot, would help. I would think you need to wait a couple seconds into flight or so, then perhaps switch to land mode if it's high. Past the first couple seconds it would be better to just warn the user.


local MAX_RESISTANCE = 0.03

local auth_id = arming:get_aux_auth_id()
Copy link
Member

Choose a reason for hiding this comment

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

You could assert this to pass ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain this in shorter words, please?

I am struggling with the cleanliness checks...

Copy link
Member

@IamPete1 IamPete1 Jul 20, 2024

Choose a reason for hiding this comment

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

This is the error:

scripts/battery_internal_resistance_check.lua:30:34
        Code: param-type-mismatch
        Cannot assign `integer|nil` to parameter `integer`.
        - `nil` cannot match `integer`
        - Type `nil` cannot match `integer`
scripts/battery_internal_resistance_check.lua:24:37
        Code: param-type-mismatch
        Cannot assign `integer|nil` to parameter `integer`.
        - `nil` cannot match `integer`
        - Type `nil` cannot match `integer`

The errors are for:

arming:set_aux_auth_passed(auth_id)

and

arming:set_aux_auth_failed(auth_id, msg)

The issue is that both functions are expecting auth_id to be a integer type. However it is declared as:

local auth_id = arming:get_aux_auth_id()

The problem is that arming:get_aux_auth_id() can fail and return nil if its run out of ids to give out. So auth_id could be a integer or it could be nil.

If we change the definition of auth_id to:

local auth_id = assert(arming:get_aux_auth_id(), "No auth IDs available")

The assert will error out if the returned auth_id is nil, so now the checker knows that auth_id must be a integer after the assert. The types then match and CI is happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert will error out if the returned auth_id is nil, so now the checker knows that auth_id must be a integer after the assert. The types then match and CI is happy.

Many thanks, I now understand a little more about LUA :-)

I've made the suggested change to this PR.

@peterbarker peterbarker force-pushed the pr/battery-high-resistance-failsafe branch from c06d38f to 965622b Compare July 20, 2024 10:22
@peterbarker
Copy link
Contributor Author

Can you add docs too? I assume these are returned in Ohms.

Docs have been added. I've annotated the variable.

Does this actually solve the problem? In my experience internal resistance estimates are pretty bad (5-10x expected values) until after takeoff and the expected current is being consumed. It's also easy to imagine that if the current sensor is slightly different the estimates could be unrealistically low too.

Not expected to fix things for your bog-standard copter for initial takeoff.

I'm not sure how a pre-arm check, especially after a fresh boot, would help. I would think you need to wait a couple seconds into flight or so, then perhaps switch to land mode if it's high. Past the first couple seconds it would be better to just warn the user.

You'll get a warning in-air and a pre-arm check once you disarm after landing. I started off writing a full-blown C++ thing, triggering a low-battery failsafe on a parameter-defined high-resistance value. Then I decided to punt to a simple script for now.

Fancier batteries might start to report a persistently-stored internal resistance, in which case you'd get protection before first takeoff.

@tpwrules
Copy link
Contributor

Docs have been added. I've annotated the variable.

Thank you. Could you specifically note in docs.lua that the return quantity is resistance in Ohms?

You'll get a warning in-air and a pre-arm check once you disarm after landing. I started off writing a full-blown C++ thing, triggering a low-battery failsafe on a parameter-defined high-resistance value. Then I decided to punt to a simple script for now.

My problem is that, if I put this script on any of my vehicles, they could never take off because the calculated internal resistance is always dangerously high after boot. It only settles to an accurate value a few seconds after takeoff. So I question the value of the pre-arm check; maybe my vehicles are an edge case?

@peterbarker peterbarker force-pushed the pr/battery-high-resistance-failsafe branch from 965622b to ef61822 Compare July 21, 2024 08:42
@peterbarker
Copy link
Contributor Author

Docs have been added. I've annotated the variable.

Thank you. Could you specifically note in docs.lua that the return quantity is resistance in Ohms?

Done.

My problem is that, if I put this script on any of my vehicles, they could never take off because the calculated internal resistance is always dangerously high after boot. It only settles to an accurate value a few seconds after takeoff. So I question the value of the pre-arm check; maybe my vehicles are an edge case?

We're starting to get into serious heuristics now, which is why this is currently and example. not an applet.

@peterbarker peterbarker merged commit 56773f0 into ArduPilot:master Jul 21, 2024
94 checks passed
@peterbarker peterbarker deleted the pr/battery-high-resistance-failsafe branch July 22, 2024 07:27
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.

Keep track of battery resistance and optionally warn if it's too low before arming
4 participants