Added GPU Temperature and utilization #3
Conversation
JeremiahPetersen
changed the title
|
Thank you so much for your PR! By the way, I based the SYSIG app into a major parts of a computer, please don't separate the section for minor section like temperature, I suggest to add that temp info to processor section and Graphics section and please minimize the use of modules as possible and lastly, run Sorry for my long and demanding request because I don't have any build workflow(just a linter 😢) for every possible OS to test. |
|
no worries at all, thank you for the feedback! I will work on these adjustments in the next 24 hours :) |
|
Okay I will review that asap, thanks again! |
|
I added GPU temp (updates every second) as a bullet in Graphics |
JeremiahPetersen
changed the title
JeremiahPetersen
changed the title
|
Ok, let me know if it's ready for review. Thanks! |
|
It is ready for review now. The gpu temp and utilization are working. The cpu temp is working, but for some windows / intel systems, it would need additional software (Open Hardware Monitor OHM) which would be adding too much and may be unnecessary. I cleaned up the code. Let me know what changes you would like. |
sysig.py
Outdated
| amd_manager = pyadl.ADLManager.getInstance() | ||
| devices = amd_manager.getDevices() | ||
| for device in devices: | ||
| temperature_data = device.getTemperature() |
There was a problem hiding this comment.
I think it's getCurrentTemperature(), please check their docs if correct
There was a problem hiding this comment.
I checked here - https://github.com/nicolargo/pyadl - and you are correct. I made the changes.
sysig.py
Outdated
| temperature_info = w.MSAcpi_ThermalZoneTemperature()[0] | ||
| return temperature_info.CurrentTemperature / 10.0 - 273.15 | ||
| except wmi.x_access_denied: | ||
| return "Access Denied (Run script as administrator)" |
There was a problem hiding this comment.
Is there any solution for this without using administrator access in Windows? All of the modules I used don't need administrator access.
There was a problem hiding this comment.
That was the problem I was running into, I researched the problem for a while and the solutions were to run wmi using admin access, or use additional software like OHM. As the graphics additions are working, I can scrap the cpu temp idea until I can figure out a better solution for Windows / Intel. I can remove the cpu temperature code for now if you would like to incorporate just the gpu additions.
There was a problem hiding this comment.
use additional software like OHM.
Please don't, This is just a simple app 😅
I can remove the cpu temperature code for now if you would like to incorporate just the gpu additions.
If it's okay with you, Please do. It's better to do GPU additions for now, and you can submit another PR if you have a solution for admin access.
There was a problem hiding this comment.
lol yea I like that it is a simple app. I was trying to implement an easy way to see cpu temp but it turned out to be a rabbit hole lol. I like keeping it simple. That sounds good. I will edit the code and send that in a moment
There was a problem hiding this comment.
Thanks! I will run the workflow and merge this later.
JeremiahPetersen
changed the title
|
This is good for review, I removed the cpu temp and will continue researching to see if it is possible to do in a simple way. If not, no worries. The gpu additions should all be working without issue now. Let me know if there are any other changes to make. Thanks for letting me contribute and providing clear direct feedback! |
|
Don't worry it's just errors for variable naming, length of line etc., I'll merge this later. thanks again! |
|
no worries! I was going through each of the errors and realizing that lol |
|
Thanks! |
|
You are welcome! Thank you! |
This pull request introduces the following changes:
Please review and let me know of any feedback or changes required.