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

sys_status.cpp: Do not use battery1 voltage for all batteries. #1704

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

amilcarlucas
Copy link
Contributor

@amilcarlucas amilcarlucas commented Feb 15, 2022

Do not use battery1 voltage as voltage for all other batteries (bugfix).
Support both cell and total voltages above 65V
Support up-to 14S batteries
If available, add cell voltage information to the battery diagnostic

Tested on a drone with 3 batteries, and multiple cells

Copy link
Member

@vooon vooon left a comment

Choose a reason for hiding this comment

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

Please explain unclear operation with cell_voltage vector.
Also processing voltages and voltages_ext might be done in lambda, so you could remove duplicate.

nr_cells++;
}
batt_msg->voltage = total_voltage;
batt_msg->cell_voltage.erase(batt_msg->cell_voltage.begin()+nr_cells, batt_msg->cell_voltage.end()); // erase the unused cells
Copy link
Member

Choose a reason for hiding this comment

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

That's unclear to me. Why do you need nr_cells? Why not to use cell_voltage.size()?
And why do you need to erase()? reserve() do not change size(), only capacity().

Copy link
Contributor Author

@amilcarlucas amilcarlucas Feb 16, 2022

Choose a reason for hiding this comment

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

Why do you need nr_cells? Why not to use cell_voltage.size()?

because if for some reason a cell has more than 65,534V it get coalesced with the next one. Hence the bookkeeping with the extra variable.

And why do you need to erase()?

Using erase allows up to neatly only present the actual number of cells in the ROS message.

reserve do not change size(), only capacity().

Yes, I an aware of that. This method gets a neat result with very low complexity. Erase will not realloc, nor move data around.

processing voltages and voltages_ext might be done in lambda, so you could remove duplicate.

The handling in voltages and voltages_ext is different. The duplication is required. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you are 100% correct about using .size() and removing nr_cells

I changed it now, and corrected other small innacuracy

Copy link
Member

@vooon vooon Feb 16, 2022

Choose a reason for hiding this comment

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

nr_cell always will be equal to size() because it's increase done after push_back().
Lambda could look like that:

int32_t volt_acc = 0;
float total_volt = 0.0f;
auto copy_cell = [&](uint16_t v, bool is_ext) {
  if (v == UINT16_MAX || (is_ext && v == 0))
    return;
    
  if (v == UINT16_MAX-1) {
    volt_acc += v;
    return;
  }
  
  float cv = (volt_acc + v) * 0.001f;
  volt_acc = 0; // clear accumulator after use
  batt_msg->cell_voltage.push_back(cv);
  total_volt += cv;
}

for (auto v : bs.voltages) copy_cell(v, false);
for (auto v : bs.voltages_ext) copy_cell(v, true);

Copy link
Member

Choose a reason for hiding this comment

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

And about ROS message: you don't have to worry about erasing capacity(), only size() matters.

Copy link
Member

Choose a reason for hiding this comment

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

// i'm curious, that chemistry type has so huge cell voltage?

Copy link
Contributor Author

@amilcarlucas amilcarlucas Feb 16, 2022

Choose a reason for hiding this comment

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

yes, you are correct the erase() is not needed at all. Removed it, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about chemistry but if you have a > 16S battery, you will need that mechanism to correctly get the battery voltage. According to the mavlink standard:
https://mavlink.io/en/messages/common.html#BATTERY_STATUS

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, so that a hack to send above 65 total volts. I changed lambda to be able to handle more than one (2**16-2) "cells".
Please note that your code has error in else {acc=0} block because you're clearing accumulator before use.

@amilcarlucas
Copy link
Contributor Author

rebased

@vooon vooon added this to the Version 1.14 milestone Feb 16, 2022
…batteries (bugfix).

Support both cell and total voltages above 65V
Support up-to 14S batteries
If available, add cell voltage information to the battery diagnostic
@amilcarlucas
Copy link
Contributor Author

Thanks for spoting that nasty bug. fixed and squashed all commits.

Copy link
Member

@vooon vooon left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@vooon vooon merged commit 03dfe17 into mavlink:master Feb 16, 2022
@amilcarlucas amilcarlucas deleted the correct-bat-voltages branch February 16, 2022 22:06
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.

None yet

2 participants