alexleigh / pve-mods

Proxmox VE modifications to add temperature sensors
157 stars 11 forks source link

NaN in Proxmox 8.0.4 #5

Open jordipalet opened 1 year ago

jordipalet commented 1 year ago

I applied manually all the patches and included all the sensors that I can get via sensors -j.

The "bar visual" works, however, instead of something like: CPU temp 39ºC (crit: 100ºC) I see: CPU temp NaN%

It looks like the calculation for the bar length size is done correctly, but not for the values display?

Or I'm missing something ?

Tks!

This is for example my CPU:

"coretemp-isa-0000":{ "Adapter": "ISA adapter", "Package id 0":{ "temp1_input": 42.000, "temp1_max": 100.000, "temp1_crit": 100.000, "temp1_crit_alarm": 0.000 },

which seems to be the default as per the patch files, so I didn't changed anything there.

so Nodes.pm is cputemp => { jsonpath => ['coretemp-isa-0000', 'Package id 0'], valkey => 'temp1_input', critkey => 'temp1_crit', },

and I also kept pvemanagerlib.js:

    {
        itemId: 'cputemp',
        iconCls: 'fa fa-fw fa-thermometer-half',
        title: gettext('CPU temp'),
        valueField: 'cputemp',
        maxField: 'cputemp',
        renderer: Proxmox.Utils.render_node_temp,
    },
alexleigh commented 1 year ago

Did you also apply the patch to proxmoxlib.js? If you did add the render_node_temp function to that file, but still getting this result, then most likely, something changed in how proxmox-widget-toolkit works in PVE 8. In PVE 7, Proxmox.Utils.render_node_temp refers to the function I added to proxmox-widget-toolkit which formats the temperature string. It's possible this has changed in PVE 8. I don't have a working PVE 8 node right now, but I will try to spin one up soon and test out to make these patches work with PVE 8.

jordipalet commented 1 year ago

Yes applied the 3 patches, just didn't tried the mobile one ... will do later.

So yes, I guess there is some minor change in PVE 8 ...

Tks!

jordipalet commented 1 year ago

The mobile didn't worked either, however, as this is a testing machine when I restarted it this morning, it seems the other 3 patches are working (I see greyed the "CPU temp 39ºC (crit: 100ºC)" for each core (I've done that for every sensor in sensor -j). So a restart was needed for some reason, not just the "systemctl restart pveproxy.service "

But even if the sensors are displayed correctly, it stays as "Loading" in that part of the Summary panel, not sure how I can debug where is the error (which yesterday was not there ...)

jordipalet commented 1 year ago

Mystery resolved, at least for the normal patch (the mobile one not working yet). It seems that V8 needs a reboot, also make sure that you don't have any empty line in the .js ...

microcrash commented 1 year ago

I'm running Proxmox 8.03 however I had a slightly different issue. After making the adjustments, nothing would load in the browser. I traced this down to the function and adjusted it as follows:

render_node_temp: function(record) {
    let currenttemp = record.used.toFixed(1);
    let maxtemp = record.total.toFixed(1);
    return `${currenttemp} °C (crit: ${maxtemp} °C)`;
},

Once I did that, all I had to do was "systemctl restart pveproxy.service" and everything worked. no reboot required.

alexleigh commented 1 year ago

I created a PVE 8.0.4 node and was able to apply the changes without any modifications. I created a new set of patch files based on the PVE 8.0.4 base files here: https://github.com/alexleigh/pve-mods/tree/main/v8.0.4-pwt4.0.6/patches

Could you try these new set of patches and see if they resolved your issues?

Something else I noticed is that, if any of the configured outputs in the JavaScript does not match an emitted dictionary in the Perl API, then the entire PVE manager display would be greyed out, but the working senosrs would still have a value. This may help you figure out which sensors are working and which aren't: the sensors which do display a value are working, while the sensors which show no value are causing the JavaScript to crash. In this case please double check the Nodes.pm code to make sure the configured JSON paths match exactly what is shown when running sensors -j.

jordipalet commented 1 year ago

for me seems to be working fine, however not yet in the mobile one. I can see CPU temp, then the title of the next value (in my case Core0 temp) but not the value. I tried removing all the cores, and leaving only nvmetemp, same result.

Tried with Safari and Chrome (in iOS).

alexleigh commented 1 year ago

That's strange. I checked the behavior on the mobile version and it actually has a safety check. If the property cannot be retrieved, it should just render a dash, instead of throwing an exception. It was actually the desktop version that was lacking this check, which is what causes the JavaScript to crash and a grey loading area to be displayed when a temperature property could not be read. I just updated the patch files to included this check on the desktop side. Now, on either the mobile page or the desktop page, if a temperature property could not be read, a dash is displayed, and should not interfere with the loading of the properties which could be read.

If you are still having issues on the desktop side, you can try updating to the latest patch for 8.0.4 and see which probes are displaying the dash. If your issue is on the mobile side only, and it is not just that some probes display as a dash, could you share a screenshot of what the mobile site looks like for you? Also, if you wouldn't mind, could you also upload the patched versions of the files on your system, and I will try to reproduce the error.

microcrash commented 1 year ago

I just wanted to give an update from my last post. I updated to 8.0.04 (from 8.0.03) and everything works and loads for me on my computer browser and mobile side. Did not have to make the modification like I did before.