Skip to content

fix: Autocpu-freq script not detecting battery on laptops #877

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
Merged
merged 2 commits into from
Nov 8, 2024
Merged

fix: Autocpu-freq script not detecting battery on laptops #877

merged 2 commits into from
Nov 8, 2024

Conversation

infstate
Copy link
Contributor

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Hotfix
  • Security patch
  • UI/UX improvement

Description

  • Added better checking when searching /sys/class/power_supply/ directory
  • Uses ls with * wildcard to find all battery entries not limited to BAT0. (e.g. BAT1, BAT2)

Testing

I have tested this on my Arch Linux install on DESKTOP. (no battery)
However it should work.

Impact

All this does is if there is any battery it will run the function for laptops and not desktops.
It fixes the issue of not detecting a laptop and instead runs the power profile meant for desktops.

Issues / other PRs related

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@infstate infstate mentioned this pull request Oct 26, 2024
Closed
4 tasks
ghost
ghost suggested changes Oct 26, 2024
View reviewed changes
@adamperkowski
remove an unneccessary comment
0873715 
Co-authored-by: nyx <nnyyxxxx@protonmail.com>
adamperkowski
adamperkowski approved these changes Oct 26, 2024
View reviewed changes
Copy link
Collaborator

@adamperkowski adamperkowski left a comment

Choose a reason for hiding this comment

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

Thanks

@infstate
Copy link
Contributor Author

Alright, thanks for the suggestions, I have removed the comment.

jeevithakannan2
jeevithakannan2 suggested changes Oct 27, 2024
View reviewed changes
Copy link
Contributor

@jeevithakannan2 jeevithakannan2 left a comment

Choose a reason for hiding this comment

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

While this solves the laptop issue. This doesn't solve the error that occurs after installation.

Add these lines after installation steps

sudo auto-cpufreq --install

@ghost
Copy link

ghost commented Oct 27, 2024

While this solves the laptop issue. This doesn't solve the error that occurs after installation.

Add these lines after installation steps

sudo auto-cpufreq --install

that is not related to this pull request, and it should be used with $ESCALATION_TOOL not sudo

@jeevithakannan2
Copy link
Contributor

While this solves the laptop issue. This doesn't solve the error that occurs after installation.
Add these lines after installation steps
sudo auto-cpufreq --install

that is not related to this pull request, and it should be used with $ESCALATION_TOOL not sudo

The issue mentions two issues and this PR only resolves only one issue.

And I just copy pasted the sudo auto-cpufreq --install from the err log

@ghost
Copy link

ghost commented Oct 27, 2024

While this solves the laptop issue. This doesn't solve the error that occurs after installation.
Add these lines after installation steps
sudo auto-cpufreq --install

that is not related to this pull request, and it should be used with $ESCALATION_TOOL not sudo

The issue mentions two issues and this PR only resolves only one issue.

And I just copy pasted the sudo auto-cpufreq --install from the err log

again, sudo auto-cpufreq --install is not related to this pull request, please re read the PR title and description

@jeevithakannan2
Copy link
Contributor

While this solves the laptop issue. This doesn't solve the error that occurs after installation.
Add these lines after installation steps
sudo auto-cpufreq --install

that is not related to this pull request, and it should be used with $ESCALATION_TOOL not sudo

The issue mentions two issues and this PR only resolves only one issue.
And I just copy pasted the sudo auto-cpufreq --install from the err log

again, sudo auto-cpufreq --install is not related to this pull request, please re read the PR title and description

Then it does not resolve that issue completely. Please re-read the issue !!

@ghost
Copy link

ghost commented Oct 27, 2024

While this solves the laptop issue. This doesn't solve the error that occurs after installation.
Add these lines after installation steps
sudo auto-cpufreq --install

that is not related to this pull request, and it should be used with $ESCALATION_TOOL not sudo

The issue mentions two issues and this PR only resolves only one issue.
And I just copy pasted the sudo auto-cpufreq --install from the err log

again, sudo auto-cpufreq --install is not related to this pull request, please re read the PR title and description

Then it does not resolve that issue completely. Please re-read the issue !!

ok? and is that my problem? why should i read the issue, once again sudo auto-cpufreq --install is not related to this pull request, please re read the pr title and description one more time !!

@ghost
Copy link

ghost commented Oct 27, 2024 •
edited by ghost
Loading

issues and pull requests should be focused on 1 bug, or 1 implementation, they should not include multiple bugs or multiple implementations, if you want you can split your issue into 2 since that would be better

@jeevithakannan2
Copy link
Contributor

jeevithakannan2 commented Oct 27, 2024 •
edited
Loading

issues and pull requests should be focused on 1 bug, or 1 implementation, they should not include multiple bugs or multiple implementations, if you want you can split your issue into 2 since that would be better

I am getting what you're trying to say but this is not a big change. Not going to waste time on two separate PRs and issues while it can be easily fixed by adding a line in single PR and change PR title to auto cpu-freq fixes.

@ghost
Copy link

ghost commented Oct 27, 2024

issues and pull requests should be focused on 1 bug, or 1 implementation, they should not include multiple bugs or multiple implementations, if you want you can split your issue into 2 since that would be better

I am getting what you're trying to say but this is not a big change. Not going to waste time on two separate PRs and issues while it can be easily fixed by adding a line in single PR.

please re read the message you're responding to !!, that second issue is not related to this PR at all and should not be included

@jeevithakannan2
Copy link
Contributor

issues and pull requests should be focused on 1 bug, or 1 implementation, they should not include multiple bugs or multiple implementations, if you want you can split your issue into 2 since that would be better

I am getting what you're trying to say but this is not a big change. Not going to waste time on two separate PRs and issues while it can be easily fixed by adding a line in single PR.

please re read the message you're responding to !!, that second issue is not related to this PR at all and should not be included

Waste of time and resources for a simple fix.

@ghost
Copy link

ghost commented Oct 27, 2024

issues and pull requests should be focused on 1 bug, or 1 implementation, they should not include multiple bugs or multiple implementations, if you want you can split your issue into 2 since that would be better

I am getting what you're trying to say but this is not a big change. Not going to waste time on two separate PRs and issues while it can be easily fixed by adding a line in single PR.

please re read the message you're responding to !!, that second issue is not related to this PR at all and should not be included

Waste of time and resources for a simple fix.

re read the reply chain one more time !! maybe you'll understand the issue we're arguing about

ghost
ghost approved these changes Oct 27, 2024
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

awesome

@infstate
Copy link
Contributor Author

What in tarnation is going on here, doesn't it run the autocpu-freq --install on line 37 of the install function?

@jeevithakannan2
Copy link
Contributor

What in tarnation is going on here, doesn't it run the autocpu-freq --install on line 37 of the install function?

Looks like I was hallucinating. When I took the SS that line was not called. See the SS in the issue.

@ghost
Copy link

ghost commented Oct 27, 2024

looks like theres an extra space separating escalation tool on that line

@jeevithakannan2
Copy link
Contributor

looks like theres an extra space separating escalation tool on that line

Yeah but that shouldn't cause issues. I cannot reproduce the issue again.

@infstate
Copy link
Contributor Author

If you want I can remove the extra space...

@adamperkowski adamperkowski added the bug Something isn't working label Oct 27, 2024
@adamperkowski
Copy link
Collaborator

adamperkowski commented Oct 27, 2024 •
edited
Loading

damn what happened here
thats a lot of words

too heated IMO
image

Repository owner locked as too heated and limited conversation to collaborators Oct 27, 2024
@ChrisTitusTech ChrisTitusTech merged commit 95cfbe8 into ChrisTitusTech:main Nov 8, 2024
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ChrisTitusTech ChrisTitusTech ChrisTitusTech approved these changes

@adamperkowski adamperkowski adamperkowski approved these changes

+2 more reviewers

@jeevithakannan2 jeevithakannan2 jeevithakannan2 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auto cpu-freq fails to detect laptop
4 participants
@infstate @jeevithakannan2 @adamperkowski @ChrisTitusTech