-
-
Notifications
You must be signed in to change notification settings - Fork 332
fix: display actual EPP and EPB values in stats output #894
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
base: master
Are you sure you want to change the base?
fix: display actual EPP and EPB values in stats output #894
Conversation
Previously, the stats output was showing values from the config file instead of actual system values. This fixes the issue by: 1. Reading EPP directly from /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference 2. Reading EPB from /sys/devices/system/cpu/cpu0/power/energy_perf_bias and converting numeric values to string representations This ensures that the displayed values reflect the actual current system settings rather than the configured values.
|
I am still not sure, if this can be applied to all systems with different cpus and OS. I noticed that GTK version works good. It works by reading auto_cpufreq_stats_file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the current_epp and current_epb methods to read actual system values instead of configuration values. The key changes include:
- Changed both methods from reading config values to reading actual hardware values from sysfs
- Corrected the
epb_pathfrom the incorrect/sys/devices/system/cpu/intel_pstateto the proper/sys/devices/system/cpu/cpu0/power/energy_perf_bias - Added numeric-to-string conversion logic for EPB values (0→"performance", 4→"balance_performance", 6→"default", 8→"balance_power", 15→"power")
Comments suppressed due to low confidence (3)
auto_cpufreq/modules/system_info.py:147
- The
is_ac_pluggedparameter is no longer used in the method implementation. This parameter should be removed to avoid confusion and maintain a clean API, as the method now reads the actual system value rather than configuration-based values.
def current_epp(is_ac_plugged: bool) -> str | None:
auto_cpufreq/modules/system_info.py:155
- Except block directly handles BaseException.
except:
auto_cpufreq/modules/system_info.py:180
- Except block directly handles BaseException.
except:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except: | ||
| return None |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bare except clause catches all exceptions including system-exiting exceptions like KeyboardInterrupt and SystemExit. Replace with a specific exception type like except (FileNotFoundError, OSError, IOError) to handle expected file operation errors.
| except: | ||
| return None |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bare except clause catches all exceptions including system-exiting exceptions like KeyboardInterrupt and SystemExit. Replace with a specific exception type like except (FileNotFoundError, OSError, ValueError, IOError) to handle expected file operation and conversion errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm bit bothered by cpu0 in /sys/devices/system/cpu/cpu0/power/energy_perf_bias line. Would love it could somehow be made more generic, as in some weird situation someone doesn't have cpu0 (crazy, but not impossible). User will get an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm running on Intel and using pstate driver, but replacing epb_path = "/sys/devices/system/cpu/intel_pstate" doesn't seem to affect me.
Would also be nice if these changes would be made made in more generic way depending on which driver user is using.
| return None | ||
|
|
||
| @staticmethod | ||
| def current_epb(is_ac_plugged: bool) -> str | None: |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_ac_plugged parameter is no longer used in the method implementation. This parameter should be removed to avoid confusion and maintain a clean API, as the method now reads the actual system value rather than configuration-based values.
|
Thank you for PR, welcome to open source and let's work on getting these changes merged :) I left a couple of comments and I also asked on auto-cpufreq discord community for others to test these changes to make sure they work every architecture. Rest of things were raised by Copilot, nothing major but would be good if they were addressed. Also, to resolve any ambiguity:
Could you please tell me how do your stats output differ before and with this PR? As besides seeing |
Previously, the stats output was showing values from the config file instead of actual system values. This fixes the issue by:
This ensures that the displayed values reflect the actual current system settings rather than the configured values.
I am new in PR culture, so any suggestion are welcome.