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

bat.d/framework: Add initial support for framework laptop #728

Closed

Conversation

chutz
Copy link

@chutz chutz commented Feb 22, 2024

This requires the currently out of tree module for the framework laptop available at https://github.com/DHowett/framework-laptop-kmod

This started with the template and added support for the framework laptop.

@linrunner
Copy link
Owner

Hi, thanks.

I'll take a look at this soon. I'm just busy at the moment with necessary changes that have resulted from testing the macbook plugin. Stay tuned please.

bat.d/41-framework Outdated Show resolved Hide resolved
bat.d/41-framework Outdated Show resolved Hide resolved
bat.d/41-framework Show resolved Hide resolved
bat.d/41-framework Outdated Show resolved Hide resolved
bat.d/41-framework Show resolved Hide resolved
bat.d/41-framework Show resolved Hide resolved
bat.d/41-framework Show resolved Hide resolved
bat.d/41-framework Show resolved Hide resolved
bat.d/41-framework Outdated Show resolved Hide resolved
bat.d/41-framework Outdated Show resolved Hide resolved
@linrunner
Copy link
Owner

linrunner commented Mar 24, 2024

Sorry for the overly long delay. See my review for required changes.

bat.d/41-framework Outdated Show resolved Hide resolved
linrunner added a commit that referenced this pull request Jun 16, 2024
* Requires framework_laptop kernel module (currently out-of-tree)
* Stop threshold only
* Force discharge not supported

* Requires Framework Laptop 13/16 Intel/AMD

* charge_control_end_threshold:
  - Valid values: 1..100
  - Default value: 100

Credits:
* #728

Reference:
* https://github.com/DHowett/framework-laptop-kmod/tree/main
@linrunner
Copy link
Owner

@chutz due to your lack of response, i have created my own implementation -> https://github.com/linrunner/TLP/tree/feature/bat.d/framework

@Simerax can you test it?

@t-8ch
Copy link
Contributor

t-8ch commented Jun 16, 2024

FYI I wrote a generic ChromeOS EC charge control driver [0]
While it won't probe by default on Framework (because the upstream CrOS EC commands are overridden by their downstream APIs) I hope to unify the Framework APIs so that driver will be used on Framework.

It would be great if TLP could be compatible with that driver.
(I expect it to make it into 6.11)

[0] https://lore.kernel.org/lkml/[email protected]/T/#t

@linrunner
Copy link
Owner

linrunner commented Jun 17, 2024

@t-8ch I'm very pleased to hear from you on this topic. I would have emailed you otherwise.

In principle, I would prefer to support your generic driver instead of framework_laptop, which is currently out-of-tree. I had my share of trouble with ThinkPad out-of-tree modules, I don't necessarily need that again.

However, the connections are not yet clear to me:

  1. Does your driver provide the sysfiles charge_control_{start,end}_threshold itself or is framework_laptop still needed for this?

  2. Are both start and stop thresholds supported? framework_laptop provides stop only, but this article by the author suggests that the hardware can do both: https://www.howett.net/posts/2021-12-framework-ec/#3e03---charge-limit-control

  3. What is the exact name your generated module, i.e. under /sys/module/?

@t-8ch
Copy link
Contributor

t-8ch commented Jun 17, 2024

The driver will (most likely) be called cros_charge-control.

It supports start/stop thresholds and charge_behaviour through the standard sysfs attributes.
On older hardware it only supports charge_behaviour but no thresholds.
AFAIK this case is new and may need some changes to TLP.
OTOH it allows simulating thresholds in userspace by toggling between force-discharge and auto.

The hardware has all of the features, as proven by the upstream commands being supported and functional. For some reason Framework reinvented their own downstream commands.

I plan on writing some patches for the EC firmware to unify both command sets, so the vanilla CrOS EC driver can work on the Framework without user intervention.

@Simerax
Copy link

Simerax commented Jun 20, 2024

@chutz due to your lack of response, i have created my own implementation -> https://github.com/linrunner/TLP/tree/feature/bat.d/framework

@Simerax can you test it?

Yes! Just checked out your branch and will now test it. :) I will probably comment my findings tomorrow or sometime over the weekend :)

@Simerax
Copy link

Simerax commented Jun 20, 2024

Just realized it needs the out of tree module..would it maybe make more sense to wait for t-8ch driver in the kernel instead of now "quickly" getting the support with an out of tree module?

@linrunner
Copy link
Owner

@Simerax you're knocking down open doors. However, I wouldn't mind if you could test the current plugin with the out-of-tree module.

@Simerax
Copy link

Simerax commented Jun 21, 2024

@linrunner Okay I tested it with the current master of the framework_laptop module.

Battery Care detects the plugin and the STOP_CHARGE_THRESH_BAT1 works. I'm guessing its expected that START_CHARGE_THRESH_BAT1 does not work right now since you said the framework_laptop module does not support that.

Are there any other settings I should test? Do you want/need any stat outputs?

linrunner added a commit that referenced this pull request Jun 22, 2024
* Requires framework_laptop kernel module (currently out-of-tree)
* Stop threshold only
* Force discharge not supported

* Requires Framework Laptop 13/16 Intel/AMD

* charge_control_end_threshold:
  - Valid values: 1..100
  - Default value: 100

Credits:
* #728

Reference:
* https://github.com/DHowett/framework-laptop-kmod/tree/main
@linrunner
Copy link
Owner

@Simerax Thanks so far.

I need the output of

sudo tlp-stat -s -b 

Please also run the automatic test script and show the output:
https://github.com/linrunner/TLP/blob/feature/bat.d/framework/unit-tests/charge-thresholds_framework

It requires the tool clitest to be installed. Should be available as a package in most distributions.

@Simerax
Copy link

Simerax commented Jun 23, 2024

Clitest doesn't seem to be available on void. Is this the tool and repository you are referring to? https://github.com/aureliojargas/clitest

Output of tlp-stat -s -b

+++ System Info
System         = Framework AA Laptop
BIOS           = 03.17
OS Release     = Void Linux
Kernel         = 6.6.34_1 #1 SMP PREEMPT_DYNAMIC Mon Jun 17 11:50:08 UTC 2024 x86_64
/proc/cmdline  = BOOT_IMAGE=/boot/vmlinuz-6.6.34_1 root=/dev/mapper/vvm-root ro loglevel=4 rd.lvm.vg=vvm rd.luks.uuid=XXX apparmor=1 security=apparmor
Init system    = sysvinit
Boot mode      = UEFI
Suspend mode   = [s2idle] deep

+++ TLP Status
State          = enabled
RDW state      = not installed
Last run       = 19:16:04, 4 sec(s) ago
Mode           = AC
Power source   = AC

+++ Battery Care
Plugin: framework
Supported features: charge threshold
Driver usage:
* natacpi (framework_laptop) = active (charge thresholds)
Parameter value ranges:
* STOP_CHARGE_THRESH_BAT0/1:   1..100(default)

+++ Battery Status: BAT1
/sys/class/power_supply/BAT1/manufacturer                   = NVT
/sys/class/power_supply/BAT1/model_name                     = Framewo
/sys/class/power_supply/BAT1/cycle_count                    =    101
/sys/class/power_supply/BAT1/charge_full_design             =  35720 [mAh]
/sys/class/power_supply/BAT1/charge_full                    =  31970 [mAh]
/sys/class/power_supply/BAT1/charge_now                     =  14530 [mAh]
/sys/class/power_supply/BAT1/current_now                    =      1 [mA]
/sys/class/power_supply/BAT1/status                         = Charging

/sys/class/power_supply/BAT1/charge_control_end_threshold   =     45 [%]

Charge                                                      =   45.4 [%]
Capacity                                                    =   89.5 [%]

@linrunner
Copy link
Owner

Is this the tool and repository you are referring to? https://github.com/aureliojargas/clitest

Yes.

@Simerax
Copy link

Simerax commented Jun 27, 2024

sorry for the delay. Output of clitest:

#1	# +++ Framework laptops +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
#2	#
#3	# --- tlp start
#4	sudo tlp start -- START_CHARGE_THRESH_BAT1= STOP_CHARGE_THRESH_BAT1= START_CHARGE_THRESH_BAT0= STOP_CHARGE_THRESH_BAT0=
Passwort:
#5	sudo tlp start -- START_CHARGE_THRESH_BAT1="60" STOP_CHARGE_THRESH_BAT1="100" START_CHARGE_THRESH_BAT0= STOP_CHARGE_THRESH_BAT0=
#6	sudo tlp start -- START_CHARGE_THRESH_BAT1="na" STOP_CHARGE_THRESH_BAT1="0" START_CHARGE_THRESH_BAT0= STOP_CHARGE_THRESH_BAT0=
#7	sudo tlp start -- START_CHARGE_THRESH_BAT1="0" STOP_CHARGE_THRESH_BAT1="101" START_CHARGE_THRESH_BAT$
#8	sudo tlp start -- START_CHARGE_THRESH_BAT1="na" STOP_CHARGE_THRESH_BAT1="86" START_CHARGE_THRESH_BAT0= STOP_CHARGE_THRESH_BAT0=
#9	sudo tlp start -- START_CHARGE_THRESH_BAT1="DEF" STOP_CHARGE_THRESH_BAT1="DEF" START_CHARGE_THRESH_BAT0= STOP_CHARGE_THRESH_BAT0=
#10	sudo tlp start -- NATACPI_ENABLE=0 START_CHARGE_THRESH_BAT1="DEF" STOP_CHARGE_THRESH_BAT1="DEF"
#11	#
#12	# --- tlp setcharge w/o arguments
#13	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="60" STOP_CHARGE_THRESH_BAT1="100"
#14	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="na" STOP_CHARGE_THRESH_BAT1="0"
#15	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="0" STOP_CHARGE_THRESH_BAT1="101"
#16	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="ABCDE" STOP_CHARGE_THRESH_BAT1="XYZZY"
#17	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="na" STOP_CHARGE_THRESH_BAT1="100"
#18	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="na" STOP_CHARGE_THRESH_BAT1="86"
#19	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="na" STOP_CHARGE_THRESH_BAT1="80"
#20	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="na" STOP_CHARGE_THRESH_BAT1="80"
#21	sudo tlp setcharge -- START_CHARGE_THRESH_BAT1="DEF" STOP_CHARGE_THRESH_BAT1="DEF"
#22	sudo tlp setcharge -- NATACPI_ENABLE=0 START_CHARGE_THRESH_BAT1="DEF" STOP_CHARGE_THRESH_BAT1="DEF"
#23	#
#24	# --- tlp setcharge w/ arguments
#25	sudo tlp setcharge 60 100 BAT1
#26	sudo tlp setcharge 0 0 BAT1
#27	sudo tlp setcharge 0 101
#28	sudo tlp setcharge ABCDE 0
#29	sudo tlp setcharge 0 XYZZY
#30	sudo tlp setcharge 97 100
#31	sudo tlp setcharge 0 66
#32	sudo tlp setcharge 0 60
#33	sudo tlp setcharge 0 60 -- X_THRESH_SIMULATE_START="35" X_THRESH_SIMULATE_STOP="100"
#34	sudo tlp setcharge 0 60
#35	sudo tlp setcharge DEF DEF
#36	sudo tlp setcharge BAT0
#37	sudo tlp setcharge 0 3 BAT0
#38	sudo tlp setcharge XYZZY ABCDE BAT0
#39	#
#40	# --- tlp-stat
#41	sudo tlp-stat -b -- | grep "charge_control_end_threshold"
#42	sudo tlp-stat -b -- X_THRESH_SIMULATE_READERR=1 | grep "charge_control_end_threshold"
#43	#
#44	# --- Reset test machine to configured thresholds
#45	sudo tlp setcharge BAT1  > /dev/null 2>&1
#46	#
OK: 46 of 46 tests passed

@linrunner
Copy link
Owner

linrunner commented Jun 27, 2024

Wow. It actually fitted perfectly on the first try. Thank you very much.

Let's just save this in the back of our minds. Now I'll wait for the cros_charge-control module be merged in mainline. I would be pleased if you could also test the corresponding plugin.

linrunner added a commit that referenced this pull request Jun 27, 2024
* Requires framework_laptop kernel module (currently out-of-tree)
* Stop threshold only
* Force discharge not supported

* Requires Framework Laptop 13/16 Intel/AMD

* charge_control_end_threshold:
  - Valid values: 1..100
  - Default value: 100

Credits:
* #728

Reference:
* https://github.com/DHowett/framework-laptop-kmod/tree/main
linrunner added a commit that referenced this pull request Jul 10, 2024
* Requires framework_laptop kernel module (currently out-of-tree)
* Stop threshold only
* Force discharge not supported

* Requires Framework Laptop 13/16 Intel/AMD

* charge_control_end_threshold:
  - Valid values: 1..100
  - Default value: 100

Credits:
* #728

Reference:
* https://github.com/DHowett/framework-laptop-kmod/tree/main
This requires the currently out of tree module for the framework laptop
available at https://github.com/DHowett/framework-laptop-kmod
@chutz chutz force-pushed the initial-framework-laptop-battery-support branch from 13efc85 to 3f0c428 Compare August 22, 2024 21:52
@chutz
Copy link
Author

chutz commented Aug 22, 2024

Sorry about disappearing for awhile here.

I have updated this MR to handle the cros_charge-control module that has been merged into mainline starting at 6.11. With this module, the cros_charge_control module needs the probe_with_fwk_charge_control option passed to it for this to work. I added documentation for this to the comments at the start of the file, I am not sure if there is a better way to do this.

@linrunner
Copy link
Owner

@chutz : thanks so far. I would prefer your PR to be based on my branch https://github.com/linrunner/TLP/tree/feature/bat.d/framework . It already includes my changes to the TEMPLATE for 1.7.

But before you do that, we need to clarify some points:

  1. Is your detection intentionally not cros_charge-control specific?
    case “$(read_sysf $BATDRV_FRAMEWORK_MD)” in
        FRA*) return 0;;

The condition is also met by the framework_laptop module, refer to the above output from @Simerax. framework_laptop does not support a start threshold though.

What about checking for /sys/module/cros_charge_control?

  1. On the one hand the code is detecting /sys/class/power_supply/BAT1, on the other hand the loops over BAT[01] are still present. Adjusting the detection to cros_charge-control would resolve this contradiction.

  2. Threshold defaults don't make sense to me:

_bt_def_start=50
_bt_def_stop=60 

They are used for tlp fullcharge, so at least it should be _bt_def_stop=100. What are the hardware (e.g. firmware) defaults if no thresholds are set?

@linrunner
Copy link
Owner

linrunner commented Aug 26, 2024

@t-8ch

OTOH it allows simulating thresholds in userspace by toggling between force-discharge and auto.

Really? No way TLP will implement that :-D.

@t-8ch
Copy link
Contributor

t-8ch commented Aug 26, 2024

If the cros_charge_ctl module is used, should the TLP functionality still be called "framework"?

Really? No way TLP will implement that :-D.

Not saying that it's reasonable, only possible.

linrunner added a commit that referenced this pull request Sep 29, 2024
* Requires framework_laptop kernel module (currently out-of-tree)
* Stop threshold only
* Force discharge not supported

* Requires Framework Laptop 13/16 Intel/AMD

* charge_control_end_threshold:
  - Valid values: 1..100
  - Default value: 100

Credits:
* #728

Reference:
* https://github.com/DHowett/framework-laptop-kmod/tree/main
linrunner added a commit that referenced this pull request Oct 4, 2024
* Requires framework_laptop kernel module (currently out-of-tree)
* Stop threshold only
* Force discharge not supported

* Requires Framework Laptop 13/16 Intel/AMD

* charge_control_end_threshold:
  - Valid values: 1..100
  - Default value: 100

Credits:
* #728

Reference:
* https://github.com/DHowett/framework-laptop-kmod/tree/main
@linrunner
Copy link
Owner

@t-8ch @Simerax @chutz To distinguish my new implementation from the original PR and my old approach, I'm closing. Continue in #765 please.

@linrunner linrunner closed this Oct 4, 2024
linrunner added a commit that referenced this pull request Oct 4, 2024
* Requires framework_laptop kernel module (currently out-of-tree)
* Stop threshold only
* Force discharge not supported

* Requires Framework Laptop 13/16 Intel/AMD

* charge_control_end_threshold:
  - Valid values: 1..100
  - Default value: 100

Credits:
* #728

Reference:
* https://github.com/DHowett/framework-laptop-kmod/tree/main
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

Successfully merging this pull request may close these issues.

4 participants