Command argument refactoring proposition

[leopoldhub]

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:

• direct : can be used in a command that will run the background service
• indirect : can be used in a command that will communicate with the background service

option	group	context	description
<strategy>	run & configure	direct & indirect	the name of the strategy to use
--run	run	direct	run the service
--config	run	direct	specify the configuration path
--no-log	run	direct	disable state logging
--list-strategies	configure	indirect	print the available strategies
--query, -q	configure	indirect	print the current strategy name
--reload, -r	configure	indirect	reload the configuration file
--pause	configure	indirect	temporarily disable the service
--resume	configure	indirect	resume the service

And here are the new ones:

| option | group | context | description |
|---------------------------------------------------------------|-----------------|----------|------------------------------------------|
| --run (?<strategy>) | run | direct | run the service |
| --config | run | direct | specify the configuration path |
| --silent | run | direct | disable state logging |
| --use <strategy> | | indirect | run the service |
| --actual-strategy,-a | | indirect | print the current strategy name |
| --strategy-detail,-d (?<strategy>) | | indirect | list a strategy configuration |
| --list-strategies,--ls,-l | | indirect | print the available strategies |
| --reload-config,--reload, -r | | indirect | reload the configuration file |
| --add-strategy,-A <strategy-name> | add | indirect | add a new strategy to the config |
| --configure-strategy,-C <strategy-name> | configure | indirect | configure a strategy |
| --update-frequency,--uf,-u <update-freq> | add & configure | indirect | set the strategy update frequency |
| --moving-average-interval,--mai,-m <moving-average-interval> | add & configure | indirect | set the strategy moving average interval |
| --curve-point,--cp,-c <temp>:<fan-duty> | add & configure | indirect | set a point in the curve |
| --delete-strategy,-D <strategy-name> | | indirect | delete a strategy |
| --pause | | indirect | temporarily disable the service |
| --resume | | indirect | resume the service |

Edit: After much thoughts, I think we should use subcommands when possible, as they are more intuitive.

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 configuration

reset : indirect : reset the current strategy to the default one

set-default : indirect : set the default strategy

argument	description
strategy	name of the strategy to use e.g: "lazy". (use print strategies to list available strategies)
--discharge, --d	is the default for discharging strategy

remove-default : indirect : remove the default strategy

argument	description
--discharge, --d	is the default for discharging strategy

add : indirect : add a new strategy

argument	description
strategy	name of the strategy to use e.g: "lazy". (use print strategies to list available strategies)
curve_point...	a point in the temperature curve in the format: temp:speed
--update-frequency, --uf, -u	time interval between every temperature update. (min: 1. default: 5)
--average-interval, --ai, -a	number of seconds on which the moving average of temperature is computed. (min: 0. default: 20)

configure : indirect : configure an existing strategy

argument	description
strategy	name of the strategy to use e.g: "lazy". (use print strategies to list available strategies)
curve_point...	a point in the temperature curve in the format: temp:speed. (optional)
--update-frequency, --uf, -u	time interval between every temperature update. (min: 1. default: 5)
--average-interval, --ai, -a	number of seconds on which the moving average of temperature is computed. (min: 0. default: 20)

delete : direct : delete an existing strategy

argument	description
strategy	name of the strategy to use e.g: "lazy". (use print strategies to list available strategies)

pause : direct : pause the service

resume : resume : pause the service

print : direct : print desired information

argument (OR)	description
current-strategy	print the current strategy
strategy-details strategy	print the details of the specified strategy
strategies	list the available 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?

[TamtamHero]

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:

• Non-volatile changes: config file
• Volatile changes: command line

Other typical arguments against change applies too:

• Don't fix what isn't broken
• Don't change UX/break compatibility if you don't have to

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.

[leopoldhub]

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 configuration

reset : indirect : reset the current strategy to the default one

pause : direct : pause the service

resume : resume : pause the service

print : direct : print desired information

argument (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.

[TamtamHero]

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)

[leopoldhub]

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.