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

Increase battery voltage resolution to 10mV #2668

Conversation

shellixyz
Copy link
Collaborator

@shellixyz shellixyz commented Jan 21, 2018

Battery voltage resolution of 100mV is not enough for me. This patch increases the voltage resolution to 10mV.

  • Voltage scaling/calibration resolution is increased 10 fold. Previous calibration value should be multiplied by 10. Previous default was 110. New default is 1100.
  • Increased battery voltage min/max/warning thresholds resolution to 10mV
  • OSD can now display battery voltage with 2 decimals. It can be configured with the new setting osd_main_voltage_decimals
  • 100% backwards compatible with current configurator code and existing MSPV1 protocol usages.
  • Adds new MSP2 frame types with increased resolution support: MSP2_ANALOG, MSP2_MISC, MSP2_SET_MISC, MSP2_VOLTAGE_METER_CONFIG, MSP2_SET_VOLTAGE_METER_CONFIG

Matching configurator update: iNavFlight/inav-configurator#328

@giacomo892
Copy link
Collaborator

Nice work! Would be awesome to add battery profiles to this battery code overhaul like the ability of having separate low voltage alarms for LiPo and LiOn batteries with the ability to switch profiles via the OSD.

@shellixyz
Copy link
Collaborator Author

Good idea. You should open a new issue for discussing the best way to do it. I also have one or two other ideas about the battery management.

@@ -588,7 +588,7 @@ void handleSmartPortTelemetry(void)
vfasVoltage = vbat / batteryCellCount;
else
vfasVoltage = vbat;
smartPortSendPackage(id, vfasVoltage * 10); // given in 0.1V, convert to volts
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure the comment is wrong. If vfasVoltage = vbat and vbat resolution is 100mV then vfasVoltage * 10 gives result in 10mV increments not in 1V increments.

@@ -739,7 +739,7 @@ void handleSmartPortTelemetry(void)
//case FSSP_DATAID_A3 :
case FSSP_DATAID_A4 :
if (feature(FEATURE_VBAT))
smartPortSendPackage(id, vbat * 10 / batteryCellCount ); // given in 0.1V, convert to volts
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure the comment is wrong. If vbat resolution is 100mV then vbat * 10 / batteryCellCount gives the individual cell voltage in 10mV increments not in 1V increments.

…100mV resolution because modifying the viewer is not trivial
@shellixyz
Copy link
Collaborator Author

I tested this apart from the telemetry. Looks good to me.

  • The telemetry modifications are pretty straightforward I just divided all the calculations by 10 since the code was expecting a vbat value with 0.1V increments and now getting 0.01V increments (10 times more per unit)
  • The blackbox data will only show 100mV resolution (same as before) because for it to stay compatible with older versions and support 10mV resolution it needs to detect the version but it looks like iNav still declares himself as Cleanflight which would most likely be an issue.

@digitalentity
Copy link
Member

@shellixyz is this ready for merge?

@shellixyz
Copy link
Collaborator Author

@digitalentity Yes it is ready

@digitalentity
Copy link
Member

digitalentity commented Jan 25, 2018

@shellixyz another thought. Maybe use voltage/current readings float and in "proper" volts/amps?
@DzikuVx @fiam @stronnag what do you think?

@shellixyz
Copy link
Collaborator Author

@digitalentity If it is an option for sure would make the code easier to read. Not sure about the practicality though. Can the OSD code manage formatting floats ? And we will still need to convert the floats to integer representation for communication purposes (telemetry, MSP). I will look into it.

@fiam
Copy link
Member

fiam commented Jan 25, 2018

I'm a bit scared of floats for performance reasons. F3 and F4 only have hardware FPU for 32 bits, not for 64 (not sure about F7, I think it does but F3 and F4 is like 99% of the userbase right now). Doing an accidental promotion to double in a calculation is very easy to introduce and difficult to spot later. However, from a functional standpoint and code clarity float would probably be better.

Is there an easy way either via compiler options or via runtime configuration to make sure we don't accidentally introduce operations that require soft-fp? I've taken a quick look to the ARM specific options in GCC but I can't find how to produce an error when the code requires FP via software.

@digitalentity
Copy link
Member

digitalentity commented Jan 25, 2018

@fiam -Wdouble-promotion -Wfloat-equal -Wfloat-conversion -Wunsuffixed-float-constants is what you're looking for

@shellixyz
Copy link
Collaborator Author

@digitalentity Compiling the development branch with these options is already producing a lot of warnings (153).

@shellixyz
Copy link
Collaborator Author

I suggest merging this before considering the float conversion.

@digitalentity
Copy link
Member

@shellixyz agreed on float-point related warnings

@shellixyz shellixyz mentioned this pull request Jan 26, 2018
@digitalentity digitalentity added this to the 1.9 milestone Jan 28, 2018
@shellixyz
Copy link
Collaborator Author

@digitalentity I intend to make another pull-request for improving the battery management even more. Adding mWh draw calculation (better than mAh), improving the way the battery capacity percentage is calculated and also the possibility to use the battery capacity as thresholds for the battery warnings. I am talking about this here because the changes also involve data that should be included in the new MSPV2 messages created by this PR and the mWh calculation also depends on the voltage resolution. So it would be better if this PR and the new one could be merged close in time to each other and that I could base the new PR on the changes made by this one.

@digitalentity
Copy link
Member

@shellixyz sure, no problem. BTW I'm ok with merging this right away.

@shellixyz
Copy link
Collaborator Author

shellixyz commented Jan 29, 2018

Nice. I have some polishing to do but I will be working on the new PR tomorrow and should be ready soon :)

@shellixyz shellixyz deleted the increase_battery_voltage_resolution branch February 3, 2018 17:57
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.

4 participants