Library assumes 10-bit ADC

[jeremy-meier]

uint16_t Battery::voltage() {
    if (activationPin != 0xFF) {
        digitalWrite(activationPin, activationMode);
        delayMicroseconds(10); // copes with slow switching activation circuits
    }
    analogRead(sensePin);
    delay(2); // allow the ADC to stabilize
    uint16_t reading = analogRead(sensePin) * dividerRatio * refVoltage / 1024;
    if (activationPin != 0xFF) {
        digitalWrite(activationPin, !activationMode);
    }
    return reading;
}

The voltage() method assumes a 10-bit ADC based on the code above. (Technically, I think that 1024 should be 1023 which is the full scale value for 10 bits: 0b1111111111 = 0x3FF = 0d1023)

However some boards such as MKR1010 support other resolutions - 12 or 16 bits. Seems like it would be pretty straightforward to add an ADC resolution property to the class and add a setter method for it, with a default of 10 bits. Happy to create a pull request if you agree.

[rlogiacco]

Actually 1024 is correct because its the divisor, not a sum/subtraction, so you actually have 1024 items in the scale.

I agree with the idea to have support for multiple boards, but the implementation should switch based on the board, not as a property you have to provide: we need macro directives properly setting the value.

Feel free to submit your proposal, I will be happy to review, suggest and incorporate once ready.

Thanks for contributing!

[digitalcardboard]

Heh, I just went round and round and round until I realized that this might be my issue, I'm trying to implement it on an ESP32, which has 12-bit ADC. I don't have the slightest on how to handle macro directives, but I'd be happy to give it a shot at some point. I'm just adding this for some additional visibility on the issue.

[danmilon]

I encountered this too (on an ESP32) and started implementing the macro approach, but then realized that it's possible for the ADC resolution to be updated by the user, so it's not something that can be predetermined by checking which MCU it's compiling for. Maybe it should be a property instead?

[NuclearPhoenixx]

Is this repo still active? It's a pretty easy fix if you just give the constructor a custom bit resolution.

[rlogiacco]

@NuclearPhoenixx the repo is actively maintained, I'm against using an additional parameter because it's not how libraries work: you determine the ADC resolution based on the board, which is specified via macro directives in the toolchain.

I've just pushed a commit: you are invited to check if that represents a solution by downloading the library from here, I will soon release a new version of the library unless you report some issues.

Thanks for your contributions

[NuclearPhoenixx]

I'm no longer using the library, but thanks for pushing this kind of change! From looking at it, I'm pretty sure it'll work as intended.

I'm against using an additional parameter because it's not how libraries work: you determine the ADC resolution based on the board, which is specified via macro directives in the toolchain.

I'm not sure what to think about this approach though. As a matter of fact, you can change the ADC resolution dynamically in sketches by calling analogReadResolution() at any time so using a fixed value for the ADC resolution doesn't seem like a good thing, even if most users will only call analogReadResolution() once.

It's also a lot less confusing if you specify the resolution in the constructor, instead of telling users to define the flag before including the library, because I guarantee you some people will miss this point and wonder why it's not changing the resolution.

Also, I'm not sure why the changes are in the ESP32 branch?

[rlogiacco]

Thanks for your feedback, really appreciated. On second thought I agree on using an additional optional constructor parameter: looking at the many possible combinations, maintaining the long list of macro variables is going to be quite difficult.

The changes are in the esp32 branch because I elected that as the branch where I was going to implement the support for adc resolutions other than 10... it could be renamed as adc-resolution, but it's going to be closed soon, so.... no need for that.

Out of curiosity, have you found a better library for battery management?

[NuclearPhoenixx]

The changes are in the esp32 branch because I elected that as the branch where I was going to implement the support for adc resolutions other than 10... it could be renamed as adc-resolution, but it's going to be closed soon, so.... no need for that.

Ok sure, makes sense!

Out of curiosity, have you found a better library for battery management?

No actually not. I think yours is by far the easiest one to use. The project I was working on used a 1200 mAh LiPo, but I didn't really get as accurate charge readings as I would have liked so I simply collected the voltage curve over a full battery discharge, fitted that and used that as an estimate. The curve looked pretty wonky too, honestly, so I don't blame your lib for that, blame my weird battery. At the end, I cancelled that project out of some problems with unrelated things.

[rlogiacco]

Thanks again @NuclearPhoenixx.

I've created a pull request #49 and I'm now performing some tests with the boards I have at hand before releasing version 1.2.0: once merged this issue will be closed automatically.