Separating FanController into different subclasses to allow HardwareController and SocketController diversity. #50

leopoldhub · 2024-06-19T19:54:10Z

separating FanController into different subclasses to allow HardwareController and SocketController diversity.
This should make things easier for #47 and windows support #30 .

leopoldhub · 2024-06-19T19:55:24Z

Hi @TamtamHero , I hope you are doing well.

Could you please double check this one if you have some time?

Have a great day!

TamtamHero · 2024-06-20T09:01:08Z

Hello @leopoldhub !
I'm a bit late to the party, sorry
Can you detail a little more why supporting 2 methods is useful, vs being opiniated on picking a single one ?
The issue #47 mentions "It would allow those unhappy with the supplied binary to use the kernel module instead."
Who isn't happy with ectool, actually ? There was some issues with ectool not being up to date for latest models, but it was my mistake and it should not happen again. What am I missing ?
For issue #30, can you also explain how this PR would help (have you been able to try ectool on WIndows ? Does it work there ?)

leopoldhub · 2024-06-20T21:28:38Z

Hi @TamtamHero ,
Thanks for the quick reply!

I think that #47 was originally created because people don't like having binary blobs in repositories (which I can understand). #51 should fix that, and #47 may become irrelevant.

For issue #30, can you also explain how this PR would help

Unix sockets don't work on Windows, and this extraction will make it easier to implement Windows-specific ones (same goes if we can't use ectool and have to use some other method).

(have you been able to try ectool on WIndows ? Does it work there ?)

Someone I know got it to work on a Framework 13 with Windows 10, I should receive my secondary SSD soon and will be able to try it by myself.

I hope this answer your questions ^^

TamtamHero · 2024-06-21T07:39:29Z

Okay, so thanks to your work on #51 the only remaining reason to implement this would be in the case where ectool is not working correctly on Windows ?
Then, the way I see it, it's better to wait until we check this before adding these changes, because there is a chance that we don't actually need them
What do you think ?

leopoldhub · 2024-06-21T09:28:59Z

Sure, the socket controller will definitely be needed, but the hardware one should not (I hope).

Features aside, I also like the fact that there are separate components with a specific purpose.
This prevents us from using the ectool in a place where it should not be, and makes the individual components (especially the fan controller) lighter and easier to read.

leopoldhub · 2024-06-24T17:11:32Z

Hi @TamtamHero , how was your day?

I received my SSD, and managed to use the ectool with Windows.
I had a bit of trouble getting it to work due to the difference in behaviour between how Windows and Linux manage I/O user interactions, but had the great help of Dustin (@DHowett) to help me troubleshoot.

With this, the only missing piece is a Windows socket equivalent, and we should be good for a Windows version!

TamtamHero · 2024-06-24T21:02:15Z

Very nice, well done !
Glad to hear that, Dustin helped again <3

About this PR, frankly you can forget what I wrote before it's garbage, I didn't get what you were doing here
It's all good 👍

From 15 seconds of googling, it looks like Windows has added support for AF_UNIX sockets in 2017, so maybe there's no code to write at all ? 👼

leopoldhub · 2024-06-24T21:07:04Z

Glad to hear that, Dustin helped again <3

Yes, he is the GOAT

About this PR, frankly you can forget what I wrote before it's garbage, I didn't get what you were doing here It's all good 👍

No, don't worry about that, It's all good too 👍

From 15 seconds of googling, it looks like Windows has added support for AF_UNIX sockets in 2017, so maybe there's no code to write at all ? 👼

it would indeed be very nice not having to re-implement the sockets

TamtamHero · 2024-06-24T21:09:14Z

Sadly, this is kind of stuck: python/cpython#14823

leopoldhub · 2024-07-06T02:19:54Z

@TamtamHero
I... messed up...
I synced my dev branch with this repo main one without thinking much and happily deleted my work 🫠...
Will have to to it again... yay...

If you happen to still have the hash of my last commit before the sync, there may be a way to retrieve it, so let me know

DHowett · 2024-07-06T02:24:50Z

@leopoldhub hey, you can still get your work back!

f7b9169

FWIW: it shows up here -

leopoldhub · 2024-07-06T02:29:31Z

@DHowett the GOAT as usual 🎉
Got the work back!
Will have to create a new PR though

DHowett · 2024-07-06T02:32:22Z

@leopoldhub aw, thanks.

You can force push that commit back to this PR with...

git push https://github.com/leopoldhub/fw-fanctrl.git f7b9169:refs/heads/dev

leopoldhub · 2024-07-06T02:43:53Z

Thanks a lot, unfortunately it didn't work ("Everything up-to-date")
I will create a new pr and link it to this one

DHowett · 2024-07-06T02:45:47Z

Alas! Sorry that didn't work out.

…ontroller and SocketController diversity. 2 (Repost of #50) (#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked)

@Svenum

* Adding @Svenum as an assignee to nix-related issues (#43) * Fixing adding @Svenum as an assignee to nix-related issues (#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * Clarify behaviour on service stop or pause (#53) (#55) * Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of #50) (#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked) * add fw-ectool in module * fixing missing print for command execution (#63) --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]>

@Svenum

* Adding @Svenum as an assignee to nix-related issues (TamtamHero#43) * Fixing adding @Svenum as an assignee to nix-related issues (TamtamHero#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (TamtamHero#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (TamtamHero#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (TamtamHero#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * Clarify behaviour on service stop or pause (TamtamHero#53) (TamtamHero#55) * Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of TamtamHero#50) (TamtamHero#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked) * add fw-ectool in module * fixing missing print for command execution (TamtamHero#63) --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]>

@Svenum

* Adding @Svenum as an assignee to nix-related issues (TamtamHero#43) * Fixing adding @Svenum as an assignee to nix-related issues (TamtamHero#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (TamtamHero#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (TamtamHero#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (TamtamHero#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * Clarify behaviour on service stop or pause (TamtamHero#53) (TamtamHero#55) * Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of TamtamHero#50) (TamtamHero#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked) * add fw-ectool in module * fixing missing print for command execution (TamtamHero#63) --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]>

@Svenum

* Adding @Svenum as an assignee to nix-related issues (TamtamHero#43) * Fixing adding @Svenum as an assignee to nix-related issues (TamtamHero#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (TamtamHero#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (TamtamHero#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (TamtamHero#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * Clarify behaviour on service stop or pause (TamtamHero#53) (TamtamHero#55) * Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of TamtamHero#50) (TamtamHero#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked) * add fw-ectool in module * fixing missing print for command execution (TamtamHero#63) --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]>

@Svenum

* Adding @Svenum as an assignee to nix-related issues (#43) * Fixing adding @Svenum as an assignee to nix-related issues (#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * Clarify behaviour on service stop or pause (#53) (#55) * Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of #50) (#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked) * fixing missing print for command execution (#63) * forcing utf-8 encoding for socket messages and usage of stopServerSocket method instead of manual closing, as well as updating error detection pattern (#64) * README.md documentation update (#65) * forcing utf-8 encoding for socket messages and usage of stopServerSocket method instead of manual closing, as well as updating error detection pattern * README.md documentation update * change log format on fatal crash * fix badges links * adding windows platform badge and issue template * fix `:` instead of `=` * disabling part of the README.md while waiting for merge * Command arguments refactoring Pt.1 (#66) * forcing utf-8 encoding for socket messages and usage of stopServerSocket method instead of manual closing, as well as updating error detection pattern * README.md documentation update * change log format on fatal crash * fix badges links * adding windows platform badge and issue template * fix `:` instead of `=` * first part of the command argument refactoring. the old argument format is deprecated but still usable. improved feedback when executing commands. #31 * trim blank lines in README.md * finishing touches (#67) * add no battery mode for mainboards without battery (#69) * add configuration for no battery mode in hardware controller * fix wrong line getting noBatteryMode * dynamically fetching battery sensor on init/reload * add --no-battery flag for install * update readme with --no-battery flag * rework no battery config to come from service args * change sensors to be ectool specific - reword the argument to be more clear about battery sensors - move `noBatteryMode` and `nonBatterySensors` to EctoolHardwareController - update `getNonBatterySensors` to be able to handle more than one sensor - update installer and readme accordingly * update grep command for checking existing `--no-battery-sensors` * combine getTemperature functions to one * add documentation for run option `--no-battery-sensors` * rename variable `NO_BATTERY` to `NO_BATTERY_SENSOR` * update the installer to use existing placeholder format * rename noBatterySensorMode variables and functions for clarity * rename placeholder to `NO_BATTERY_SENSOR_OPTION` for clarity * update comments in installer to reflect new argument name * adding ectool sub-dependency to documentation (#70) * typo "tempurature" => "temperature" (#71) typo "tempurature" => "temperature" * typo "tempurature" => "temperature" (#72) typo "tempurature" => "temperature" * Add ToC + link to NixOS Documentation (#75) * add doc folder * update nix link * add toc * add link * add missin # * add doc * fix link * add new line under titles * add --no-sudo option (#76) * Add choice to print fan speed percentage (#78) * Add option to print current speed percentage * Update README.md * Update commands.md * Add print choice descriptions to help text * add missing no_sudo check (#79) * Add NixOS Flake (#26) * initial * update gitignore * update inputs * add fw-ectool dependencie * add module * fix tabs * fix package * fix typo * fix service * fix type * add options * fix service * fix build inputs * add Readme + add suspend script * remove unneeded }; * fix pkgs.writeShellScript * remvoe \ * try * add self * fix module * update package * fix package * use sleep script * add config options * fix typo * fix typo * add defaults * fix type * add prettyier * remove beautifyer * udpate readme * update installer script * add missing path * Update README.md Co-authored-by: Thomas Eizinger <[email protected]> * Update flake.nix Co-authored-by: Thomas Eizinger <[email protected]> * Update nix/module.nix Co-authored-by: Thomas Eizinger <[email protected]> * add descriptions * fix uninstall * update readme * add description * remove requiremetns.txt + add github actions * update action * rename workflow test * fix service * try * try * Update README.md * Update README.md * chagne flake description * fix suspend script * fix script * fix path * fix install.sh * fix --no-sudo * add --no-sudo to other scripts * fix check * add option check * add missing " * Rename nix action --------- Co-authored-by: Thomas Eizinger <[email protected]> * Update branch to main branch (#54) * Adding @Svenum as an assignee to nix-related issues (#43) * Fixing adding @Svenum as an assignee to nix-related issues (#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * use ectool form nixpkgs * update flake * remove old deps * remove duplicated pkgs --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]> * Update to main branch + switch to fw-ectool (#61) * Adding @Svenum as an assignee to nix-related issues (#43) * Fixing adding @Svenum as an assignee to nix-related issues (#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * Clarify behaviour on service stop or pause (#53) (#55) * Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of #50) (#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked) * add fw-ectool in module * fixing missing print for command execution (#63) --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]> * add doc + .gitignore * Add NixOS Flake (#26) * initial * update gitignore * update inputs * add fw-ectool dependencie * add module * fix tabs * fix package * fix typo * fix service * fix type * add options * fix service * fix build inputs * add Readme + add suspend script * remove unneeded }; * fix pkgs.writeShellScript * remvoe \ * try * add self * fix module * update package * fix package * use sleep script * add config options * fix typo * fix typo * add defaults * fix type * add prettyier * remove beautifyer * udpate readme * update installer script * add missing path * Update README.md Co-authored-by: Thomas Eizinger <[email protected]> * Update flake.nix Co-authored-by: Thomas Eizinger <[email protected]> * Update nix/module.nix Co-authored-by: Thomas Eizinger <[email protected]> * add descriptions * fix uninstall * update readme * add description * remove requiremetns.txt + add github actions * update action * rename workflow test * fix service * try * try * Update README.md * Update README.md * chagne flake description * fix suspend script * fix script * fix path * fix install.sh * fix --no-sudo * add --no-sudo to other scripts * fix check * add option check * add missing " * Rename nix action --------- Co-authored-by: Thomas Eizinger <[email protected]> * Update branch to main branch (#54) * Adding @Svenum as an assignee to nix-related issues (#43) * Fixing adding @Svenum as an assignee to nix-related issues (#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * use ectool form nixpkgs * update flake * remove old deps * remove duplicated pkgs --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]> * Update to main branch + switch to fw-ectool (#61) * Adding @Svenum as an assignee to nix-related issues (#43) * Fixing adding @Svenum as an assignee to nix-related issues (#44) (non contributors cannot be assigned to issues) * Reload will report if config couldn't be parsed and the service keeps running. (#46) Authored-by: Nina Alexandra Klama <[email protected]> * Removing binary blobs from the project (#51) * removing binary blobs from the project. we now fetch the ectool from the gitlab artifacts and confirm the checksum. * remove bin references from README.md * extracting $TEMP_FOLDER from installEctool * Fix README spelling/grammer, fix "FrameWork" capitalization in service description (#52) * Review README spelling/grammar * Fix "FrameWork" capitalization in service * Clarify behaviour on service stop or pause (#53) (#55) * Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of #50) (#58) * separating `FanController` into different subclasses to allow `HardwareController` and `SocketController` diversity * adding the new arguments into the README.md * fixing an indentation error causing `--strategy <strategy>` not to work (the simple `<strategy>` still worked) * add fw-ectool in module * fixing missing print for command execution (#63) --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]> --------- Co-authored-by: Léopold Hubert <[email protected]> Co-authored-by: Nina Alexandra Klama <[email protected]> Co-authored-by: DeflateAwning <[email protected]> Co-authored-by: Oli Thornton <[email protected]> Co-authored-by: Ryan <[email protected]> Co-authored-by: Thomas Eizinger <[email protected]>

leopoldhub added the enhancement New feature or request label Jun 19, 2024

leopoldhub self-assigned this Jun 19, 2024

leopoldhub requested a review from TamtamHero June 19, 2024 19:54

leopoldhub added this to the Windows support milestone Jun 19, 2024

TamtamHero approved these changes Jun 22, 2024

View reviewed changes

leopoldhub closed this Jul 6, 2024

leopoldhub force-pushed the dev branch from f7b9169 to 6fa5e64 Compare July 6, 2024 01:10

leopoldhub mentioned this pull request Jul 6, 2024

Separating FanController into different subclasses to allow HardwareController and SocketController diversity. 2 (Repost of #50) #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separating FanController into different subclasses to allow HardwareController and SocketController diversity. #50

Separating FanController into different subclasses to allow HardwareController and SocketController diversity. #50

leopoldhub commented Jun 19, 2024

leopoldhub commented Jun 19, 2024

TamtamHero commented Jun 20, 2024

leopoldhub commented Jun 20, 2024

TamtamHero commented Jun 21, 2024

leopoldhub commented Jun 21, 2024

leopoldhub commented Jun 24, 2024 •

edited

Loading

TamtamHero commented Jun 24, 2024 •

edited

Loading

leopoldhub commented Jun 24, 2024 •

edited

Loading

TamtamHero commented Jun 24, 2024

leopoldhub commented Jul 6, 2024 •

edited

Loading

DHowett commented Jul 6, 2024 •

edited

Loading

leopoldhub commented Jul 6, 2024

DHowett commented Jul 6, 2024

leopoldhub commented Jul 6, 2024

DHowett commented Jul 6, 2024

Separating FanController into different subclasses to allow HardwareController and SocketController diversity. #50

Separating FanController into different subclasses to allow HardwareController and SocketController diversity. #50

Conversation

leopoldhub commented Jun 19, 2024

leopoldhub commented Jun 19, 2024

TamtamHero commented Jun 20, 2024

leopoldhub commented Jun 20, 2024

TamtamHero commented Jun 21, 2024

leopoldhub commented Jun 21, 2024

leopoldhub commented Jun 24, 2024 • edited Loading

TamtamHero commented Jun 24, 2024 • edited Loading

leopoldhub commented Jun 24, 2024 • edited Loading

TamtamHero commented Jun 24, 2024

leopoldhub commented Jul 6, 2024 • edited Loading

DHowett commented Jul 6, 2024 • edited Loading

leopoldhub commented Jul 6, 2024

DHowett commented Jul 6, 2024

leopoldhub commented Jul 6, 2024

DHowett commented Jul 6, 2024

leopoldhub commented Jun 24, 2024 •

edited

Loading

TamtamHero commented Jun 24, 2024 •

edited

Loading

leopoldhub commented Jun 24, 2024 •

edited

Loading

leopoldhub commented Jul 6, 2024 •

edited

Loading

DHowett commented Jul 6, 2024 •

edited

Loading