acidanthera / bugtracker

Acidanthera Bugtracker
385 stars 45 forks source link

Dual Battery Support in SMCBatteryManager #661

Closed Sniki closed 4 years ago

Sniki commented 4 years ago

I wanted to raise some attention toward this issue, as there are plenty of laptops that have Dual Batteries and many people are affected by this.

MacOS has buggy or better say no dual battery support, the current state is something like this: Percentage will remain at 100% until external battery is drained completely, percentage will start to go down when external is drained and the internal starts to be used. Anyway.

The current working fix we have is what we had with Rehabman on ACPIBatteryManager.kext, that is the SSDT-BATC.dsl that Rehabman created to combine both batteries into one. Besides the use of that SSDT-BATC we also need to rename Battery notifiers of BAT0 and BAT1 to BATC so they are read as one. Examples of these renames are commented on the SSDT-BATC.dsl for reference.

This currently works fine with VirtualSMC + SMCBatteryManager but is not clean, as it requires a lot of renames. Im not a coding expert so I will need some assistance or someone that posses the knowledge to solve this.

What I think we need if it is possible, is to add support for Dual Batteries on SMCBatteryManager.kext so it can see & poll both batteries as one by either a boot-arg /device property/Quirk

Sincerely, Sniki

vit9696 commented 4 years ago

Bug description actually makes no sense to me. What it should actually request is letting SMCBatteryManager see & poll 2 batteries as if it was 1. Reopening with low priority, as ACPI edits are needed anyway to split the registers.

Sniki commented 4 years ago

Bug description actually makes no sense to me. What it should actually request is letting SMCBatteryManager see & poll 2 batteries as if it was 1. Reopening with low priority, as ACPI edits are needed anyway to split the registers.

Thanks @vit9696 Fixed the bug description.

athlonreg commented 4 years ago

I have a dual battery pc, the model is thinkpad t580. And the problem of notify renames has been resolved. You can handle the method involved with notify, example use hotpatch. That is to say you can use if osi darwin code to rewrite these method to let them notify batc, for other os use origin code, then used with some method rename. The t580 is successful. You can see in my repo. https://github.com/athlonreg/ThinkPad-T580-i7-8550u. The SSDT to handle notify is SSDT-NTFY, https://github.com/athlonreg/ThinkPad-T580-i7-8550u/blob/master/OC/OC/ACPI/DSL/SSDT-NTFY.dsl

Sincerely, athlonreg

Sniki commented 4 years ago

I have a dual battery pc, the model is thinkpad t580. And the problem of notify renames has been resolved. You can handle the method involved with notify, example use hotpatch. That is to say you can use if osi darwin code to rewrite these method to let them notify batc, for other os use origin code, then used with some method rename. The t580 is successful. You can see in my repo. https://github.com/athlonreg/ThinkPad-T580-i7-8550u. The SSDT to handle notify is SSDT-NTFY, https://github.com/athlonreg/ThinkPad-T580-i7-8550u/blob/master/OC/OC/ACPI/DSL/SSDT-NTFY.dsl

Sincerely, athlonreg

I already have/im using that method for about two months, i forgot to update the bug description as i found the If _OSI ("Darwin") examples just like 3-4 days after, but the point here is to make this a feature of SMCBatteryManager.kext so it can see and poll 2 batteries as one, like @vit9696 mentioned. Currently a lot of renames are required to have that working correctly which is not clean and too complicated. Instead it would be better to have that function by a boot-arg/DeviceProperty/Quirk.

Andrey1970AppleLife commented 4 years ago

https://github.com/acidanthera/VirtualSMC/commit/e1506877f584b941456723ece3303ff907a9573a

Sniki commented 4 years ago

@Andrey1970AppleLife I already have been using this for many years, in fact im using a slightly modified version, there should be a If (_OSI (“Darwin”)) in _STA & _INI or just one of them (added to both of them) in my case, So that way this loads only under macOS and not affect other operating systems.

This also requires to patch all the methods that have BAT0 and BAT1 notifiers to have BATC instead of BAT0 and BAT1

So it is a complex solution, if we stick with this, we will need to do a Dual Battery Support.md with a description on how to properly implement this workaround without affecting other operating systems.

@vit9696 idea was a better solution so SMCBatteryManager gets a feature to see & poll two batteries as one.

vit9696 commented 4 years ago

@Sniki I am afraid implementing this in SMCBatteryManager is not feasible. It will require quite some time, and I am afraid we will not implement this unless somebody submits a solution.

Andrey1970AppleLife commented 4 years ago

@Andrey1970AppleLife I already have been using this for many years, in fact im using a slightly modified version, there should be a If (_OSI (“Darwin”)) in _STA & _INI or just one of them (added to both of them) in my case, So that way this loads only under macOS and not affect other operating systems.

Please send PR with fix If (_OSI (“Darwin”))

Sniki commented 4 years ago

@vit9696 that is all fine, this is already a solution that we use, that was just something that would've made the end-user work easier.

I will send a pull request as @Andrey1970AppleLife asked for https://github.com/acidanthera/VirtualSMC/commit/e1506877f584b941456723ece3303ff907a9573a with some changes and possibly create a short tutorial on how to properly implement it for a "Darwin" only style.