Reading fan tables from a configuration file #44

mbway · 2020-08-30T19:12:32Z

added support for reading fan tables from a configuration file.

I chose not to use JSON as the configuration language because:

it is not the most appropriate format for storing tabular data
machine-generated JSON may not be very human-friendly to manually edit
this csv-based configuration format is easier to parse from other languages besides javascript

I'm thinking about building an alternative (more efficient) tccd implementation in C++ for my own use (which I would like to upstream if possible) as a solution to #37 . The first roadblock I hit is that the daemon and the control center share a lot of (typescript) code. The first step, therefore is to decouple the control center and the daemon a bit by reading from configuration files. This has the added benefit of being able to define custom fan curves, providing a partial solution for #8 but without any UI.

nickma82 · 2020-11-12T18:44:26Z

.vscode/launch.json

@@ -10,7 +19,7 @@
            "windows": {
              "runtimeExecutable": "${workspaceRoot}/node_modules/.bin/electron.cmd"
            },
-            "runtimeArgs": ["./dist/tuxedo-control-center"],
+            "runtimeArgs": ["./dist/tuxedo-control-center", "--debug"],


Is this just for executions in VSCode or for release as well?

--debug is only passed when you run the Debug Main Process target from vscode. It would possible to pass to a release build as well. I'm not sure how to avoid that though.

It opens the dev tools / inspector immediately as the application launches which I found useful, especially for placing breakpoints and stepping through things.

nickma82 · 2020-11-12T18:48:28Z

src/common/classes/ConfigHandler.ts

+                currentRows.push([parseInt(match[1]), parseInt(match[2]), parseInt(match[3])]);
+            } else if((match = sectionLineRegex.exec(line)) !== null) {
+                if(currentProfile !== null) {
+                    profiles.push(this.createFanProfile(currentProfile, currentRows));


What happens if currentRows is null?

currentRows should never be null but could be an empty list.
are you saying that empty profiles should be discarded? For example

[MyProfile] [MyOtherProfile] 0,0,0

only MyOtherProfile should be created?

src/common/classes/ConfigHandler.ts

nickma82 · 2020-11-12T18:54:28Z

src/service-app/classes/TuxedoControlCenterDaemon.ts

@@ -134,7 +134,11 @@ export class TuxedoControlCenterDaemon extends SingleProcess {

    public catchError(err: Error) {
        this.logLine('Tccd Exception');
-        const errorLine = err.name + ': ' + err.message;
+        let errorLine = err.name + ': ' + err.message;
+        let stack = err.stack;


could be const?

nickma82 · 2020-11-12T18:58:47Z

README.md

-2. Clone & install libraries
+
+2. Install `tuxedo-cc-wmi` (can be obtained from the respositories [here](https://www.tuxedocomputers.com/en/Infos/Help-and-Support/Instructions/Add-TUXEDO-Computers-software-package-sources.tuxedo#)).
+   This package provides a driver at `/dev/tuxedo_cc_wmi`. The control center can run without this but will not be functional.


Is there a dependency list to add it to?

I'm not sure I understand the question. I just thought I would mention that if you are building the control center from source then you need to install the driver separately. Previously there was no mention of that in the README.

Co-authored-by: NickMa <[email protected]>

nickma82 · 2020-11-30T09:07:51Z

I'd love to see this one merged! IMHO fan speed can be more aggressive.

mbway · 2021-01-30T23:41:56Z

is anyone ever going to have a look at this or tell me what should be done differently? It's fine if there are reasons you don't want to take my implementation but just ignoring it for months is a bit annoying. I see developers responding to issues and making commits so it's not that the project is dormant.

Matheus-Garbelini · 2021-03-06T16:38:03Z

This merge is really important.
For now, I'm going to use @mbway fork.

tuxedoxt · 2021-03-08T09:38:16Z

Hello,

please don't take offence, there is no intentional ignoring going on :) Sometimes feature requests are overlooked or postponed due to other matters. An answer is however warranted!

In the case of this merge request it goes in a slightly different direction. The implementation is still unclear but there are added features planned TUXEDO-wise (which are needed) for modifying the fan behaviour. Also as you mention in the original message, you intend to reimplement tccd in C++. This is of course very much possible and because of the already existing decoupled c++ hardware interface should be pretty simple. This also, however, there currently are no plans on implementing in the main repo.

For now it sounds like this is best left out of the main repo and used separately as an alternative by those who find it useful. There seem there might be enough interest.

Apart from the bigger design decisions there are some useful commits regarding debugging and descriptions that could be useful for sure!

mbway · 2021-03-08T12:17:40Z

[creating an alternative tccd] is of course very much possible

but not in a way that can be configured and kept in sync with the settings configured in the control center since settings like the fan curves do not go through configuration files but instead refer to data structures in the tcc code itself. I would rather not keep a fork up to date with any changes made to the source code. It makes sense for such things to be communicated to the daemon without requiring source code to be shared. This makes it easier for other projects to share the configuration without having to parse the source code and gives people the option to edit configuration files without a GUI if they prefer.

there currently are no plans on implementing [a native daemon] in the main repo

Ok, in which case if I do write this daemon I will not submit a pull request. This makes it a bit easier because I will probably use rust instead of C++ (C++ was my suggestion since it would have been more likely to be accepted to the main repo) and will probably not attempt to interface with the control center at all and just use the tuxedo driver but none of the userspace logic or configuration files.

The reason I wanted to take the approach of trying to get things merged upstream is so that the maximum number of people can benefit, rather than creating something just for myself and maybe a few people that stumble across it.

The implementation is still unclear but there are added features planned TUXEDO-wise (which are needed) for modifying the fan behaviour

does this mean that custom fan curves are going to be added, but not by using a configuration file in /etc/tcc? Or will configuration files be used but you don't like the parser / configuration format implemented in this pull request?

For now it sounds like this is best left out of the main repo

Ok that's fine. Should I close this pull request or create a new one with cherry-picked commits that you would like to take?

tuxedoxt · 2021-03-08T13:21:34Z

but not in a way that can be configured and kept in sync with the settings configured in the control center since settings like the fan curves do not go through configuration files but instead refer to data structures in the tcc code itself. I would rather not keep a fork up to date with any changes made to the source code. It makes sense for such things to be communicated to the daemon without requiring source code to be shared. This makes it easier for other projects to share the configuration without having to parse the source code and gives people the option to edit configuration files without a GUI if they prefer.

This is true. Some things are still to do to communicate through the d-bus. Example that comes to mind is TCC gui still reads the config files itself and figures out which profile is active => state of tccd should come from tccd directly.

The reason I wanted to take the approach of trying to get things merged upstream is so that the maximum number of people can benefit, rather than creating something just for myself and maybe a few people that stumble across it.

Yeah the intentions are great!

does this mean that custom fan curves are going to be added, but not by using a configuration file in /etc/tcc? Or will configuration files be used but you don't like the parser / configuration format implemented in this pull request?

Yes most likely, probably will be some config files, yes, and kind of. Yeah it's just that. Not taking it too far in any direction before it's known what is actually needed, work optimization etc. Also, two types of config files might be one too many ;)

Ok that's fine. Should I close this pull request or create a new one with cherry-picked commits that you would like to take?

Right, the debug + readme additions look useful at a glance. (With the tuxedo-cc-wmi => tuxedo-io change) If you'd like to put them in a PR I'd be happy to include them.

Matheus-Garbelini · 2021-03-09T04:30:35Z

hum... Here's my suggestion. I think there could be too much overengineering over this without need:
Isn't it much easier or faster to

Merge @mbway PR on a separate branch, but use /home/$USER/.tuxedo as a global path to the shared configuration
Implement on this TCC an experimental headless mode towards having the final goal of a standalone daemon that works with other clients by whatever endpoint communication you guys want (more on that later). Since TCC is using electron and electron does support headless mode, the guys working on this repo already have experience with how to only import the necessary GUI or tray handling code to make it headless or not while the main fan control code is running (easier to maintain for them). Using an argument --headless to do the switch for testing (don't worry about config files for now).
Use this repo, on a different branch for the experimental headless mode implementation which could be helped by @mbway . Many projects like to create different repos for different features just to later figure it out it was easier work on a single one, albeit a different branch. In short, give PR permission for certain users on this new branch.
once headless is working via the --headless switch, other options can be added on /home/$USER/.tuxedo alongside the existing files from @mbway. There is no issue in using more than one configuration format. For example, yaml or json for daemon options and .csv for tables. The idea is that headless mode runs as root and loads the desired profile name via the configuration file. For instance, reusing the code that the user press "save" for the desired profile change when initializing.
D-bus support can be added as a final feature once tuxedo control center works with or without GUI via headless switch so profiles can be changed during runtime on headless mode.

In short, there is no need to rework a separate daemon if it's simpler to cut the minimum things on TCC to make it work without the GUI. Plus, @tuxedoxt , @mbway and other tuxedo developers can continue developing the same GUI without the hassle of reworking on a completely isolated GUI.

However, I'm not sure if tuxedo as a company has some policy on the permission of the repo, so in this case, @tuxedoxt could not accept PR even if he wanted, without some approval 😨. I'm saying this because only the employers of Tuxedo had PRs accepted so far.

brunoais · 2021-03-09T08:02:57Z

1. Merge @mbway PR on a separate branch, but use /home/$USER/.tuxedo  as a global path to the shared configuration

/home/$USER/.config/.tuxedo or /home/$USER/.local/share/.tuxedo instead please.
Bc those are directories meant for configuration files. I already have too many programs that use my home directory to put their own settings while there's 2 directories for those already.

For the rest, I agree with you.

Matheus-Garbelini · 2021-03-09T08:12:10Z

@brunoais agreed, /home/$USER/.config/.tuxedo is indeed better as a lot of other programs already use this as their default config path.

mbway · 2021-03-09T11:26:54Z

I'm not sure I understand this comment:

cut the minimum things on TCC to make it work without the GUI

The control center already has a GUI component which uses electron, and a standalone node.js application (tccd) which is always running and carries out the configuration as instructed by the control center (via configuration files) and monitors temperature and updates fan speed accordingly. Currently several source files are shared by both programs and there is configuration state (like fan curves) in them.

My main issue is with the daemon is that it uses an excessive amount of cpu at all times: #37 and it doesn't handle well when the system is under load (which is when fan control is most important!) #29.
The control center also configures things like screen brightness, cpu govener etc which I would prefer the daemon didn't change at all so making a simple daemon which uses the tuxedo driver to just control fans could be a nice solution. I would still be open to working with tuxedo computers on this to improve the official daemon but it looks like they aren't interested.

Matheus-Garbelini · 2021-03-09T12:08:07Z

Thanks @mbway I was not aware of all services there. I thought that the control center and GUI were not isolated as separate services, but rather threads.

sleepingtux · 2021-05-20T18:54:55Z

hum... Here's my suggestion. I think there could be too much overengineering over this without need:
Isn't it much easier or faster to

Merge @mbway PR on a separate branch, but use /home/$USER/.tuxedo as a global path to the shared configuration

Implement on this TCC an experimental headless mode towards having the final goal of a standalone daemon that works with other clients by whatever endpoint communication you guys want (more on that later). Since TCC is using electron and electron does support headless mode, the guys working on this repo already have experience with how to only import the necessary GUI or tray handling code to make it headless or not while the main fan control code is running (easier to maintain for them). Using an argument --headless to do the switch for testing (don't worry about config files for now).

Use this repo, on a different branch for the experimental headless mode implementation which could be helped by @mbway . Many projects like to create different repos for different features just to later figure it out it was easier work on a single one, albeit a different branch. In short, give PR permission for certain users on this new branch.

once headless is working via the --headless switch, other options can be added on /home/$USER/.tuxedo alongside the existing files from @mbway. There is no issue in using more than one configuration format. For example, yaml or json for daemon options and .csv for tables. The idea is that headless mode runs as root and loads the desired profile name via the configuration file. For instance, reusing the code that the user press "save" for the desired profile change when initializing.

D-bus support can be added as a final feature once tuxedo control center works with or without GUI via headless switch so profiles can be changed during runtime on headless mode.

In short, there is no need to rework a separate daemon if it's simpler to cut the minimum things on TCC to make it work without the GUI. Plus, @tuxedoxt , @mbway and other tuxedo developers can continue developing the same GUI without the hassle of reworking on a completely isolated GUI.

However, I'm not sure if tuxedo as a company has some policy on the permission of the repo, so in this case, @tuxedoxt could not accept PR even if he wanted, without some approval . I'm saying this because only the employers of Tuxedo had PRs accepted so far.

Hello. That's exactly what i am doing

a TCD daemon in C++ using the tuxedo kernel module that will control fan and only set somethings that you want by script (for exemple set TDP number of core enabled but not a lot of things), it will read curve from file. ;)
a gui in QT (or somebody could do other)

Of course all will be openSource and will be on git.

Matheus-Garbelini · 2021-05-20T19:36:43Z

@sleepingtux nice to know. If I may suggest, you can add an offset parameter that increases all values in the table by a fixed value.

sleepingtux · 2021-05-20T19:41:59Z

@sleepingtux nice to know. If I may suggest, you can add an offset parameter that increases all values in the table by a fixed value.

Lol that exactly what i did. :D
I let you choose the offset, but for my test i am using a 30° offset. ;)

mbway added 9 commits November 7, 2020 18:58

added tuxedo_cc_wmi to installation instructions

af1cbd9

added start-service command

a9960da

added option to open dev tools with --debug

67a1348

reading fan tables from a configuration file

22dce61

removed hard coded fan table defaults

26a7b81

small fixes

065525f

added a way to debug the service

033710c

added stack trace to error printing

dfb9bc6

removed script used to create hard coded fan tables

7709ee9

mbway force-pushed the decoupling branch from 9eca62f to 7709ee9 Compare November 7, 2020 19:01

nickma82 reviewed Nov 12, 2020

View reviewed changes

mbway and others added 3 commits November 12, 2020 20:04

changed error message

9ed1256

Co-authored-by: NickMa <[email protected]>

print line number in error message

29c0d76

use const

7e639b9

nickma82 mentioned this pull request Nov 30, 2020

Option to change the CPU governor #22

Closed

mbway mentioned this pull request Feb 9, 2021

tccd uses 3% CPU continuously #37

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reading fan tables from a configuration file #44

Reading fan tables from a configuration file #44

mbway commented Aug 30, 2020

nickma82 Nov 12, 2020

mbway Nov 12, 2020

nickma82 Nov 12, 2020

mbway Nov 12, 2020

nickma82 Nov 12, 2020

nickma82 Nov 12, 2020

mbway Nov 12, 2020

nickma82 commented Nov 30, 2020

mbway commented Jan 30, 2021

Matheus-Garbelini commented Mar 6, 2021

tuxedoxt commented Mar 8, 2021

mbway commented Mar 8, 2021

tuxedoxt commented Mar 8, 2021

Matheus-Garbelini commented Mar 9, 2021 •

edited

Loading

brunoais commented Mar 9, 2021

Matheus-Garbelini commented Mar 9, 2021

mbway commented Mar 9, 2021

Matheus-Garbelini commented Mar 9, 2021

sleepingtux commented May 20, 2021

Matheus-Garbelini commented May 20, 2021

sleepingtux commented May 20, 2021

Reading fan tables from a configuration file #44

Are you sure you want to change the base?

Reading fan tables from a configuration file #44

Conversation

mbway commented Aug 30, 2020

nickma82 Nov 12, 2020

Choose a reason for hiding this comment

mbway Nov 12, 2020

Choose a reason for hiding this comment

nickma82 Nov 12, 2020

Choose a reason for hiding this comment

mbway Nov 12, 2020

Choose a reason for hiding this comment

nickma82 Nov 12, 2020

Choose a reason for hiding this comment

nickma82 Nov 12, 2020

Choose a reason for hiding this comment

mbway Nov 12, 2020

Choose a reason for hiding this comment

nickma82 commented Nov 30, 2020

mbway commented Jan 30, 2021

Matheus-Garbelini commented Mar 6, 2021

tuxedoxt commented Mar 8, 2021

mbway commented Mar 8, 2021

tuxedoxt commented Mar 8, 2021

Matheus-Garbelini commented Mar 9, 2021 • edited Loading

brunoais commented Mar 9, 2021

Matheus-Garbelini commented Mar 9, 2021

mbway commented Mar 9, 2021

Matheus-Garbelini commented Mar 9, 2021

sleepingtux commented May 20, 2021

Matheus-Garbelini commented May 20, 2021

sleepingtux commented May 20, 2021

Matheus-Garbelini commented Mar 9, 2021 •

edited

Loading