rlogiacco / BatterySense

Arduino library to monitor battery consumption
GNU Lesser General Public License v3.0
416 stars 76 forks source link

Several fixes in code mistakes #1

Closed nunonunes closed 8 years ago

nunonunes commented 8 years ago

Hi!

This pull request fixes three problems I found in the code:

  1. minVoltage and maxVoltage where being initialised in the wrong method, so they never got the correct values;
  2. The level()method would almost always overflow and return bogus values, due to an overflow in the calculations;
  3. The voltage() method was off by a factor of 1000.

Cheers!

rlogiacco commented 8 years ago

By fixing the voltage formula I believe you are reducing the precision of the measurement down to 1V, rather than 1mV as it was intended. The whole library operates in mV, as per documentation....

Are you still sure my calculations were off by a factor of 1000?

nunonunes commented 8 years ago

Are you still sure my calculations were off by a factor of 1000?

That is what I observed in using the code, yes.

This doesn't make much sense to me: you are subtracting minVoltage twice: once when calculating voltageRead and a second time when calculating voltagelevel.

You're right of course, I committed the wrong version by mistake.

Then you return an unsigned long which is then fitted into an uint_8 the function return type.

The cast is used to force the whole calculation into a long type. In my tests, using a 9v battery, when you calculate voltageLevel * 100 using these values it overflows and you get a wrong value. This way the calculation is done using larger sized variables and when the function returns it casts it back into a uint_8 type.

nunonunes commented 8 years ago

This is what I meant to commit:

diff --git a/Battery.cpp b/Battery.cpp
index 07acba3..c78a224 100644
--- a/Battery.cpp
+++ b/Battery.cpp
@@ -37,7 +37,7 @@ void Battery::begin(uint16_t refVoltage, float dividerRatio) {
 }

 uint8_t Battery::level() {
-       int16_t voltageRead = this->voltage() - minVoltage;
+       int16_t voltageRead = this->voltage();
        int16_t voltageLevel = voltageRead - minVoltage;
        if (voltageLevel <= 0) {
                return 0;
nunonunes commented 8 years ago

Hi there,

I've committed a fix for my mistake in the code and I have commented on each of the changes I'm proposing in case you want to consider applying any of them.

Cheers!

rlogiacco commented 8 years ago

Thanks for your contribution and sorry for the long wait :+1: