Implement battery table for Windows
#8267
Conversation
| // Once we find one battery, no need to do anything | ||
| // else |
There was a problem hiding this comment.
Shouldn't we return all the batteries? There are some laptops with 2 batteries. Also, for some laptops you can get an external battery, like https://www.amazon.com/593553-001-593554-001-Extended-Pavilion-CQ42/dp/B07SGHM9RR
There was a problem hiding this comment.
Yeah, good point. I've never encountered such a setup (and don't have one to test on) but it's pretty easy to allow this code to support it.
There was a problem hiding this comment.
Updated to support this but I'm not able to test it. Code still works for 0/1 batteries.
| if (!SetupDiEnumDeviceInterfaces( | ||
| hdev, 0, &GUID_DEVCLASS_BATTERY, idev, &did)) { | ||
| LOG(ERROR) << "Failed to set up enumeration for batteries: " | ||
| << GetLastError(); |
There was a problem hiding this comment.
What happens on a Windows desktop without a battery? Also, what happens in a Windows VM?
There was a problem hiding this comment.
Did this testing as well and updated the top level description
Testing also performed on a macOS laptop with Windows 10/11 in VMs - the table successfully reports the VMWare virtual battery when that is configured and no results (along with appropriate logging) when there is no virtual battery.
zwass
requested review from
getvictor and
directionless
and removed request for
getvictor
|
@directionless I updated the PR to support multiple batteries as suggested by @getvictor and also performed additional testing. @getvictor yes there are integration tests for Windows. I turned on the test for this table in the CMakeLists.txt. There is no diff for the .cpp file. |
| if (!SetupDiEnumDeviceInterfaces( | ||
| hdev, 0, &GUID_DEVCLASS_BATTERY, idev, &did)) { | ||
| if (GetLastError() != ERROR_NO_MORE_ITEMS) { |
There was a problem hiding this comment.
I'm not quite sure what this does. Does it cause an early return if the OS says we're done with batteries, or does it always try 100 times? (Because if it's the latter, I'm wondering if there's a speed/overhead impact)
There was a problem hiding this comment.
It does an early return no matter what the error is, but only logs if the error is something besides the OS saying there are no more batteries to enumerate (which is an expected case).
| // Assume that 12 volts is the intended voltage for the | ||
| // battery in order to convert from the mWh units that | ||
| // Microsoft provides to match the mAh units that the battery | ||
| // table already uses for macOS. | ||
| const int designedVoltage = 12; |
There was a problem hiding this comment.
How confident are you here? And why?
There was a problem hiding this comment.
I was not able to find any API that provides the designed voltage, so I'm not sure what to do here to make the values on Windows match the values on macOS besides trying this assumption. I also only have a single Windows device for testing, so my inclination is to see how these values do in the real world with bigger Windows deployments.
Almost all of the columns are able to line up with those already available on macOS.
This has been tested on a single Windows 11 Pro laptop. All columns look correct except for
cycle_countin which this device reports0. We seem to be using the correct API though and will hopefully get values on other devices.Testing also performed on a macOS laptop with Windows 10/11 in VMs - the table successfully reports the VMWare virtual battery when that is configured and no results (along with appropriate logging) when there is no virtual battery.