Power.cpp wasn't using the INA sensor if it was available

[akohlsmith]

The firmware can use the INA series of sensors for measuring battery voltage and current if it's detected, but the device metrics weren't using them. What was therefore happening was that while the power telemetry looked good, the device telemetry was just always reporting 101% (plugged in).

This PR attempts to do properly connect the INA sensor back in to the device telemetry, based on some of the existing code in Power.cpp which did not actually get used because the HasBatteryLevel class wasn't getting assigned.

[CLAassistant]

All committers have signed the CLA.

[todd-herbert]

Just another thought, it'd probably be handy too to edit the initial PR comment to describe exactly what was going wrong, and how this PR fixes it (or even better, open an issue and link it). Will make it easier to follow for anyone drifting in here (me tomorrow, oops) with no context.

[todd-herbert]

Just with a look through, no big issues are jumping out at me. I gave it a quick test and it doesn't seem to disrupt ADC readings or anything.

It does assume that users have the INA sensor wired in a specific configuration, yeah?

I don't actually have any of these sensors to test it out so it'd be great for someone else to give it a run and a close look over!

[warrenguy]

The firmware can use the INA series of sensors for measuring battery voltage and current if it's detected, but the device metrics weren't using them. What was therefore happening was that while the power telemetry looked good, the device telemetry was just always reporting 101% (plugged in).

Anecdotally, I'm pretty sure the device metrics appeared to already use the correct (same as power metrics) value for me when using INA219 sensors, but I could be mistaken... I'll check that again more closely, and have a look at your PR here.

I separately have a PR properly enabling the INA3221 for environment and device metrics, and writing to the correct (power vs environment) measurements variant when getMetrics is called.

[akohlsmith]

Just with a look through, no big issues are jumping out at me. I gave it a quick test and it doesn't seem to disrupt ADC readings or anything.

It does assume that users have the INA sensor wired in a specific configuration, yeah?

Yes - it is assuming that the INA is wired in series with the battery positive lead, such that the more positive side of the measurement is facing the device, and the more negative side of the measurement is facing the battery positive lead. This leads to the concept of "negative current" representing the battery discharging and "positive current" representing the battery accepting charge.

[todd-herbert]

Just another thought, it'd probably be handy too to edit the initial PR comment to describe exactly what was going wrong, and how this PR fixes it (or even better, open an issue and link it). Will make it easier to follow for anyone drifting in here (me tomorrow, oops) with no context.

I still think even more detail here might helpful! An exact description of what you see happening right now, and what the PR would help clear up things like this:

Anecdotally, I'm pretty sure the device metrics appeared to already use the correct (same as power metrics) value for me when using INA219 sensors, but I could be mistaken... I'll check that again more closely, and have a look at your PR here.

Now that there are two (potentially overlapping) PRs, I'd love to get a new issue opened (with "walk-me-though-it" level details of the problem). Hopefully we can catch the eye of there of some other devs who have worked with these INA sensors and get a bit more feedback in a general sense; and a consensus on how to go with handling this bug.

[thebentern]

This PR has merge conflicts now based on a previously merged one.

[Mictronics]

I wonder why the code needs to be more complex for that. In actual code, the INA sensor is considered in Power.cpp for battery voltage reading under two conditions:

• BATTER_PIN must be defined
• the INA sensor address must be set in the apps power configuration

The working example is the rp2040-lora variant. This board has no onboard voltage divider for battery sensing, hence an external is needed through one of the ADC pins. However, I have defined the BATTERY_PIN with a dummy value 0xFF to enable all the battery voltage reading code by preprocessor. And I have set the INA address in the power configuration. The results is that the battery voltage&percentage is present in device metrics and voltage&current are in environment metrics. In the existing code with that solution the plugged/unplugged indication is not working, but that fine I guess since the usage of an INA sensor may implies that external battery management is used.

This PR also creates a philosophy problem about the charge/discharge current. e.g. on my INA nodes the positive current indication means a battery discharge, while negative is charging the battery from solar.

IMHO, it looks like I have a solution, let's find the problem.

[akohlsmith]

I wonder why the code needs to be more complex for that. In actual code, the INA sensor is considered in Power.cpp for battery voltage reading under two conditions:

• BATTER_PIN must be defined
• the INA sensor address must be set in the apps power configuration

Why would BATTERY_PIN need to be defined if there is no voltage divider connected to an analog input pin?

The working example is the rp2040-lora variant. This board has no onboard voltage divider for battery sensing, hence an external is needed through one of the ADC pins. However, I have defined the BATTERY_PIN with a dummy value 0xFF to enable all the battery voltage reading code by preprocessor. And I have set the INA address in the power configuration. The results is that the battery voltage&percentage is present in device metrics and voltage&current are in environment metrics. In the existing code with that solution the plugged/unplugged indication is not working, but that fine I guess since the usage of an INA sensor may implies that external battery management is used.

Is this dummy/magic value of 0xff mean something special to the code, or does having an invalid pin (as opposed to no definition or a value of -1 meaning "no pin") do something deeper in the code? It seems it would make a lot more sense to either not define BATTERY_PIN at all (similar to how you don't define the pin for an LED or user button if you don't have one) or to define its value as -1 which means no pin (not sure if it's a 32-bit or 8-bit value, 0xff could very well be interpreted as -1 if it's only an 8 bit value). I'm fine with either method but prefer the former as it is logical. The code is a hodgepodge of these odd little exceptions and it'd be nice to try to nudge it more toward a consistent way to do things.

This PR also creates a philosophy problem about the charge/discharge current. e.g. on my INA nodes the positive current indication means a battery discharge, while negative is charging the battery from solar.

Sure, but short of adding another configuration option (and trying to get it into the phone apps, python utility and web client) it seems like there isn't a good way to define this, and I'd really, really prefer not to have another compile-time option for this. I chose "current flow from battery point of view" as it seemed logical to me, but do understand it can be logical in the other direction for other people with other applications. I'd prefer a config option but it seems that might be useful in a second PR to address it rather than making this one larger.

IMHO, it looks like I have a solution, let's find the problem.

Agreed; once I understand your comment more thoroughly I will try to do the same and see if I can clean this up significantly.

[Mictronics]

I wonder why the code needs to be more complex for that. In actual code, the INA sensor is considered in Power.cpp for battery voltage reading under two conditions:

• BATTER_PIN must be defined
• the INA sensor address must be set in the apps power configuration

Why would BATTERY_PIN need to be defined if there is no voltage divider connected to an analog input pin?

BATTERY_PIN will enable the entire analog battery voltage reading code via preprocessor. Without the power status will always report back 101% and never reads anything.

The working example is the rp2040-lora variant. This board has no onboard voltage divider for battery sensing, hence an external is needed through one of the ADC pins. However, I have defined the BATTERY_PIN with a dummy value 0xFF to enable all the battery voltage reading code by preprocessor. And I have set the INA address in the power configuration. The results is that the battery voltage&percentage is present in device metrics and voltage&current are in environment metrics. In the existing code with that solution the plugged/unplugged indication is not working, but that fine I guess since the usage of an INA sensor may implies that external battery management is used.

Is this dummy/magic value of 0xff mean something special to the code, or does having an invalid pin (as opposed to no definition or a value of -1 meaning "no pin") do something deeper in the code? It seems it would make a lot more sense to either not define BATTERY_PIN at all (similar to how you don't define the pin for an LED or user button if you don't have one) or to define its value as -1 which means no pin (not sure if it's a 32-bit or 8-bit value, 0xff could very well be interpreted as -1 if it's only an 8 bit value). I'm fine with either method but prefer the former as it is logical. The code is a hodgepodge of these odd little exceptions and it'd be nice to try to nudge it more toward a consistent way to do things.

0xFF is like RADIOLIB_NC which is 32bit, but for ordinary GPIO an 8bit value is required. The value itself doesn't matter much, the preprocessor define is needed. See Power.cpp, Power::analogInit(), the entire reading of the battery level is guarded by #ifdef BATTERY_PIN.

#ifdef BATTERY_PIN
.
.
.
    batteryLevel = &analogLevel;
    return true;
#else
    return false;
#endif

If not defined the variable batteryLevel is not initialized, hence the sensor doesn't exist. static HasBatteryLevel *batteryLevel; // Default to NULL for no battery level sensor See rp2040-lora variant for example.

This PR also creates a philosophy problem about the charge/discharge current. e.g. on my INA nodes the positive current indication means a battery discharge, while negative is charging the battery from solar.

Sure, but short of adding another configuration option (and trying to get it into the phone apps, python utility and web client) it seems like there isn't a good way to define this, and I'd really, really prefer not to have another compile-time option for this. I chose "current flow from battery point of view" as it seemed logical to me, but do understand it can be logical in the other direction for other people with other applications. I'd prefer a config option but it seems that might be useful in a second PR to address it rather than making this one larger.

Being an electronics engineer, I learned more than 30 years ago that a current drawn by a load has a positive sign. And I consider the node hardware as being the load on the battery. From my childhood I remember that analog dash ampere meter in cars. A needle swing to the right, positive scale, was discharging the battery, while a left swing, negative scale, was charging. But world has changed since.

IMHO, it looks like I have a solution, let's find the problem.

Agreed; once I understand your comment more thoroughly I will try to do the same and see if I can clean this up significantly.