MSzturc / ThinkpadAssistant

An Assistant Application that allows you to use all your Function Keys on a T-Series Thinkpad Laptop
122 stars 13 forks source link

Microphone led state does not persist after sleep #19

Open igorkulman opened 4 years ago

igorkulman commented 4 years ago

Steps reproduce:

MSzturc commented 4 years ago

It's a problem in ACPI. We have to restore LED State after wakeup. We have to patch _WAK and inside you have to restore LED state ( we created an Variable which should be still present after wakeup ). Have a look here for reference : https://github.com/zhtengw/EFI-for-X1C6-hackintosh/blob/e73ddb21e1192990e3e2156f3613b735bf71de75/ACPI/SSDT-SLPWAK-X1C6.dsl

xma commented 4 years ago

I've modified T480 configuration with this idea https://github.com/xma/T480-Clover/commit/7320f0866e5f9c6cd1fbe52204962a0b99daf5d6

It's within a method WKBD, called from _WAK -> EXT4 next to required call for switching led state on thinkpad case idea is to avoid putting more logic on upper level, and keeping everything related to KBD within same dsl file

MSzturc commented 4 years ago

It's within a method WKBD, called from _WAK -> EXT4 next to required call for switching led state on thinkpad case idea is to avoid putting more logic on upper level, and keeping everything related to KBD withing same dsl file

I experiment with a similar solution currently. Im not a fan of mixing two separate functionalities. The current solution with the ssdt & the acpi patch overhelmed many users. Patching 2 ssdt's will make their experience worse ( and it will drive my supporttime up :-) )

A better solution would be to write an kext that synchronizes the states of the led's. It could solve this problem and another bug i noticed: When you mute your sound by reducing volume to 0 the sound muted led will stay off.

My goal (for Thinkpad Assistant 2.0) would be to ship it with a kext that does 3 things:

@xma @EETagent @tylernguyen @Sniki Do you have experience in writing a kext? For me it would be the first time and i could need some help and advise

tylernguyen commented 4 years ago

@MSzturc I do not think there's a need to write one from scratch There's already such a kext. See https://github.com/qwerty12/ThinkPadMuteLEDSetter

We would only need to clean it up and make it compatible with Catalina and ThinkpadAssistant.

junaedahmed commented 4 years ago

Hi, I found a simple solution to this problem without modifying other SSDT. According to ACPI spec _TTS (transition to sleep) is a reserved method which is run once before sleep and once again after wake, but as it is not mandatory in all thinkpad's DSDT this method is not implemented, we can exploit this inconsistency to restore any method calling after wake with only just this snippet:

    Scope (\)
    {

        // This ACPI reserved method is run once before sleep and once after awakened
        Method (_TTS, 1, NotSerialized)
        {
            If (_OSI ("Darwin"))
            {
                // Arg0 contains the system state of transition
                // for wake state it is Zero.
                If (Arg0 == Zero & \_SB.PCI0.LPC.EC.LED1 == One)
                {
                    \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
                }
            }
        }
    }

I tested it it works.... Pls test yourselves and report.

best wishes, Junaed

Sniki commented 4 years ago

Hi, I found a simple solution to this problem without modifying other SSDT. According to ACPI spec _TTS (transition to sleep) is a reserved method which is run once before sleep and once again after wake, but as it is not mandatory in all thinkpad's DSDT this method is not implemented, we can exploit this inconsistency to restore any method calling after wake with only just this snippet:

    Scope (\)
    {

        // This ACPI reserved method is run once before sleep and once after awakened
        Method (_TTS, 1, NotSerialized)
        {
            If (_OSI ("Darwin"))
            {
                // Arg0 contains the system state of transition
                // for wake state it is Zero.
                If (Arg0 == Zero & \_SB.PCI0.LPC.EC.LED1 == One)
                {
                    \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
                }
            }
        }
    }

I tested it it works.... Pls test yourselves and report.

best wishes, Junaed

Thanks for this, we need this for LED Blink fix as well, im learning ACPI slowly and this is what vit9696 suggested me but I wasn’t able to formulate that time and which is the right way. No need to patch _WAK and add extra complications

Sniki commented 4 years ago

@MSzturc @junaedahmed @tylernguyen @tluck Thanks to @junaedahmed post above, with the SSDT providing a fix for saving Mic Mute LED toggle state after wake from sleep, i also included the cleanest possible Power and ThinkPad LED breathing light after wake from sleep. Now we don't need to patch WAK by renaming it to ZWAK and providing all those methods, here is my final SSDT that i think should be used in all the ThinkPads line-up as it's the cleanest one as far as i have researched:

// _TTS Method (TransitionToState) to fix LED issues like:
// Power Button LED and Red LED blinking after Wake from Sleep
// Save Microphone Mute F4 toggle LED State after Wake from Sleep
// Credits: @junaedahmed (Mic Mute LED) @Sniki (Power LED)

DefinitionBlock ("", "SSDT", 1, "ThinkPad", "LED", 0)
{
    External (_SB.PCI0.LPC.EC.HKEY.MMTS, MethodObj)
    External (_SB.PCI0.LPC.EC.LED1, IntObj)
    External (_SI._SST, MethodObj)

    Method (_TTS, 1, NotSerialized)
    {
        If (_OSI ("Darwin"))
        {
            // Arg0 contains the system state of transition
            // for wake state it is Zero.
            If (Arg0 == Zero & \_SB.PCI0.LPC.EC.LED1 == One)
            {
                \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
            }
            If (Arg0 == Zero)
            {
                \_SI._SST(One)
            }
        }
    }
}
junaedahmed commented 4 years ago

Hi @Sniki, Nice work, one correction, External (_SB.PCI0.LPC.EC.LED1, MethodObj) here MethodObj should be IntObj.

Sniki commented 4 years ago

Hi @Sniki, Nice work, one correction, External (_SB.PCI0.LPC.EC.LED1, MethodObj) here MethodObj should be IntObj.

Thanks, will do the correction. Update: Done !

MSzturc commented 4 years ago

@Sniki @junaedahmed What if SB.PCI0.LPC.EC.LED1 is not present? Will this code work or crash?

junaedahmed commented 4 years ago

@MSzturc I was thinking using it on SSDT-KBRD.dsl. If we use on other SSDT like Sniki's one, then we could try conditionally referencing it like this:

Method (_TTS, 1, NotSerialized)
    {
        If (_OSI ("Darwin"))
        {
            // Arg0 contains the system state of transition
            // for wake state it is Zero.
            // Here conditional reference will yield false if LED1 not found and the other functionality 
            will not break.
            If (Arg0 == Zero & CondRefOf (\_SB.PCI0.LPC.EC.LED1 == One))
            {
                \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
            }
            If (Arg0 == Zero)
            {
                \_SI._SST(One)
            }
        }
    }

I think you should add it on KBRD and those who want power led fix too should make their own custom ssdt. it will be simpler to maintain for you. Most hackintosher already use power led fix on _wak method so they don't have to change any thing new.

Sniki commented 4 years ago

@MSzturc I was thinking using it on SSDT-KBRD.dsl. If we use on other SSDT like Sniki's one, then we could try conditionally referencing it like this:

Method (_TTS, 1, NotSerialized)
    {
        If (_OSI ("Darwin"))
        {
            // Arg0 contains the system state of transition
            // for wake state it is Zero.
            // Here conditional reference will yield false if LED1 not found and the other functionality 
            will not break.
            If (Arg0 == Zero & CondRefOf (\_SB.PCI0.LPC.EC.LED1 == One))
            {
                \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
            }
            If (Arg0 == Zero)
            {
                \_SI._SST(One)
            }
        }
    }

Agree, this should be a must as if any ThinkPad model or newer model doesn't have the method at least the LED Blink Fix will work. It may also be a good idea to add an extra comment as note: you may want to adapt the LED1 device according to your ACPI if it is different than LED1

Note: there is an error with your change, please check it on maciASL

junaedahmed commented 4 years ago

@Sniki Sorry posted without testing. Use it like this:

Method (_TTS, 1, NotSerialized)
    {
        If (_OSI ("Darwin"))
        {
            // Arg0 contains the system state of transition
            // for wake state it is Zero.
            If CondRefOf (\_SB.PCI0.LPC.EC.LED1)
            {
                If (Arg0 == Zero & \_SB.PCI0.LPC.EC.LED1 == One)
                {
                    \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
                }
            }

            If (Arg0 == Zero)
            {
                \_SI._SST(One)
            }
        }
    }
Sniki commented 4 years ago

@Sniki Sorry posted without testing. Use it like this:

Method (_TTS, 1, NotSerialized)
    {
        If (_OSI ("Darwin"))
        {
            // Arg0 contains the system state of transition
            // for wake state it is Zero.
            If CondRefOf (\_SB.PCI0.LPC.EC.LED1)
            {
                If (Arg0 == Zero & \_SB.PCI0.LPC.EC.LED1 == One)
                {
                    \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
                }
            }

            If (Arg0 == Zero)
            {
                \_SI._SST(One)
            }
        }
    }

There is still an issue, I believe it should be:

If (CondRefOf (\_SB.PCI0.LPC.EC.LED1))

Check it one more time and correct me if im wrong, as there were still errors.

Thanks !

Update: also ThinkPad exceeds 6 characters for OEM ID, so we want something shorter, either "hack" short word for hackintosh or whatever you guys prefer.

junaedahmed commented 4 years ago

Yes there should be one more pair of parentheses.

Sniki commented 4 years ago

Yes.

Just wanted to share this: https://www.tonymacx86.com/threads/guide-lenovo-thinkpad-t440s-opencore-0-5-9.297423/

The way i did my configuration is that i place the keyboard map on SSDT-KBD and TouchPad configuration there as well

SSDT-LED is this SSDT that does the blink fix and Mic Mute LED fix by @junaedahmed .

As for IRQ fixes like TIMR, RTC, IPIC, in my ACPI they do have the IRQ flags and no need to patch these, only HPET was necessary.

Posted the link if you guys want to cherry pick some cleaner configuration and something that you may miss or have more complicated instead.

Thanks to @MSzturc setup i fixed HPET and completed Keyboard Map.

@MSzturc Final SSDT-LED (or whatever name you prefer, should be):

// _TTS Method (TransitionToState) to fix LED issues like:
// Power Button LED and Red LED blinking after Wake from Sleep
// Save Microphone Mute F4 toggle LED State after Wake from Sleep
// Credits: @junaedahmed (Mic Mute LED) @Sniki (Power LED)

DefinitionBlock ("", "SSDT", 1, "hack", "LED", 0)
{
    External (_SB.PCI0.LPC.EC.HKEY.MMTS, MethodObj)
    External (_SB.PCI0.LPC.EC.LED1, IntObj)
    External (_SI._SST, MethodObj)

    Method (_TTS, 1, NotSerialized)
    {
        If (_OSI ("Darwin"))
        {
            // Arg0 contains the system state of transition
            // for wake state it is Zero.
            If (CondRefOf (\_SB.PCI0.LPC.EC.LED1))
            {
                If (Arg0 == Zero & \_SB.PCI0.LPC.EC.LED1 == One)
                {
                    \_SB.PCI0.LPC.EC.HKEY.MMTS (0x02)
                }
            }

            If (Arg0 == Zero)
            {
                \_SI._SST(One)
            }
        }
    }
}
tylernguyen commented 4 years ago

Thank you @Sniki and @junaedahmed

Your ACPI patch works perfectly. The Mute LED now persists after sleep. However, I think that @MSzturc points and intention still stands:

A kext would be a better and simpler implementation for most people, especially those who are not used to ACPI or not as involved in the hackintosh scenes. Furthermore, it would reduce support time for this project, especially support time related to ACPI patching issues. Furthermore, a kext could enhance this project in various ways:

The last two points are most important for the improvements of this project. In a simpler sense, a kext would be act as a universal patch for everyone, reduce ACPI headaches, and possibility extend ThinkpadAssistant supports to other laptops as well, not just ThinkPads.

junaedahmed commented 4 years ago

@tylernguyen I completely agree with you, kext development will be time consuming but a better solution. In the meantime we could use this acpi method as a go to solution.

Sniki commented 4 years ago

@tylernguyen I completely agree with you, kext development will be time consuming but a better solution. In the meantime we could use this acpi method as a go to solution.

Developing a kext would require a substantial amount of time and effort to be honest, which im also not sure if i can help with this unless project starts. Not much experience with building kexts.

MSzturc commented 4 years ago

https://github.com/MSzturc/Lenovo-T460-OpenCore/commit/dbea0c2292d82a3a99a13a6e1dfb3423103097fc

This bug is fixed for my T460 build with the commit above. I've decided to not separate the keyboard and the led state. I've asked myself what is the domain object im facing on and for me it's the keyboard. The led's (mute mic and mute speaker) are part of this domain so the code to manipulate its state should be inside this domain.

MSzturc commented 4 years ago

Btw. I've started yesterday developing an Kext for Thinkpad Assistant. Currently im able to read the state of LED from ACPI and delegate it to Thinkpad Assistant.

The Problem im facing right now is my bad roundtrip time i have in development process. I create a new version of the kext put it on an usb stick with an OpenCore based build on, boot from this stick and test my new changes. This tooks about 5min every attempt and feels very exhausting.

It's possible to install an kext into the running macOS but since a coding error could cause kernel panic and my T460 is my main machine where i do my daily business on it im forced to have a more restricted workflow that does not infect the stability of my system.

Sniki commented 4 years ago

Btw. I've started yesterday developing an Kext for Thinkpad Assistant. Currently im able to read the state of LED from ACPI and delegate it to Thinkpad Assistant.

The Problem im facing right now is my bad roundtrip time i have in development process. I create a new version of the kext put it on an usb stick with an OpenCore based build on, boot from this stick and test my new changes. This tooks about 5min every attempt and feels very exhausting.

It's possible to install an kext into the running macOS but since a coding error could cause kernel panic and my T460 is my main machine where i do my daily business on it im forced to have a more restricted workflow that does not infect the stability of my system.

Nice, did you get any further progress ?

BTW should there be a way to fix the LED Mic Mute State on Cold Boot/Startup ? Like what is the Arg0 value of a startup/boot/coldboot ? So we can add another line into _TTS method for that ?

What do you guys think @junaedahmed @MSzturc

The only problem right now is that if you reboot or boot and the mic is muted, the mic will be still muted but the LED will be Off, pressing twice will reactivate it.

junaedahmed commented 4 years ago

What do you guys think @junaedahmed @MSzturc

The only problem right now is that if you reboot or boot and the mic is muted, the mic will be still muted but the LED will be Off, pressing twice will reactivate it.

I don't think this can be done without a kext based solution. The problem is LED1 value discarded upon shutdown/restart. To persist the value it should somehow stored in nvram and read after power on.

MSzturc commented 4 years ago

Nice, did you get any further progress ?

It's going on but slower as I thought since my daily jobs gets very time consuming.

kekkokk commented 3 years ago

really interested in the topic. works rally well on osx 11.1 on thinkpad t480s. Also would be nice to reset the led status when a default device change in preference. or also add an option to mute ALL input devices at once and maintain consistency. ex. in system preference i have the default option to airpods when present. but for video meeting I always prefer to use the internal pc microphone. So, if thinkpadassistant would mute my airpods mic, during the video calls on meet it has no effect. Available to help to test or think about these feature. I would describe my ACPI and swift programming skills as completely newbie but if I can help somewhat I'd gladly spend some time. thanks for the awesome work