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.

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

Documentation #458

Merged
merged 13 commits into from
Nov 27, 2022
Merged

Documentation #458

merged 13 commits into from
Nov 27, 2022

Conversation

abvee
Copy link
Contributor

@abvee abvee commented Nov 24, 2022

The documentation for auto-cpufreq was very confusing, sometimes with conflicting information. Hopefully, I've managed to clear things up a bit :D

README.md Outdated

Instead run `systemctl start auto-cpufreq` to start the service. Run `systemctl status auto-cpufreq` to see the status of service, and `systemctl enable auto-cpufreq` for service to persist running across reboots.
## Post Installation
After installation `auto-cpufreq` will be available as a binary and you can refer to [auto-cpufreq modes and options](https://github.com/AdnanHodzic/auto-cpufreq#auto-cpufreq-modes-and-options)for more information on how to run and configure `auto-cpufreq`.
Copy link
Owner

Choose a reason for hiding this comment

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

Please add space between (https://github.com/AdnanHodzic/auto-cpufreq#auto-cpufreq-modes-and-options) and for more, i.e: (https://github.com/AdnanHodzic/auto-cpufreq#auto-cpufreq-modes-and-options) for more

README.md Outdated
@@ -156,11 +157,48 @@ turbo = auto
```

## How to run auto-cpufreq

auto-cpufreq can be run by simply running the `auto-cpufreq` and following on screen instructions, i.e:
Running just `auto-cpufreq` like so:
Copy link
Owner

@AdnanHodzic AdnanHodzic Nov 24, 2022

Choose a reason for hiding this comment

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

Let's remove line 160 to 166, i.e:

Running just `auto-cpufreq` like so:

`sudo auto-cpufreq`

will give an error:

`auto-cpufreq: wrong invocation. try --help for help.`

No need to get into so much detail, and even if someone runs it, message is returned and it's self explanatory what they should do.

README.md Outdated

`auto-cpufreq: wrong invocation. try --help for help.`

auto-cpufreq should be run with with one of the following long options:
Copy link
Owner

Choose a reason for hiding this comment

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

auto-cpufreq should be run with with one of the following long options:

Let's just say:

auto-cpufreq should be run with with one of the following options:

Copy link
Owner

Choose a reason for hiding this comment

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

I like that you created this new section, let's add a line here for full description of each option please to auto-cpufreq modes and options section.

README.md Outdated
After daemon is installed, `auto-cpufreq` is available as a binary and is running in the background. Its stats can be viewed by running: `auto-cpufreq --stats`
After the daemon is installed, `auto-cpufreq` is available as a binary and is running in the background. Its stats can be viewed by running: `auto-cpufreq --stats`

Alternatively, using just systemd:
Copy link
Owner

@AdnanHodzic AdnanHodzic Nov 24, 2022

Choose a reason for hiding this comment

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

Instead of saying "Alternatively, using just systemd" and lines 226 to 232 let's rephrase this or completely remove these lines not to encourage users to do this, as it can only lead to user breaking stuff.

Since sudo auto-cpufreq --install will enable (systemctl enable auto-cpufreq) and start (systemctl start auto-cpufreq) newly created auto-cpufreq systemd service automatically ...

README.md Outdated
@@ -197,6 +245,14 @@ auto-cpufreq daemon and its systemd service, along with all its persistent chang

`sudo auto-cpufreq --remove`

You can also stop the service with:
Copy link
Owner

@AdnanHodzic AdnanHodzic Nov 24, 2022

Choose a reason for hiding this comment

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

Just as in section above with install same goes for 248 to 255, sudo auto-cpufreq --remove and/or sudo ./auto-cpufreq-installer --remove will do this for you automatically. If done manually it can only break things, so let's leave these lines out or rephrase that sudo auto-cpufreq --remove does this for you automatically.

@AdnanHodzic
Copy link
Owner

Great stuff, I've left a few comments, let's go over them before this is merged.

@abvee
Copy link
Contributor Author

abvee commented Nov 25, 2022

Great stuff, I've left a few comments, let's go over them before this is merged.

Thanks for the recommendations, I have gone through them and committed them. I also took the liberty of adding an index and fixing some broken links.

Let me know if there is anything to add or remove :)

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Nov 26, 2022

This looks perfect. One last minor change, I have a feeling people who didn't use auto-cpufreq-installer to install auto-cpufreq might be missing some context and want to avoid unneeded questions/confusion.

So let's please rephrase following section something like this:

...
Please Note:
The power_helper.py script is located at auto_cpufreq/power_helper.py. In order to have access to it you first need to clone the repo.
...

@abvee
Copy link
Contributor Author

abvee commented Nov 27, 2022

done. Sorry for taking some time, I had some homework to finish yesterday :D.

Feel free to give any other suggestions :D

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Nov 27, 2022

done. Sorry for taking some time, I had some homework to finish yesterday :D.

Feel free to give any other suggestions :D

No worries and thank your contribution!

Merging, I'll make sure you're credited for your work as part of upcoming 1.9.7 release as soon as regression with #460 is fixed followed by your #459 PR merge.

@AdnanHodzic AdnanHodzic merged commit 70c58fb into AdnanHodzic:master Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants