AstraExt / astra-monitor

Resource Monitor for GNOME shell
GNU General Public License v3.0
281 stars 14 forks source link

Can't read data via lm-sensors under certain circumstances #133

Open goc9000 opened 3 months ago

goc9000 commented 3 months ago

Description

On my machine, the hwmon source produces a confusing, unusable mess, so I switched to the lm-sensors source. This worked great with Freon, and it allows one to add overrides in /etc/sensors.d to edit out badly configured or defective sensors.

However, Astra does not display any sensors at all from that source.

I did some digging and I think the problem is that Astra Monitor gets the data by executing sensors -j. I think the expectation was that a JSON output is easier to machine-parse than the regular human-readable version.

Unfortunately, the JSON output as read by Astra Monitor is not guaranteed to be valid. There are at least two bugs which come into play here:

First, if sensors fails to read some values (e.g. sensors are broken, device is turned off etc), it will emit messages on stderr. For instance, on my machine, the output of sensors -j begins like this:

{
   "iwlwifi_1-virtual-0":{
      "Adapter": "Virtual device",
      "temp1":{
ERROR: Can't get value of subfeature temp1_input: Can't read

      }
   },
(... many more lines omitted ...)

Note the glaring error message. It is emitted on stderr, but Utils.executeCommandAsync redirects stderr to stdout, so the message breaks the JSON structure.

Second, and this is a severe bug with sensors itself, even if I turn off the offending sensors in /etc/sensors.d, the output is now:

{
   "iwlwifi_1-virtual-0":{
      "Adapter": "Virtual device",
   },
(... many more lines omitted ...)

Note the trailing comma, invalid for standard JSON. This is because sensors has no real JSON support, it just pastes text lines and doesn't account for all edge cases.

It's unlikely that the bug will be fixed in sensors anytime soon, but here are some workarounds I recommend:

  1. Ensure stderr output is not mixed with stdout, at least for sensors
  2. Apply a regex before JSON.parse to try to get rid of trailing commas, e.g. stdoutString = stdoutString.replace(/,\s*(?=}|])/g, '');

Steps to Reproduce

  1. Ensure that sensors produces some output on your machine

  2. Disable all sensors under some minor category in lm-sensors by adding e.g. /etc/sensors.d/temp.conf with a content like:

    chip "chip-name-here"
           ignore temp1
           ignore temp2
           ... etc ...
  3. Run sensors again and note that it should now show a traling comma for that chip/adapter

  4. Switch to lm-sensors in the extension and observe how it doesn't detect any sensors at all anymore

Environment

Logs

Error seen in logs:

Jun 20 00:13:58 REDACTED gjs[1175960]: ###### Astra Monitor ###### Error getting sensors sources: SyntaxError: JSON.parse: expected double-quoted property name at line 4 column 4 of the JSON data

Sensors output with sensor error and stderr redirected to stdout:

sensors_output_with_error.txt

Sensors output with the offending sensor disabled:

sensors_output_with_ignore.txt

Additional Context

N/A

ljuzig commented 2 months ago

Thank you! I should create a debug build to better understand why hwmon source produces "a confusing, unusable mess" on your system. That should be the preferred source since it has a far better performance.

Utils.executeCommandAsync shouldn't redirect the stderr into stdout, I used to have the very same error on my system as yours and I've always been able to use lm-sensors until hwmon source has been introduced, the problem must be somewhere else. I'm gonna investigate further into that if I'm able to reproduce it.

The sensors bug with trailing commas is an interesting one, didn't know that. I'm gonna fix that.

goc9000 commented 2 months ago

Thanks for working on this.

Regarding hwmon, the issue is complicated. Reading the data technically works, but there are are several problems that would require a more thorough re-engineering for it to be useful:

ljuzig commented 2 months ago

Having a lot of sensors is currently considered an edge case. In every device I've tested, I've encountered a manageable number of sensors. Nonetheless, I would like to address edge cases as well, but please note that it is not at the top of the priority list right now. If you could provide a screenshot or something to help me understand your specific case, it would be very helpful.

goc9000 commented 1 week ago

I've split off the discussion about the hwmon display to a separate issue ( https://github.com/AstraExt/astra-monitor/issues/145 ).

Note that I've upgraded to the 26 EGOv43 version which should include the trailing commas fix but the lm-sensors source still doesn't work. There must be more to it.