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

Microphone led state does not persist after sleep #19

Open
igorkulman opened this issue May 28, 2020 · 25 comments
Open

Microphone led state does not persist after sleep #19

igorkulman opened this issue May 28, 2020 · 25 comments

Comments

@igorkulman
Copy link
Contributor

Steps reproduce:

  • press fn+f4 to disable the microphone, the led on f4 turns on
  • put the Thinkpad to sleep
  • wake up the Thinkpad
  • observe the led on f4 being off although the microphone is still muted
@MSzturc
Copy link
Owner

MSzturc commented May 28, 2020

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
Copy link

xma commented May 29, 2020

I've modified T480 configuration with this idea
xma/T480-Clover@7320f08

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
Copy link
Owner

MSzturc commented May 29, 2020

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:

  • Patches the SSDT's dynamically on load. This would fix pitfalls like LPC & LPCB mismatch
  • Synchronize the state of LEDs ( to get rid of the 2/3 button bindings thats only need because Thinkpad Assistant doesn't know what the current LED state is ).
  • Introduce a Communication Layer between Thinkpad Assistant (which still will do the rendering of the HUD) and Assistant Kext. With that we get rid of the Keybinding problematic (almost all F-Key are bound right now and using an Apple Bluetooth Keyboard that has F13-F19 on it causes the problem that this Buttons are not usable for us since the are already used by Thinkpad Assistant) With that we could have as many binding as we want

@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
Copy link

@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
Copy link

junaedahmed commented May 30, 2020

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
Copy link

Sniki commented May 30, 2020

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
Copy link

Sniki commented May 30, 2020

@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
Copy link

junaedahmed commented May 30, 2020

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

@Sniki
Copy link

Sniki commented May 30, 2020

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
Copy link
Owner

MSzturc commented Jun 1, 2020

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

@junaedahmed
Copy link

junaedahmed commented Jun 1, 2020

@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
Copy link

Sniki commented Jun 1, 2020

@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
Copy link

@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
Copy link

Sniki commented Jun 1, 2020

@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
Copy link

Yes there should be one more pair of parentheses.

@Sniki
Copy link

Sniki commented Jun 1, 2020

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
Copy link

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:

  • Persist Mute LED after a reboot.
  • Patches the SSDT's dynamically on load. This would fix pitfalls like LPC & LPCB mismatch @MSzturc
  • Synchronize the state of LEDs ( to get rid of the 2/3 button bindings thats only need because Thinkpad Assistant doesn't know what the current LED state is ). @MSzturc
  • Introduce a Communication Layer between Thinkpad Assistant (which still will do the rendering of the HUD) and Assistant Kext. With that we get rid of the Keybinding problematic (almost all F-Key are bound right now and using an Apple Bluetooth Keyboard that has F13-F19 on it causes the problem that this Buttons are not usable for us since the are already used by Thinkpad Assistant) With that we could have as many binding as we want @MSzturc

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
Copy link

@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
Copy link

Sniki commented Jun 2, 2020

@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
Copy link
Owner

MSzturc commented Jun 2, 2020

MSzturc/Lenovo-T460-OpenCore@dbea0c2

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
Copy link
Owner

MSzturc commented Jun 2, 2020

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
Copy link

Sniki commented Jun 10, 2020

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
Copy link

junaedahmed commented Jun 11, 2020

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
Copy link
Owner

MSzturc commented Jun 11, 2020

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
Copy link

kekkokk commented Jan 27, 2021

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

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

No branches or pull requests

7 participants