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

Improvements to battery monitoring #1158

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

EMN-CSharp
Copy link
Contributor

This PR improves the following aspects of battery monitoring:

  • The battery device name and the tag are now included in the hardware identifier.
  • Added code and constants to handle availability of battery values.
    This is already tested, and prevents errors such as Battery update negative of minimum value overflows #976 from happening. In that case, the battery returned BATTERY_UNKNOWN_RATE (0x80000000), the same value as int.MinValue.
  • I added support for battery temperature monitoring, though I can't test it at the moment, since I don't have a laptop with a battery temperature sensor.

@inckie
Copy link
Contributor

inckie commented Sep 14, 2023

@EMN-CSharp checked it on my ARM tablet, temperature readings seems to be in 1/1000 Celsius, but it can be quirk of my device. It seems to react to battery cooling (applying cold gel pack, see image). I will test on other laptops I have to see if 1000 multiplier is permanent.

Battery #1:
 Name: N13411
 Manufacturer: SCUD
 Chemistry: Unknown
 Degradation Level: 0.15 %
 Designed Capacity: 41022 mWh
 Fully-Charged Capacity: 40960 mWh
 Remaining Capacity: 36269 mWh
 Charge Level: 88.55 %
 Voltage: 8.862 V
 Temperature: 29756.850 ºC
 Charge Rate: 11.3 W
 Charge Current: 1.276 A
image

@EMN-CSharp
Copy link
Contributor Author

@inckie Thank you for testing it!
As for those strange values, I analysed them and it seems that dividing by 10 instead of multiplying gives better results, although Microsoft's documentation says that the returned value is in 10ths of a Kelvin and therefore it should be multiplied by 10 to convert to Kelvins.
If that 1000 multiplier persists, please confirm it and I'll do the necessary changes. Also, it would be nice to compare the temperature values with other HW monitoring software.

@inckie
Copy link
Contributor

inckie commented Sep 20, 2023

@EMN-CSharp if something is in 10ths, its means resolution of 0.1, you definitely need to divide by 10.0f, not multiply. Then after subtracting it should actually bring temperature range to correct 2 digit range.

The returned temperature value must be divided, not multiplied.
@EMN-CSharp
Copy link
Contributor Author

EMN-CSharp commented Sep 20, 2023

@inckie Oh, thanks for clarifying it. I misunderstood it as if the temperature was divided into tenths. Subtraction by 273.15 is already done.
I just committed the fix. Hope it works fine now.

@inckie
Copy link
Contributor

inckie commented Sep 20, 2023

@EMN-CSharp yes, now it makes more sense, seems legit.

image

@wlkmanist
Copy link
Contributor

wlkmanist commented Mar 5, 2024

Tested with VP1200ELCD UPS and looks better now.
Why not disable sensors that have "null" value? May be disable it by default and enable it once, when there is right value?

Before:
image

After:
image

There is also random symbols with random length in device name (after name), with your PR and with official branch.
Can we fix that? Looks like a string without terminator char.

@EMN-CSharp
Copy link
Contributor Author

Sorry for the late response, I haven't had enough time lately.

Why not disable sensors that have "null" value? May be disable it by default and enable it once, when there is right value?

I just implemented that.

There is also random symbols with random length in device name (after name), with your PR and with official branch.
Can we fix that? Looks like a string without terminator char.

That's interesting. Microsoft's documentation says that battery device name and manufacturer are actually null-terminated strings. In any case, I fixed it (I think) by using the nOutBufferSize parameter of DeviceIoControl to know the size of the string in bytes, instead of relying on a terminator char. Hope it works properly now.

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.

None yet

3 participants