vmatare / thinkfan

The minimalist fan control program
GNU General Public License v3.0
534 stars 61 forks source link

/sys/class/hwmon/hwmonX is a directory #203

Closed Foosec closed 1 year ago

Foosec commented 1 year ago
fans:
  - tpacpi: /proc/acpi/ibm/fan

sensors:
  - hwmon: /sys/class/hwmon
    name: coretemp
    indicies: [1,2,3,4,5,6,7,8]

levels:
  - [1, 0,      40]
  - [3, 40,     60]
  - [4, 50,     60]
  - [5, 60,     70]
  - [6, 70,     75]
  - [7, 75,     85]
  - [127, 85, 100]

If im reading examples and docs right, this should find the driver with the name coretemp and use that, however:

foobar@foobars-thonkpad /s/c/hwmon> pwd
/sys/class/hwmon
foobar@foobars-thonkpad /s/c/hwmon> cat hwmon*/name
AC
acpitz
nvme
nvme
BAT0
pch_cannonlake
thinkpad
ucsi_source_psy_USBC000:001
coretemp
iwlwifi_1
ucsi_source_psy_USBC000:002
foobar@foobars-thonkpad /s/c/hwmon> sudo thinkfan

ERROR: Lost sensor read_temps_: Failed to read temperature(s) from /sys/class/hwmon/hwmon8: Is a directory
vmatare commented 1 year ago

Hi @Foosec, which thinkfan version are you using?

Foosec commented 1 year ago

Hi @vmatare, aur reports 1.3.1-1.

I also attempted to change the name to another one, and the hwmonX number changed, the error remained the same leading me to believe that the name search is functioning correctly.

I also attempted with:

ERROR: Lost sensor read_temps_: Failed to read temperature(s) from /sys/devices/platform/coretemp.0/hwmon/hwmon8: Is a directory
Foosec commented 1 year ago

Didn't notice help prints the version, OOPS!

Here you go

foobar@foobars-thonkpad /s/d/p/c/hwmon> thinkfan -h
thinkfan 1.3.1: A minimalist fan control program
vmatare commented 1 year ago

Ah thanks, could you do me a favor see if the current master version works for you? It's slated for the 2.0.0 release, and the search logic has undergone a significant refactoring there, so this bug may have been eliminated already.

Foosec commented 1 year ago

Ofcourse, i installed thinkfan-git from AUR i assume its the correct version, since the version number reports 2.0.0, if not please let me know, and i will pull and compile it manually.

foobar@foobars-thonkpad ~> sudo thinkfan

ERROR: read_temps_: Failed to read temperature(s) from /sys/class/hwmon/hwmon8: Is a directory
foobar@foobars-thonkpad ~ [1]> thinkfan -h
thinkfan 2.0.0: A minimalist fan control program
vmatare commented 1 year ago

Oh wow, I just noticed you have spelled indicies instead of indices. So that should be a quick fix for you, but it's a bug nonetheless because obviously that should result in a helpful error message ;-)

Now the question remains why @bhundven is seeing the same problem in #206 although he has not misspelled the word...

bhundven commented 1 year ago

Because I don't have the problem when indices is set. I have a problem when it is absent.

Foosec commented 1 year ago

Oh my, can't believe i missed that! Thank you for the spellcheck, it indeed seems to work now. Sorry! I won't close this issue, seeing as you might want to keep it open until that helpful error message gets implemented, for any others that can't spell :P

If you prefer to close it be my guest. Thanks again!

bhundven commented 1 year ago

Seems like maybe a "catch-all" in the yaml parser should have unmatched keys cause an error.

vmatare commented 1 year ago

@bhundven Right, sorry. OK, so my initial design idea was to make the indices mandatory when you don't specify a full path to a specific temp*_input file. But it was poorly implemented, in that it should of course yield a specific error message.

However, now that I think about it again, maybe making the indices mandatory is not very nice because your config is logically valid and thinkfan should certainly try its best to make sense of it, as long as it can be interpreted unambiguously.

vmatare commented 1 year ago

I think with the current state we're as close to the desired behavior as we're gonna get without deprecating old features. At least spelling errors will give a proper error message right now. Leaving the indices field out entirely will still yield a runtime error, but that is sort-of by design, because using a directory entry is syntactically valid, and the config parser isn't supposed to check semantics.