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.

Sign up for GitHub

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

Jump to bottom

Windows support #57

Open
wants to merge 44 commits into
base: packaging/windows
Choose a base branch
from
Open

Windows support #57

wants to merge 44 commits into from

Conversation

leopoldhub
Copy link
Collaborator

No description provided.

@leopoldhub leopoldhub added the enhancement New feature or request label Jul 6, 2024
@leopoldhub leopoldhub added this to the Windows support milestone Jul 6, 2024
@leopoldhub leopoldhub requested a review from TamtamHero July 6, 2024 01:51
@leopoldhub leopoldhub self-assigned this Jul 6, 2024
@leopoldhub
Copy link
Collaborator Author

At this stage, we have a working installer/uninstaller and the service running as expected 🎉

The only blocking parts are the sockets, so we cannot interact with the script yet.

Also, I now understand why nobody wants to port anything to windows... every single one of their provided tools sucks...

The install script can certainly be improved in many ways, but I will stop here for now and focus on the socket part.

If you have any questions or comments, please let me know 👍

@leopoldhub
Copy link
Collaborator Author

leopoldhub commented Jul 6, 2024 •
edited
Loading

Also an important part

The crosec driver being unsigned and the secure boot having to be disabled, causes windows to ask for the bitlocker key on boot if it is enabled.
(see https://github.com/DHowett/FrameworkWindowsUtils/releases for more details)
This can lock inexperienced users out of their computer if done incorrectly.

To mitigate this, I have added an acknowledgement part with a clear red warning.

I don't know if this is the right way to do things, so I would like to know your thoughts.

@leopoldhub leopoldhub linked an issue Jul 6, 2024 that may be closed by this pull request
ectool and windows support #30
Open
@TamtamHero
Copy link
Owner

TamtamHero commented Jul 7, 2024 •
edited
Loading

Ahem... LGTM ?
You're kind of on your own for Windows related stuff... not only do I lack a Windows install, but I also lack knowledge about how things are supposed to be there 😬
I've been Linuxing ever since I got into software engineering, sorry 😁
I think you should go on with it. People will certainly come and open an issue if there's something to improve.

I don't know if this is the right way to do things, so I would like to know your thoughts.

That's the way our beloved @DHowett does, and since the dude works at M$ I guess that's the way ✨

@leopoldhub
Copy link
Collaborator Author

leopoldhub commented Aug 3, 2024 •
edited
Loading

Hi @TamtamHero,

I have found some time and motivation to continue this and hope to finish by the end of next week.

However, I have a question, should we:

  1. Merge the Windows related stuff (install script, socket controller, services...) into the main branch?
    This would be easier for us to maintain, but would add a lot of unnecessary complexity and files that are not relevant to users of each OS (Windows stuff in Linux and vice versa).
  2. Create a separate branch, like we do with packaging solutions like NixOs (e.g. /windows, /packaging/windows...) and get rid of linux stuffs in it?
    This would make things much cleaner, but maintaining a new separate branch and keeping it up to date can be time consuming in the long run.
  3. Something else?

What do you think?
Which one do you think we should go for?

Have a nice day.

@leopoldhub leopoldhub linked an issue Aug 3, 2024 that may be closed by this pull request
fanctrl.py for windows [FEATURE] #60
Open
@leopoldhub leopoldhub mentioned this pull request Aug 4, 2024
Open
@leopoldhub
Copy link
Collaborator Author

Hi @TamtamHero,

After much thought, I have come to the conclusion that we should go with a separate branch and exclude Unix specific scripts, services and Socket Controllers.

Do you share this point of view?

I am currently working on the documentation and it should be ready for merging this weekend.

Have a great day.

@leopoldhub leopoldhub marked this pull request as ready for review August 9, 2024 20:19
leopoldhub
leopoldhub commented Aug 9, 2024
View reviewed changes
Copy link
Collaborator Author

@leopoldhub leopoldhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good for me,

If you have anything you want to change, please let me know ^^

@leopoldhub leopoldhub changed the base branch from main to packaging/windows August 9, 2024 23:52
leopoldhub and others added 16 commits August 13, 2024 20:11
@leopoldhub
Command arguments refactoring Pt.1 (TamtamHero#66)
871566f 
* 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.

TamtamHero#31

* trim blank lines in README.md
@leopoldhub
Merge remote-tracking branch 'refs/remotes/real/main' into dev-windows
64d62b4 
# Conflicts:
#	README.md
#	fanctrl.py
#	services/fw-fanctrl.service
#	services/system-sleep/fw-fanctrl-suspend
@leopoldhub
finishing touches (TamtamHero#67)
b4f87b6
@leopoldhub
Merge remote-tracking branch 'refs/remotes/real/main' into dev-windows
9b63ded
@leopoldhub
adding back stacktrace on fatal fail
00cfec4
@leopoldhub
removing unused dependencies
1196999
@leopoldhub
update windows "one-line" download command
3b1779d
@leopoldhub
windows batch are a bit capricious...
e4d6441 
changing indentation to fix various weird behaviours
@leopoldhub
small fixes after merge
7f17443
@saizo80
add no battery mode for mainboards without battery (TamtamHero#69)
80a239d 
* 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
@leopoldhub
Merge remote-tracking branch 'refs/remotes/real/main' into dev-windows
da6e8d2 
# Conflicts:
#	README.md
#	install.sh
#	services/fw-fanctrl.service
@leopoldhub
adding ectool sub-dependency to documentation (TamtamHero#70)
cc65f26
@leopoldhub
typo "tempurature" => "temperature" (TamtamHero#71)
88590e3 
typo "tempurature" => "temperature"
@leopoldhub
typo "tempurature" => "temperature" (TamtamHero#72)
6d2d3de 
typo "tempurature" => "temperature"
@leopoldhub
add "/no-battery-sensors" installation argument
6681c6b
@leopoldhub
Merge remote-tracking branch 'refs/remotes/real/main' into dev-windows
c8223fd 
# Conflicts:
#	README.md
@leopoldhub
Copy link
Collaborator Author

Up to date and (hopefully) ready to merge.

I will wait for a week and test it thoroughly, and if all is well and no one objects, I will push it to the packaging/windows branch.

Have a nice day.

leopoldhub and others added 7 commits August 22, 2024 01:31
@leopoldhub
adding service pause on sleep and resume on wake
a961cfa
@leopoldhub
adding service pause on sleep and resume on wake
23d53ed
@leopoldhub
Merge remote-tracking branch 'origin/dev-windows' into dev-windows
db6fb77 
# Conflicts:
#	install.bat
@Svenum
Add ToC + link to NixOS Documentation (TamtamHero#75)
ec9a9ae 
* add doc folder

* update nix link

* add toc

* add link

* add missin #

* add doc

* fix link

* add new line under titles
@Svenum
add --no-sudo option (TamtamHero#76)
fe98b44
@Speenah
Add choice to print fan speed percentage (TamtamHero#78)
4a8a3a0 
* Add option to print current speed percentage

* Update README.md

* Update commands.md

* Add print choice descriptions to help text
@Svenum
add missing no_sudo check (TamtamHero#79)
2a9828f
@TamtamHero
Copy link
Owner

It's indeed going to be a bit more work to maintain a separate Windows branch but I think it's worth it too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TamtamHero TamtamHero Awaiting requested review from TamtamHero

Assignees

@leopoldhub leopoldhub

Labels
enhancement New feature or request
Projects
None yet
Milestone
Windows support
Development

Successfully merging this pull request may close these issues.

fanctrl.py for windows [FEATURE] ectool and windows support
5 participants
@leopoldhub @TamtamHero @saizo80 @Svenum @Speenah