sardemff7
commented
4 years ago Hi,
A tip for next time: when you make an MR with a new feature like that, you should link to the relevant documentation explaining said feature. :-) After reading this, I agree the plugin should use those values. However, I think we should use them better.
For example, when both crit and max are present, along with hysteresis values, I think the section state should be J4STATUS_STATE_AVERAGE when lowering into the hysteresis range. (Basically, while the cooling system is still on.)
And it’s starting to feel like 3 states is not enough, we’d need at least 5… Or maybe just a dynamic range of colour.
I’ll need to do some ground work on this one.
Also, we currently never set J4STATUS_STATE_BAD if max is missing, even if temperature is above crit, which could be confusing (the bar pops up as urgent, but nothing bad displayed).
So I guess your patch is good for now, as a temporary-that-may-stick-around-quite-some-time fix :-) (It may actually be the best in your specific case, I just didn’t check all the present/missing combinations here.)
Could you please update the commit summary to something a bit shorter (and with an initial cap), e.g. “sensors: Use crit_hyst as a fallback for max temp“, the commit message is good.
Tomsod
Hello again. I'd like to propose a tiny hack. My GPU temperature sensor has no defined 'high' temperature, but does have a 'crit hysteresis' temperature, so I patched my build of sensors plugin to treat it like 'high', displaying the readings in red. It makes sense as it's also a significant boundary (although mostly only when the GPU cools down after overheating). Do you think it's enough of a good idea to merge it?