Closed leopoldhub closed 1 week ago
Hmmm I'm not convinced this would be a positive change.
Currently, we have quite a small program and --help
gives all the info you need to use it. The args can be read and understood in seconds, since the feature set is minimal. Which is good, and expected for a program whose only job is to deal with fans and temperatures.
This would change with your proposal, with the addition of new niche arguments that won't be used by most people.
All the features added by these new commands can be achieved currently by editing manually the config file. I'd never use CLI to add a new curve (or to change anything that would affect the non-volatile state of the service, aka the config file), so I don't see how this could have a positive impact on user friendliness. I believe that opening a file and making some changes there before reloading a service is pretty typical and accepted.
Basically, I think we should respect these rules:
Other typical arguments against change applies too:
Also, adding these would increase the line count. I feel like it's a nice feature that the script is small, so people can have a look before giving it sudo rights. Anything increasing the size of the script must have serious arguments going for it.
Yes, I understand I plan to add (or create a separate project?) a simple GUI for fan management using this project, hence my suggestion for rule management, but using the config.json for permanent changes is fine too.
The current command line arguments have some problems too, eg. we can ask to reload, change strategy, query current strategy and print the strategy list at the same time.
Refactoring them using subcommands allows us to restrict the user to one and only one command at a time.
With this in mind, we could continue to support the old arguments (without generating help for them, forcing new users to use new ones) and add the new format.
run
: direct : run the service. (use reset
to reset to the default strategy)argument | description |
---|---|
strategy |
name of the strategy to use e.g: "lazy". (use print strategies to list available strategies). (optional) |
--config , -c |
config file path. (default: /etc/fw-fanctrl/config.json ) |
--silent , -s |
disable printing speed/temp status to stdout |
reload
: indirect : reload configurationreset
: indirect : reset the current strategy to the default onepause
: direct : pause the serviceresume
: resume : pause the serviceprint
: direct : print desired informationargument (OR) | description |
---|---|
current-strategy |
print the current strategy |
strategy-details strategy |
print the details of the specified strategy |
strategies |
list the available strategies |
as for the line count, I plan to separate the script into several well named files/modules to improve readability.
I see. It's a bit of a dilemma for me, on one hand I like that this project fits in a single script. On the other hand, you're there, willing to increase the feature set and to commit some good work, at the price of adding complexity and breaking a few things.
I'm a strong partisan of "better is the enemy of good", but also I don't want to discourage you.
So, all things measured, if you're wiling to work on this, please do.
The GUI might live in a different repo, but it would be better that it uses fw-fanctrl
under the hood rather than tweaking directly its config file (which means you can add back the arguments about adding/removing a curve)
I invited you on this repo btw, so as to give you collaborator permissions (let me know if you don't want them)
I will think a bit more about the file splitting but I keep thinking that separating them would make the project easier to maintain, we will see.
I will also do my best not to break current systems and keep previous arguments as a backup for a while when adding the new ones.
Thank you for your trust in me, feel free to revoke these permissions if you ever need/want to.
Hi @TamtamHero ,
I would like to refactor the existing command line arguments, complete them with missing essential ones and would appreciate your feedback.
Here are the current arguments (including those in PR #29):
contexts:
And here are the new ones:
Edit: After much thoughts, I think we should use subcommands when possible, as they are more intuitive.
run
: direct : run the service. (usereset
to reset to the default strategy)strategy
print strategies
to list available strategies). (optional)--config
,-c
/etc/fw-fanctrl/config.json
)--silent
,-s
reload
: indirect : reload configurationreset
: indirect : reset the current strategy to the default oneset-default
: indirect : set the default strategystrategy
print strategies
to list available strategies)--discharge
,--d
remove-default
: indirect : remove the default strategy--discharge
,--d
add
: indirect : add a new strategystrategy
print strategies
to list available strategies)curve_point
...temp:speed
--update-frequency
,--uf
,-u
--average-interval
,--ai
,-a
configure
: indirect : configure an existing strategystrategy
print strategies
to list available strategies)curve_point
...temp:speed
. (optional)--update-frequency
,--uf
,-u
--average-interval
,--ai
,-a
delete
: direct : delete an existing strategystrategy
print strategies
to list available strategies)pause
: direct : pause the serviceresume
: resume : pause the serviceprint
: direct : print desired informationcurrent-strategy
strategy-details strategy
strategies
The aim is to make them more intuitive and organised, and to achieve this many of them will have to be renamed/removed and users will have to change the way they use the actual commands.
What are your thoughts on this? Should we go for it or add a deprecation warning for now?