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

SMCBatteryManager show 0 cycle count project:vsmc #892

Closed
kecinzer opened this issue May 5, 2020 · 30 comments
Closed

SMCBatteryManager show 0 cycle count project:vsmc #892

kecinzer opened this issue May 5, 2020 · 30 comments
Labels
duplicate This issue or pull request already exists invalid This doesn't seem right project:vsmc

Comments

@kecinzer
Copy link

kecinzer commented May 5, 2020

I'm so sorry for opening another issue, but I have still this issue (#879.).
I removed my combined battery hack, so now I have correct ACPI patch, but result is still same.

smc-ioreg

Should be Kext attached to battery like with ACPIBatteryManager?

@kecinzer kecinzer changed the title SMCBatteryManager show 0 cycle count SMCBatteryManager show 0 cycle count project:vsmc May 5, 2020
@Andrey1970AppleLife
Copy link
Contributor

#879

@Andrey1970AppleLife Andrey1970AppleLife added duplicate This issue or pull request already exists project:vsmc labels May 5, 2020
@kecinzer
Copy link
Author

kecinzer commented May 5, 2020

Issue #879 was marked as invalid and closed for write. I corrected my ACPI patch as suggested. That mean this issue can't be fixed?

@vit9696
Copy link
Contributor

vit9696 commented May 5, 2020

@kecinzer it is basically unjustified that your patch is correct and we are not ready to investigate every not working configuration. I am afraid a more technical report will be needed for us to look at this issue.

@kecinzer
Copy link
Author

kecinzer commented May 5, 2020

OK, I understand. More technical report you mean EFI folder with OpenCore and Lilu DEBUG log? I'm so sorry to being so annoying, I'm just trying to understand what is wrong and fix it.

@vit9696
Copy link
Contributor

vit9696 commented May 5, 2020

More technical is the explanation of what values comes from ACPI, why are they correct, and what SMCBatteryManager does wrong. So far we received many similar reports like yours, and they always were due to incorrect ACPI.

CC @usr-sse2

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 5, 2020

It shows both design capacity and current capacity 0x12c6 = 4806 mA•h, so the battery has 100% capacity and so we report 0 cycles. How old is your battery?
I see, it's 1 year from manufacture => less than year in use. Wait a few years while using the laptop, and it will show non-zero.

@usr-sse2 usr-sse2 added the invalid This doesn't seem right label May 5, 2020
@kecinzer
Copy link
Author

kecinzer commented May 5, 2020

It shows both design capacity and current capacity 0x12c6 = 4806 mA•h, so the battery has 100% capacity and so we report 0 cycles. How old is your battery?
I see, it's 1 year from manufacture => less than year in use. Wait a few years while using the laptop, and it will show non-zero.

In fact I don’t use it so much, but I tought that cycle is one discharge from 100 to 0% or 2 discharges from 100 to 50%.
But ACPIBatteryManager show 8 cycles. And its attached to BAT0 in IORegistry. I tought that SMCBatteryManager has to be attached too. If its working correctly this way, then allright :)).

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 5, 2020

@kecinzer Does your BAT0 device have _BIX method?

Sent with GitHawk

@kecinzer
Copy link
Author

kecinzer commented May 5, 2020

@kecinzer Does your BAT0 device have _BIX method?

Sent with GitHawk

No, only _STA, _BIF, _BST and _PCL.

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 5, 2020

@kecinzer Does your BAT0 device have _BIX method? ...

@kecinzer Then your battery doesn’t report cycle count, so the driver approximates it from loss of capacity, like I said before. Does ACPIBatteryManager report different max and current capacity? We took the formula for cycle count from ACPIBatteryManager, so it should get the same value. Besides, which ACPIBatteryManager are you comparing with?

Sent with GitHawk

@kecinzer
Copy link
Author

kecinzer commented May 5, 2020

@kecinzer Does your BAT0 device have _BIX method? ...

@kecinzer Then your battery doesn’t report cycle count, so the driver approximates it from loss of capacity, like I said before. Does ACPIBatteryManager report different max and current capacity? We took the formula for cycle count from ACPIBatteryManager, so it should get the same value. Besides, which ACPIBatteryManager are you comparing with?

Sent with GitHawk

No, ACPIBatteryManager report same max and current capacity as "0x12c6" .
So I don't understand why ACPIBatteryManager reports different cycle count. I took it from there - https://bitbucket.org/RehabMan/os-x-acpi-battery-driver/downloads/RehabMan-Battery-2018-1005.zip

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 6, 2020

We based on Slice's ACPIBatteryManager. RehabMan's one may have different formula.

@kecinzer
Copy link
Author

kecinzer commented May 6, 2020

We based on Slice's ACPIBatteryManager. RehabMan's one may have different formula.

Ah, great. So it looks like SMCBatteryManager works perfectly, just have different count methods. Thank you a lot for this explanation!

What about consider to change formula to RehabMan's one? For me it's more accurate. I have almost new battery, but some usage there was and when I compare to my MacBook Pro, where I replaced battery about 1 year ago (now it has 10 cycle count), counts increases very similarly...

According this - https://github.com/RehabMan/OS-X-ACPI-Battery-Driver/blob/master/AppleSmartBatteryManager/AppleSmartBattery.cpp#L1291
it expanding _BIF to include cycle count.

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 6, 2020

• About expanding _BIF to include cycle count: this line of code only gets cycle count if _BIF size is larger than defined by ACPI specification, e. g. you wrote some code in DSDT to calculate cycle count and provide it as additional field. I doubt that laptop manufacturers do such hacks, but I can check it if you upload your DSDT.
• The formula used by RehabMan: (DesignCapacity - LastFullChargeCapacity) / 6.
• The formula used by SMCBatteryManager: (DesignCapacity - LastFullChargeCapacity) * 1000 / DesignCapacity. It is based on the assumption that after 1000 cycles the capacity of the battery will become 0 (1000 cycles is what MacBook batteries are designed for). In practice, they will still have some capacity.
• Both formulas are linear, as DesignCapacity is constant, but with different coefficients. In your case, our formula divides by 4.8.
• According to your data, you have LastFullChargeCapacity=DesignCapacity, so both formulas give 0.

Please upload the DSDT and something that shows capacity (IOReg, iStat, System Profiler) with RehabMan's ACPIBatteryManager.

@kecinzer
Copy link
Author

kecinzer commented May 6, 2020

@usr-sse2 thank you so much for this investigation with me.

Here are all needed stuff.
DSDT origin from clover
DSDT-origin-from-clover.zip
DSDT from MaciASL
System-DSDT-MaciASL.zip

Screenshots from IORegistry, System info and iStat Menus
ioreg
syteminfo
istatmenus

And also screenshots without AC:
systeminfo-battery
istatmenus-battery
istatmenus2-battery

@armenio
Copy link

armenio commented May 6, 2020

RehabMan´s method implements a possible ACPI custom cycle count parameter.

Is a good idea to take a look here: https://github.com/RehabMan/OS-X-ACPI-Battery-Driver/blob/master/SSDT-ACPIBATT.dsl#L12

@kecinzer
Copy link
Author

kecinzer commented May 6, 2020

RehabMan´s method implements a possible ACPI custom cycle count parameter.

Is a good idea to take a look here: https://github.com/RehabMan/OS-X-ACPI-Battery-Driver/blob/master/SSDT-ACPIBATT.dsl#L12

Yes, its only formula constant like @usr-sse2 already mentioned earlier.

@armenio
Copy link

armenio commented May 6, 2020

RehabMan´s method implements a possible ACPI custom cycle count parameter.
Is a good idea to take a look here: https://github.com/RehabMan/OS-X-ACPI-Battery-Driver/blob/master/SSDT-ACPIBATT.dsl#L12

Yes, its only formula constant like @usr-sse2 already mentioned earlier.

as you refer to constant, you did not understand that this is precisely so that the formula is not based on a "constant" but on a parameterized variable

@kecinzer
Copy link
Author

kecinzer commented May 7, 2020

RehabMan´s method implements a possible ACPI custom cycle count parameter.
Is a good idea to take a look here: https://github.com/RehabMan/OS-X-ACPI-Battery-Driver/blob/master/SSDT-ACPIBATT.dsl#L12

Yes, its only formula constant like @usr-sse2 already mentioned earlier.

as you refer to constant, you did not understand that this is precisely so that the formula is not based on a "constant" but on a parameterized variable

In my case, is no matter what this variable is, 0 / variable is always zero, but its 8 for me.

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 7, 2020

@kecinzer From your DSDT I see that BTIF method is implemented in SSDT. Please upload all (patched) tables from running system with MaciASL's File->Export Tableset.

@kecinzer
Copy link
Author

kecinzer commented May 7, 2020

@kecinzer From your DSDT I see that BTIF method is implemented in SSDT. Please upload all (patched) tables from running system with MaciASL's File->Export Tableset.

Ok, here.
keciEB.acpi.zip

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 7, 2020

So, your battery reports cycle count (in BCC field, which is split as BCC0 and BCC1), but doesn't implement _BIX method and instead does it in GBTI method which seems to be proprietary. Your battery patch in one of SSDTs adds this field to _BIF (this is that zprood's technique). It violates the ACPI specification, and we don't want to support it. Instead, an ACPI 4.0 _BIX method should be implemented in your SSDT that would report the same info but in different structure, like here: https://github.com/gsly/OS-X-ACPI-Battery-Driver/wiki/_BIX-Method
I'll try to rewrite your SSDT tomorrow.

@kecinzer
Copy link
Author

kecinzer commented May 8, 2020

So, your battery reports cycle count (in BCC field, which is split as BCC0 and BCC1), but doesn't implement _BIX method and instead does it in GBTI method which seems to be proprietary. Your battery patch in one of SSDTs adds this field to _BIF (this is that zprood's technique). It violates the ACPI specification, and we don't want to support it. Instead, an ACPI 4.0 _BIX method should be implemented in your SSDT that would report the same info but in different structure, like here: https://github.com/gsly/OS-X-ACPI-Battery-Driver/wiki/_BIX-Method
I'll try to rewrite your SSDT tomorrow.

Thank you a lot for you findings.
I start to split my SSDT file, so here is my BATT file. I already looked at this and found GBTI, but I don't know how to convert it into _BIX.

SSDT-08-BATT.dsl.zip

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 8, 2020

Try these two (separately, of course).
In SSDT-08-BATT-BIX I added _BIX method implementation.
In SSDT-08-BATT-BIX-BDC I also corrected a supposed mistake in the original implementation – it reports design capacity = last full charge capacity, I made it to report design capacity (so it may become greater than 4806). Of course, it's only a guess by variable names. (BFC vs BDC).
SSDT-08-BATT-BIX.zip

@kecinzer
Copy link
Author

kecinzer commented May 8, 2020

Try these two (separately, of course).
In SSDT-08-BATT-BIX I added _BIX method implementation.
In SSDT-08-BATT-BIX-BDC I also corrected a supposed mistake in the original implementation – it reports design capacity = last full charge capacity, I made it to report design capacity (so it may become greater than 4806). Of course, it's only a guess by variable names. (BFC vs BDC).
SSDT-08-BATT-BIX.zip

Thanks a lot. I tried SSDT-08-BATT-BIX and cycle count is now 8. So it works great! Full capacity is now different - it shows full capacity as current capacity, as you said.
I want to try SSDT-08-BATT-BIX-BDC, but when I compare files, there are no differences.

@usr-sse2
Copy link
Contributor

usr-sse2 commented May 8, 2020

Try these two (separately, of course). ...

@kecinzer What’s now in ioreg?
I accidentally uploaded two identical files, they they are both -BIX-BDC.

Sent with GitHawk

@kecinzer
Copy link
Author

kecinzer commented May 9, 2020

Try these two (separately, of course). ...

@kecinzer What’s now in ioreg?
I accidentally uploaded two identical files, they they are both -BIX-BDC.

Sent with GitHawk

Ah, ok :). In IOREG is now bigger design capacity and iStatMenus shows 99% health of battery.

ioreg

With ACPIBatteryManager there was DesignCapacity=MaxCapacity=CurrentCapacity = 0x12c6

kecinzer added a commit to kecinzer/hpelitebook850g5-opencore that referenced this issue May 9, 2020
Thank you so much @usr-sse2 for his work on this issue acidanthera/bugtracker#892.
He prepares new SSDT file that fully working with SMCBatteryManager with cycle count and correct
display of full designed capacity.

Signed-off-by: kecinzer <1541555+kecinzer@users.noreply.github.com>
kecinzer added a commit to kecinzer/hpelitebook850g5-opencore that referenced this issue May 9, 2020
Thank you so much @usr-sse2 for his work on this issue acidanthera/bugtracker#892.
He prepares new SSDT file that fully working with SMCBatteryManager with cycle count and correct
display of full designed capacity.

Signed-off-by: kecinzer <1541555+kecinzer@users.noreply.github.com>
@Mateo1234454545
Copy link

@usr-sse2 , can you help me with battery also?
my battery show cycles count but it is unreliable.
Sometimes is 309 and sometimes 208.
And cycle count doesn't progress after a full charge.
Also , as you said battery health is calculated by the capacity and not from cycles.
Here are the files.
Thank you in advance.
CATALINA.acpi.zip
CATALINA.ioreg.zip

@usr-sse2
Copy link
Contributor

@Mateo1234454545, kecinzer's laptop exposes cycle count through EC register but didn't implement _BIX, so we were able to do it (the register was guessed by name). Look at your DSDT:

Method (_BIF, 0, NotSerialized)  // _BIF: Battery Information
{
    Name (BIF0, Package (0x0D){})
    ECG9 (One, BIF0)
    Return (BIF0) /* \_SB_.BAT0._BIF.BIF0 */
}
Method (ECG9, 2, NotSerialized)
{
    Acquire (ECM1, 0xFFFF)
    ECWB (0x03, Arg0)
    Arg1 [Zero] = One
    Local0 = ECRW (0x20)
    Arg1 [One] = Local0
    Local1 = ECRW (0x1E)
    Arg1 [0x02] = Local1
    Arg1 [0x03] = One
    Local2 = ECRW (0x22)
    Arg1 [0x04] = Local2
    Divide (Local0, 0x0A, Local5, Local3)
    Arg1 [0x05] = Local3
    Divide (Local0, 0x21, Local5, Local3)
    Arg1 [0x06] = Local3
    Divide (Local0, 0x64, Local5, Local3)
    Arg1 [0x07] = Local3
    Arg1 [0x08] = Local3
    Local3 = ECU0 (BS01, Zero)
    Arg1 [0x09] = Local3
    Local3 = ECRW (0x26)
    Local3 = XPTS (Local3)
    Arg1 [0x0A] = Local3
    Local3 = ECRB (0x29)
    Local3 = ECU0 (BS03, Local3)
    Arg1 [0x0B] = Local3
    Local3 = ECRB (0x28)
    Local3 = ECU0 (BS02, Local3)
    Arg1 [0x0C] = Local3
    Release (ECM1)
}

It's obfuscated by getting registers by numeric offsets using ECRW method. There's nothing that we can do here. The approximation that we have is best that we can if the hardware doesn't count cycles or doesn't expose the counter.

@kecinzer
Copy link
Author

kecinzer commented Nov 9, 2020

NEVERMIND, I found solution by myself :).

Dear @usr-sse2 HP released new BIOS with changes into battery. They added BTIX method. Now your modified SSDT hotfix didn't work - battery can't be seen. I tried to fix it by additional ACPI renames for BTIX method and some edit of SSDT. Now I can see battery, but it shows only 1% charged. I can also see cycle count etc. But current battery level isn't reported.

Here is my new DSDT (without renames) and my new SSDT-BATT.aml try.

Can you please help me with this?
dsdt-ssdt-batt.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists invalid This doesn't seem right project:vsmc
Development

No branches or pull requests

6 participants