search

intel / thermal_daemon

Thermal daemon for IA
GNU General Public License v2.0
540 stars 117 forks source link

Make "Unsupported condition" message less confusing #386

Closed dradetsky closed 1 month ago

dradetsky commented 1 year ago

I started thermald & got the following error

thermald[1297]: Unsupported condition 58 (UKNKNOWN)

That sounded ominous, so I went looking for answers, and came across #282, in which the user was told that his specific unsupported condition error (which was different from mine) could be ignored, but not what that message even means. By looking at #343 and poking around the code a bit, my guess is something like this: thermald reads a list of conditions which can be reported by some event emitter hardware thing from somewhere in sysfs. It presumably does this to check that this emitter is capable of emitting all the conditions/events thermald needs to receive to work properly. However, since my computer is fairly new, it advertises some fancy new condition which thermald is not aware of and presumably does not actually need to monitor in order to do its job. So when I run thermald from the terminal, I see

[1675583700][ERR]Unsupported condition 58 (UKNKNOWN)
[1675583700][ERR]Unsupported conditions are present
[1675583700][INFO]Some conditions are not supported, so check if any condition set can be matched
[1675583700][INFO]Condition Set matched:9 target:4
[1675583700][INFO]Condition Set matched:9 target:4
[1675583700][INFO]adaptive engine reached end

Which, I think, means: "We found all the conditions that we need to see, and also some other condition we don't even know about, but that's fine, we'll just ignore it and it should still work." I THINK. Someone who actually knows how this code/hardware works should confirm/deny.

Assuming I'm right, or vaguely close to right, I was going to suggest that a line or two be added so that the user is less mystified. However, a better option (assuming I'm vaguely correct) is that "Unsupported condition" shouldn't actually be an error, but a warning. I mean, if we get an unsupported condition we will still fall back to trying to figure out if we have all the conditions we need to run, and even if this fails we still do something which we call find_aggressive_target() and describe as

Falling back to use configuration with the highest power

(which sounds like the kinda thing that ought to be logged as warn, not info, since it sounds like something the user hoped wouldn't happen, but i'm not 100% sure)

and afaict only if this find_aggressive_target() thing fails do we actually give up & go home. All in all, it doesn't really seem like an unsupported condition is an error. It's just something unusual, but which isn't a problem in itself unless our backup plans fail. Unless of course I've misunderstood everything.

Executive Summary/More Clear Call-To-Action

  1. Please make the "Unsupported condition" message less confusing in some way.
  2. Please explain what an "Unsupported condition" even is, and/or check what I wrote above & if I have understood it correctly.
  3. Consider making the "Unsupported condition" message be logged as a warning, not an error, assuming I'm correct about what's going on, or explain why that would be stupid & I should feel stupid (don't worry, I'm used to it).
spandruvada commented 1 month ago

Unsupported conditions is now information messages only: src/thd_gddv.cpp: thd_log_info("Unsupported condition %" PRIu64 " (%s)\n", condition.condition, cond_name); src/thd_gddv.cpp: thd_log_info("Unsupported conditions are present\n");

Some conditions defined for Windows OS can't be implemented. We don't know what they mean.