LCDproc - Add CPU Temperature Screen. Feature: #10462 #1125
Conversation
|
Hi, works fine! Thanks a lot! |
| @@ -102,6 +102,27 @@ function get_uptime_stats() { | |||
| return($status); | |||
| } | |||
|
|
|||
| /* Returns CPU temperature if available from the system */ | |||
| function get_cpu_temperature() { | |||
There was a problem hiding this comment.
There are very similar function in the main repo under https://github.com/pfsense/pfsense/blob/master/src/usr/local/www/includes/functions.inc.php#L141 and https://github.com/pfsense/pfsense/blob/master/src/usr/local/www/widgets/include/thermal_sensors.inc#L33 -- It would be best to include (not copy) and use one of those functions instead of making a new one that does essentially the same task.
There was a problem hiding this comment.
Hi Jim, thank you for looking into this.
If I try and include those, such as functions.inc.php to use get_temp(), it won't allow it since there's similar named functions in lcdproc_client.php. First error: PHP ERROR: Type: 64, File: /usr/local/www/includes/functions.inc.php, Line: 84, Message: Cannot declare cpu_usage() (previously declared in /usr/local/pkg/lcdproc_client.php:69)
My PHP is rusty but I had a look and it seems to not be like Python where I can import specific functions. All or nothing if I'm not mistaken? There's also a lot of other same named functions in lcdproc_client.php that would clash.
So if this leaves me with my original code and how it's done here, I've also just about got a commit to switch between celsius and fahrenheit.
Cheers.
There was a problem hiding this comment.
It's likely that those functions also overlap and work in a similar manner. You can probably use the one from functions.inc or rename the lcdproc function -- ideally the lcdproc-specific functions should all be named something unique anyhow, perhaps stating with lcdproc_<name>. PHP namespaces are also possible but more complicated. It's best to avoid the conflict entirely and remove as much redundant code as possible.
There was a problem hiding this comment.
PHP namespaces looks very interesting. That seems like the ideal route if I'm not mistaken. While I do see it's not ideal to have the other redundant code, that's a bit of tech debt that eventually needs to be sorted. But at least I can start with what I implement using better practices.
So, before dinner a quick initial toe in the water:
namespace LcdProc; and require_once("includes/functions.inc.php"); in lcdproc_clients.php
That cleared up any function collisions and allowed me to use get_temp() in my get_cputemperature() function. That worked and returned what I expected.
Just some validation before I continue on. Is this the correct path with my initial idea above?
There was a problem hiding this comment.
It allows it to function but it ignores the underlying problem of similar functions duplicating code and effort. IMO, it's the wrong solution
There was a problem hiding this comment.
What would be the ideal solution and how should I go about it?
|
Ok, so back to the drawing board. The one that's tricky is cpu_usage() since it does something different to the Please let me know if that's in the right direction. The temperature code itself isn't final - I have another commit for that once this first part is resolved. Thank you. |
georou
force-pushed
the
lcdproc-cputemperature
branch
2 times, most recently
from
013feb1
to
897a081
Compare
|
Hi, Wondering on the progress for this. Was tested working previously when I last commit and commented. Looks like it's still working fine on 2.6.0, presently. Also open to suggestions to doing a more thorough test. I enabled all the screen options and checked they displayed correctly. Thank you. |
|
It doesn't look like the concerns I raised about the function names were addressed yet, they're still named generically ( |
|
Thank you for checking, Jim. I've prepended all the functions as mentioned. Thoughts? Also, I noticed while combing the code on lines 1247, 1260 and 1273 Cheers |
|
@jim-p Could I please get an update on your thoughts on the progress for this PR? I noticed you did a commit on the issue I raised in my last comment from May 2022, only last month - I'm assuming then, that my last comments haven't been viewed. From my last commit, everything appears to have the required prefixes. |
|
It looks OK at a glance but there are conflicts that need to be resolved now before it could be merged. Most likely it needs the version bumped higher since the package is already at revision 11 now, and would need to go to 12. As for the last comment about |
….inc.php functions
georou
force-pushed
the
lcdproc-cputemperature
branch
from
f72ac41
to
853f363
Compare
|
No worries, Jim. I've had a look over the code again and tested it on 2.6.0 and a 2.7.0 nightly - no errors popped up in my system log and seems to work well. A bit of guidance if you could please, Jim. To bring in the changes that happened since last year, I chose rebase. Was that the better way to update my local repo or should I have merged devel to my local repo instead? Reading about both options I though rebase sounds like the more logical answer in this instance. I'm not completely comfortable with my git work flow currently, please excuse my noobiness. |
|
Rebase is usually ideal. As long as the current code runs and looks like you expect, and it doesn't show any conflicts, it should be OK. |
|
Thanks, Jim. No worries. |
|
Other work I did on the package made most of the changes here unnecessary. I picked up just the CPU temp parts and added them into the package in version 0.11.2 which is available now. |
* Add background color option for Adafruit LCD backpack devices * Add screen for CPU temperature * Add screen for installed package count/update count * Other misc formatting/code fixes
Appreciate any input on implementing this feature. I'm not a programmer, so I've tried to follow other examples while learning. Specifically how the pfsense temperature widget works.
Cheers.