search

TamtamHero / fw-fanctrl

A simple systemd service to better control Framework Laptop's fan(s)
BSD 3-Clause "New" or "Revised" License
178 stars 33 forks source link

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

Closed leopoldhub closed 1 month ago

leopoldhub commented 2 months ago

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

leopoldhub commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/TamtamHero/fw-fanctrl/issues/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 :+1:

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 ? :angel:

leopoldhub commented 2 months ago

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 commented 2 months ago

Sadly, this is kind of stuck: https://github.com/python/cpython/pull/14823

leopoldhub commented 1 month ago

@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 commented 1 month ago

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

https://github.com/TamtamHero/fw-fanctrl/commit/f7b91691f5aaae83d50cc32155686db421b81076

FWIW: it shows up here - image

leopoldhub commented 1 month ago

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

DHowett commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

Alas! Sorry that didn't work out.