Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dashboard): add CPU temperature #232

Merged
merged 22 commits into from
Jun 1, 2022
Merged

feat(dashboard): add CPU temperature #232

merged 22 commits into from
Jun 1, 2022

Conversation

ravenclaw900
Copy link
Owner

No description provided.

@ravenclaw900 ravenclaw900 added the enhancement New feature or request label May 8, 2022
@ravenclaw900 ravenclaw900 marked this pull request as ready for review May 10, 2022 23:20
@ravenclaw900
Copy link
Owner Author

@MichaIng, 2 things I don't really like with the current implementation.

  1. The graph always shows in Celsius. Do you think it's worth a config file setting to decide between Celsius and Fahrenheit? (or maybe a frontend setting)?
  2. The graph color. I'm actually running out of colors for the graph, do you have any ideas?

@MichaIng
Copy link
Collaborator

Probably a config file setting for °C vs °F is best, since we don't expect multiple people accessing one dashboard instance (do we?), so no need to have it client dependant.

Not sure about the colours, I think there are tools to generate a set of colour codes based on amount which are max different from each other.

@ravenclaw900
Copy link
Owner Author

ravenclaw900 commented May 21, 2022

Hmm... After (partially) implementing it, I realize that it will need to send the Celsius temperature anyway, to use it for the message calculation. Really, the only place it would actually be used is in the graph, so I think that a setting in the frontend actually makes more sense, as to not repeat code.

@ravenclaw900
Copy link
Owner Author

Actually, even better would probably be to send it as a global setting, so that it can't be changed on-the-fly and mess up the graph, and would still be client-independent.

@ravenclaw900
Copy link
Owner Author

@MichaIng, should work (with the setting) now, please review.

@ravenclaw900 ravenclaw900 enabled auto-merge (squash) May 28, 2022 02:37
@MichaIng
Copy link
Collaborator

MichaIng commented May 29, 2022

Works well. Few things I recognised while playing around with the graphs, but not directly related to this PR:

  • The % scale is still shown even when CPU usage is hidden. Temperature and MB scales are hidden when all related graphs are, as expected.
  • It will be a rare case, but probably the % scale could move to the left, when the MB scale is hidden?
  • MB and MiB are used inconsistently on graph and stats. I'm personally fans of using MiB/GiB/multi-byte units, as Mega/Giga is, to be precise, incorrect (power of 1000 vs power of 1024): https://en.wikipedia.org/wiki/Byte#Multiple-byte_units
  • Since the network values are usually very small, they are not visible (at x-axis) when disk or RAM graphs are shown. They are actually MiB/s, isn't it? Probably worth to give them an own scale? Related is that the MB scale is not adjusted to KB/KiB, so when only the network graphs are shown, the whole y-axis often shows 0 MB from bottom to top.
  • To quicker see graph and related scale, probably it would help to colour temperature and % scales the same way the related graphs are? That silver for temperature is not so different from white, probably a fresh colour palette with a more pregnant colour, like red or orange for temperature, would help as well then 😉.

But as said, those are basically own enhancements to the stats page. Shall I open a new issue with those, to be picked up once someone finds time?

MichaIng
MichaIng previously approved these changes May 29, 2022
@ravenclaw900
Copy link
Owner Author

ravenclaw900 commented May 30, 2022

  • That seems to be because I locked it to 0% - 100%. Since the change does stop the scale from disappearing, I think that it might be better to revert it.
  • That would require some sort of workaround, but it could be done.
  • I use the binary units (MiB, GiB) for RAM and swap, which seems to be the common usage for it. Everything else, from what I've seen, is measured with MB and GB. E.g.:
dietpi@rpi4:~ $ df -h
Filesystem      Size  Used Avail Use% Mounted on
/dev/root        30G  4.4G   24G  16% /
/dev/mmcblk0p1  127M   34M   93M  27% /boot
(snip)
  • I chose the MB scale because it was the most in the 'middle' of the other values (KB for network, GB for disk, MiB for RAM and swap). I think that having 2 scales with different information units might be slightly confusing, as it might look like it's sending 200GB instead of 200KB of data. Then again, network is its own thing, as it's per second instead of a constant measure. Adjusting the scale would also require a workaround of some sort, to detect when everything using the MB scale is off, and then flip the CPU scale.
  • You mean something like this? I could add that, it works well.
    Screen Shot 2022-05-30 at 11 09 09
    I chose the silver color to be more neutral, so it wouldn't be confused with any of the temperature colors. What I would really prefer is a way to have the color in the graph change based on the temperature value, to match the reading on the side, but that doesn't currently seem to be possible.

@MichaIng
Copy link
Collaborator

I've seen, is measured with MB and GB. E.g.:

Many scripts use the units inconsistently, either MB or just M while showing MiB. df indeed shows true power's of 1000 🤔. I like "MiB" since it is only unmistakable way to show the most commonly used power's of 1024 for any sizes in computer world.

having 2 scales with different information units might be slightly confusing

Indeed, at least it would need to be made more distinguishable by using colours. In theory the units could be adjusted depending on highest value, so that having only network graphs selected, e.g. KiB/KB is used and hence network activity better visible.

But yeah, it's nothing urgent IMO, just something to keep in mind when having time and touching those graphs code anyway.

@ravenclaw900 ravenclaw900 enabled auto-merge (squash) May 30, 2022 22:19
Copy link
Collaborator

@MichaIng MichaIng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works very well, great job 👍. Also in dark mode, the colours are good visible.

@ravenclaw900 ravenclaw900 merged commit 6567d10 into main Jun 1, 2022
@MichaIng MichaIng deleted the cpu-temp branch June 1, 2022 14:44
@MichaIng MichaIng linked an issue Aug 19, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] please add cpu temperature meter
2 participants