Closed senyc closed 3 months ago
Thank you for the PR!
This review is a bit more pedantic, since I want the config variables to behave consistent. I hope you understand :)
Some general points:
- Can you make the config variable relative to
/sys/class/drm/<card>/
? (Since there is already a selector for the specific card)- The config variable + documentation is missing from
data/config
Yeah for sure. I wasn't sure if that was how you wanted it done since it would be a little different than setting a CPU thermal zone (since for that we set a full path instead of a partial). To remove this difference between gpu/cpu's It might be worth changing things to only use full paths instead of setting the drmAmdCard
with extra configuration for the sub path.
For example we could have config options for
AmdGpuUtilizationPath
AmdGpuThermalPath
AmdGpuVramPath
Removing the need for setting an explicit DrmAmdCard
which is a little more work for the user, but a lot less confusing then adding arbitrary partial paths.
This would make things more inline with how things are set for the CPU. Let me know what you think, otherwise I can move forward with the points above (:
Hmm, good points. I've thought about it a little and I think I still prefer relative paths, though:
DrmAmdCard
or basically duplicating functionality in case AmdGpu*
are not set. I'm not particularly fond of either of these options.Yeah sounds good, using relative paths is good for now, I'll get this updated to use them. It might be worth at some point looking into using a dependency or library that handles these queries in the future, or looking at what other bars/widgets do to solve this, not that it is that big of a deal haha.
Last thing missing now is an entry in data/config
with documentation (Can be copy-pasted from module.nix
). Other than that, LGTM.
LGTM, thanks!
I noticed a bug where the
tempFile
was the entire path (instead of just the subpath) leading to some issues. So I fixed that and also added an option to set the entire path via the config since I'm not sure if the path will be the same for everyone.If the path:
device/hwmon/hwmon0/temp1_input
won't change across machines then the configuration addition probably isn't needed since you can configure the card.