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

[Macbook Pro 6,2] Power mode not updating when switching from AC to BAT. #573

Closed
mightyllama89 opened this issue Aug 20, 2021 · 8 comments
Closed

Comments

@mightyllama89
Copy link
Contributor

[x] I've read and accepted the Bug Reporting Howto
[x] I've attached all required tlp-stat outputs via Gist (see below)

Describe the bug
When tlp is already running and in AC mode, if you unplug from power the power mode remains unchanged while the power source is updated and accurate.
Mode = AC Power source = battery

I have found the bug. It resides in tlp-func-base, line 789.
is_uint "$wait" 2 || wait=8
The timeout is too short. Changing it to 10 fixed it for me.

Expected behavior
Power_mode should switch to "battery"
A clear and concise description of what you expected to happen.

To Reproduce

Find an ancient 2010 Macbook Pro. Plug it in to AC. Then unplug it.

Additional context
tlp-stat output:
https://gist.github.com/mightyllama89/6b25293c4327fdb4245d36abe67ee0e8

@linrunner
Copy link
Owner

Hi,

i've noticed your pull request too. Nevertheless i need to check some basic assumptions because there is already a workaround for MacBooks in the code, see tlp-func-base.in#L736.

Quirk: MBP 5.3 reports AC offline when started with charger connected

I would like to understand if this is still the case. Because if the AC status were correct, we would not need the delay loop at all.

So with TLP 1.4.0-beta.1 installed: please power off your MacBook, make sure AC is connected, power on, boot and show:

sudo tlp-stat --psup

@linrunner linrunner added this to the 1.4 Release milestone Aug 21, 2021
@linrunner linrunner self-assigned this Aug 21, 2021
@mightyllama89
Copy link
Contributor Author

mightyllama89 commented Aug 22, 2021

The MBP quirk doesn't look necessary on mine. AC status is reported correctly as 'Online' when booting with the power supply plugged in. From what I can tell the quirk is unrelated to the need for the wait time. The wait time is needed anytime the battery status doesn't update immediately regardless of manufacturer. I have attached the information you requested in addition to info on arriving at the fix I suggested.

https://gist.github.com/mightyllama89/31e790ac2374e2125fa59b8814fea508

@linrunner
Copy link
Owner

From what I can tell the quirk is unrelated to the need for the wait time.

Yes, we do: if the AC status is wrong we can't use it to determine the actual power source. Instead we need a reliable BAT status, therefore the wait time is necessary.

When your MacBook doesn't have the quirk, you may configure

TLP_PS_IGNORE="BAT"

and use the AC status directly, without waiting time.

Unfortunately, one MacBook reporting the status correctly is not enough evidence to remove the quirk completely.

@linrunner linrunner changed the title [Macbook Pro] Power mode not updating when switching from AC to BAT. [Macbook Pro 6,2] Power mode not updating when switching from AC to BAT. Aug 22, 2021
@mightyllama89
Copy link
Contributor Author

I am sorry if there is confusion.

Unfortunately, one MacBook reporting the status correctly is not enough evidence to remove the quirk completely.

I was not recommending that the quirk be removed.

Because if the AC status were correct, we would not need the delay loop at all.

You asked me to test if the quirk was still necessary. I only mentioned the quirk to clarify that the workaround was not necessary in this case. However the quirk is what makes it check the battery status. If you keep the quirk then the wait time should be changed.

I went through the commits and issues and see that I am not the first one to have this kind of problem.
Issue #443
Commit 338068e

Commit c0c02d8 probably broke the fixes for those two. It looks like this quirk applies to more than just the MBP. The way you currently have it the program checks for AC status immediately upon the udev change. You are assuming that the AC status will update immediately and that any status not online must mean battery. Only AC type ADP1 gets to run any of the battery checks.

In conclusion, it takes longer than 0.8 seconds for the battery status to update. Increasing the wait time to 15 fixes it for me and probably others too. I spent a lot of time figuring out my issue and thought I'd try to contribute what I found out.

@linrunner
Copy link
Owner

linrunner commented Aug 22, 2021

If you keep the quirk then the wait time should be changed.

Right.

You are assuming that the AC status will update immediately and that any status not online must mean battery.

In fact this assumption holds for almost all laptops. For some of the remaining, the quirk solves the problem without users having to configure TLP_PS_IGNORE="AC".

But if the battery status is used to determine the power source, the wait time is needed because the battery status won't change immediately. This is also necessary for TLP_PS_IGNORE="AC".

The changes you list were a consequence of the original quirk, which applied to all ac devices, not just ADP1. Your problem is also a consequence of the quirk. I'm taking this opportunity to rethink the sense and necessity of the quirk as a whole. So i'm grateful for your input.

Because the wait is needed in case of TLP_PS_IGNORE="AC", your pull request makes sense anyway.

I would be happy if you stay tuned here to re-test in case i decide to remove the quirk completely.

linrunner added a commit that referenced this issue Aug 22, 2021
Issue #573: Fix power_mode not changing from AC to battery.
@linrunner
Copy link
Owner

ps. i forgot to mention why i don't like long wait times: udevd kills tasks started with a RUN rule after 4-5 sec, 1.5 sec would already be a third of that.

@mightyllama89
Copy link
Contributor Author

In fact this assumption holds for almost all laptops.
That is good to know.

Looking at all the commits regarding power supply detection I could tell you hadn't made up your mind on how exactly you wanted it done. I was hoping it would help you if I shared what found out.

consequence of the original quirk, which applied to all ac devices,

The question then becomes how many devices can you not trust the AC offline status. Changing it from all devices (not a quirk) to only ADP1 is a fundamental change and only more testing will tell.

why i don't like long wait times

That makes sense. The status update delay actually varied on mine. Luckily your log reports the time remaining on the wait when it exits the loop. The longest I saw was 1 second so I added .5 more just to be safe. So you might be able to shorten it if you really want to.

Unrelated but probaly not worth opening an issue: in tlp-func-base after line 746 I think you need a ';;' for the case/switch.

Let me know if you need anything else.

linrunner added a commit that referenced this issue Aug 24, 2021
Streamline detection of active power supply:
* AC status has always precedence
* When AC is not detected (or ignored), battery status is used after a
  wait up to 1.5 sec when not 'Discharging' (on 1st battery only)
* quirk removed for MBP 5.3 reporting AC offline when started with charger
  connected; TLP_PS_IGNORE_PS="AC" required instead.

Reference:
* #573
@linrunner
Copy link
Owner

linrunner commented Sep 15, 2021

Please test again with 1.4 beta 2 --> https://download.linrunner.de/packages/

Looking at all the commits regarding power supply detection I could tell you hadn't made up your mind on how exactly you wanted it done. I was hoping it would help you if I shared what found out.

Oh, I know where I want to go. Unfortunately, there are conflicting requirements due to a variety of hardware quirks. Also, I don't want too many users to have to adjust their existing configuration. Some - hopefully few - will have to configure

TLP_IGNORE_PS="AC"

retroactively (not you).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants