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.

Sign up for GitHub

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

Jump to bottom

AP_BattMonitor: add option minimum volt option #28148

Closed
wants to merge 1 commit into from
Closed

AP_BattMonitor: add option minimum volt option #28148

wants to merge 1 commit into from
+24 −2

Conversation

loki077
Copy link
Contributor

@loki077 loki077 commented Sep 19, 2024

Allows aggregate monitors like Sum and ESC to report the minimum voltage instead of the average voltage. This is useful for systems with multiple isolated batteries where the lowest voltage is the limiting factor.

@robertlong13

@IamPete1
Copy link
Member

You can setup your low voltage failsafes on the individual battery monitors before combining them in the sum. That is not a option for ESC of course.

I think the problem here is that the consumed energy calculation will be thrown off. Maybe you could apply the min voltage after the update_consumed call?

@robertlong13
Copy link
Collaborator

robertlong13 commented Sep 19, 2024 •
edited
Loading

That works for failsafes, but we wanted to display the relevant number easily to the pilot. That could be done with GCS customizations instead I suppose, but this felt nicer.

Good point about the consumed energy calculation. We can revise so that this option doesn't impact that calc.

Hwurzburg
Hwurzburg requested changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@Hwurzburg Hwurzburg left a comment

Choose a reason for hiding this comment

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

this should not be included on minimize boards..

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Sep 19, 2024
@robertlong13
Copy link
Collaborator

this should not be included on minimize boards..

If we need to go this route to prevent this 96 byte change (for Plane on Pixhawk1-1M), then I'd recommend we go further and save 200 bytes over the current master by disabling BATT#_OPTIONS entirely on minimized boards. That would certainly be a lot clearer from a documentation standpoint, since compiling out one bit of an options parameter can really lead to confusion. It also makes it much easier to explain in build_options.py

I can make that a part of this PR if we want.

@Hwurzburg
Copy link
Collaborator

this should not be included on minimize boards..

If we need to go this route to prevent this 96 byte change (for Plane on Pixhawk1-1M), then I'd recommend we go further and save 200 bytes over the current master by disabling BATT#_OPTIONS entirely on minimized boards. That would certainly be a lot clearer from a documentation standpoint, since compiling out one bit of an options parameter can really lead to confusion. It also makes it much easier to explain in build_options.py

I can make that a part of this PR if we want.

I agree, the options are not used in the minimized boards since its difficult( if not impossible )on some boards to use more than one monitor or a non-analog one(usually built-in) ....thats why everything except analog is omitted from minimize builds....we should make the SUM backend a build option while we are at it like the others are

@peterbarker
Copy link
Contributor

Wait - getting rid of BATTn_OPTIONS would get rid of value 64, use-resting-voltage. That would be a very bad change IMO.

I suggest we accept this PR without fussing too much about the 100 bytes.

IamPete1
IamPete1 reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Basically looks good.

libraries/AP_BattMonitor/AP_BattMonitor_ESC.cpp Outdated
@@ -133,6 +137,12 @@ void AP_BattMonitor_ESC::read(void)
update_consumed(_state, dt_us);

}

// Check if we want to report the minimum voltage instead of the average
// (we do this after calculating consumed Wh so this doesn't affect that calculation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// (we do this after calculating consumed Wh so this doesn't affect that calculation
// (we do this after calculating consumed Wh so this doesn't affect that calculation)

libraries/AP_BattMonitor/AP_BattMonitor_Params.cpp Outdated
@@ -154,7 +154,7 @@ const AP_Param::GroupInfo AP_BattMonitor_Params::var_info[] = {
// @Param: OPTIONS
// @DisplayName: Battery monitor options
// @Description: This sets options to change the behaviour of the battery monitor
// @Bitmask: 0:Ignore DroneCAN SoC, 1:MPPT reports input voltage and current, 2:MPPT Powered off when disarmed, 3:MPPT Powered on when armed, 4:MPPT Powered off at boot, 5:MPPT Powered on at boot, 6:Send resistance compensated voltage to GCS, 7:Allow DroneCAN InfoAux to be from a different CAN node
// @Bitmask: 0:Ignore DroneCAN SoC, 1:MPPT reports input voltage and current, 2:MPPT Powered off when disarmed, 3:MPPT Powered on when armed, 4:MPPT Powered off at boot, 5:MPPT Powered on at boot, 6:Send resistance compensated voltage to GCS, 7:Allow DroneCAN InfoAux to be from a different CAN node, 9:Aggregate monitor reports minimum voltage instead of average.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be better to call out sum and ESC specifically.

libraries/AP_BattMonitor/AP_BattMonitor_Sum.cpp Outdated
Comment on lines 77 to 80
if (voltage_count == 0 || _mon.voltage(i) < voltage_min) {
voltage_min = _mon.voltage(i);
}
voltage_sum += _mon.voltage(i);
Copy link
Member

Choose a reason for hiding this comment

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

Add the fetched voltage as a const float just to prevent getting it three times.

@loki077 @robertlong13
AP_BattMonitor: add option minimum volt option
9f2f5a4 
Allows aggregate monitors like Sum and ESC to report the minimum voltage
instead of the average voltage. This is useful for systems with multiple
isolated batteries where the lowest voltage is the limiting factor.
IamPete1
IamPete1 approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

LGTM

tridge
tridge requested changes Sep 24, 2024
View reviewed changes
libraries/AP_BattMonitor/AP_BattMonitor_Sum.cpp
@@ -103,6 +108,12 @@ AP_BattMonitor_Sum::read()

update_consumed(_state, dt_us);

// Check if we want to report the minimum voltage instead of the average
Copy link
Contributor

Choose a reason for hiding this comment

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

race condition for scripting or DDS, I think we need to create a local float state_voltage and then set that once

@robertlong13 robertlong13 mentioned this pull request Sep 25, 2024
Merged
@robertlong13
Copy link
Collaborator

Closed in favor of #28227

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tridge tridge tridge requested changes

@Hwurzburg Hwurzburg Hwurzburg requested changes

@IamPete1 IamPete1 IamPete1 approved these changes

Assignees
No one assigned
Labels
WikiNeeded needs wiki update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@loki077 @IamPete1 @robertlong13 @Hwurzburg @peterbarker @tridge @CraigElder