Adding support for framework-laptop-kmod as an alternative to ectool #47
Conversation
|
I've also added using the GPU temperature as well (as an option). |
|
Hello, thank you for your contribution, I will check your various PRs in detail in about 3 hours. At first glance, this one looks pretty good but I think all these conditions add a lot of complexity (and more in the future if other ways to get temperature/control the fans are implemented as well). This should reduce the complexity and make the code a bit easier to read. |
|
I can do that. |
|
How is this? |
|
Did a quick test using both ectool and framework-laptop-kmod. |
|
Hi, thanks for your hard work! |
| return self.configuration.getDefaultStrategy() | ||
| return self.configuration.getDischargingStrategy() | ||
| @abstractmethod | ||
| def pause(self): |
There was a problem hiding this comment.
self.active = False should be present in any implementation of the pause method, so I think that this function should not be abstract but extended instead.
| text=True).stdout | ||
| return len(re.findall(r'Flags.*(AC_PRESENT)', rawOut)) > 0 | ||
| @abstractmethod | ||
| def setSpeed(self, speed: int) -> None: |
There was a problem hiding this comment.
The same applies here, self.speed = speed should always be present.
|
|
||
| return round(temps[0], 1) | ||
| def getBatteryChargingStatus(self): |
There was a problem hiding this comment.
This part still uses ectool even in kmod mode, is there an equivalent?
There was a problem hiding this comment.
Oh, I never used that feature. Totally overlooked this. ;)
| for file in hwmon_interface.iterdir(): | ||
| if file.name.startswith("temp") and file.name.endswith("_input"): | ||
| self.sysfs_get_temp.append(file) | ||
| elif self.configuration.getUseGPU() and device_name == "amdgpu": |
There was a problem hiding this comment.
I'm struggling to understand why the end user would want to disable the GPU temperature sensor.
Please let me know me if I have missed something.
There was a problem hiding this comment.
Disabled is the default state, if I can, I make changes to code that don't change the main functionality.
Since it didn't include the dGPU temperature before, that's the default configuration right now.
Still, it's preferable to have it on. ;)
There was a problem hiding this comment.
it didn't include the dGPU temperature before
In fact, it does since March
You can disable it if you want, but it definitely should be on by default.
If this option stays, could you please also document it in the README, and add its default state in the config.json?
| and len(self.sysfs_reset_to_auto_files) == 0 and len(self.sysfs_get_temp) == 0: | ||
| raise KModNotWorkingException(f"Could not find files necessary to control fans") | ||
|
|
||
| if strategyName is not None and strategyName != "": |
There was a problem hiding this comment.
This part should be present in every constructor, I think it might deserves its own function if you want to call it only after the initialization.
| args = parser.parse_args() | ||
|
|
||
| if args.run: | ||
| strategy = args.strategy | ||
| if strategy is None: | ||
| strategy = args._strategy | ||
| fan = FanController(configPath=args.config, strategyName=args.strategy) | ||
| config = Configuration(args.config) | ||
| mode = config.getMode() |
There was a problem hiding this comment.
I think the choice between using kmod or ectool should be made using the script arguments instead of the config file as it cannot be changed at runtime.
This would also allow users to choose via the install script.
There was a problem hiding this comment.
It could be changed at runtime.
We could easily swap them out.
There was a problem hiding this comment.
Doing that would require a complete rework of our runtime cycles, and considering the fact that it certainly will never be changed after the initial setup, I believe it is not worth it
There was a problem hiding this comment.
I'm just playing with the idea, I know it's pointless to change it at runtime.
|
Hi @NAKlama, Are you still working on it? Have a nice day |
leopoldhub
added
the
inactive
Since there is a kernel module (https://github.com/DHowett/framework-laptop-kmod
) allowing fan control over sysfs, I added the ability to use that instead of ectool.
It would allow those unhappy with the supplied binary to use the kernel module instead.