Add LCDPROC NTP Screen #1278
Add LCDPROC NTP Screen #1278
Conversation
| // When local GPS is used. Show the IP of the source and the stratum. Also append how many satellites are in use or in view | ||
| $gps_sat_count = lcdproc_get_ntp_gps_sat_count(); | ||
| $lcd_cmds[] = "widget_set {$name} time_st_wdgt 1 1 {$lcdpanel_width} 2 h 4 \"NTP: {$ntp_hms} Strtm: {$peerinfo[2]}\""; | ||
| $lcd_cmds[] = "widget_set {$name} ref_wdgt 1 2 {$lcdpanel_width} 2 h 4 \"Ref: {$peerinfo[1]} " . (($gps_sat_count == false) ? "" : "{$gps_sat_count}") . "\""; |
There was a problem hiding this comment.
This is a bit confusing but it may be the comment that is wrong and not the code.
The comment above says it should be showing the IP address but that would be in $peerinfo[0]. As-is here it would just be printing .GPS. due to the if test, which seems correct to me. When the reference is a local GPS, the "IP address" is bunk, just an internal signal to ntpd that it should be using the local GPS.
It seems like in the case of a local GPS, though, printing .GPS. like you are already, or maybe printing a string saying "Local GPS Device", is best.
tl;dr: If the code is doing what you want, adjust the comment to match.
There was a problem hiding this comment.
I'll update the comment.
I was trying to avoid a long description when on a local GPS to leave enough room for the number of satellites in-view or in-use (depending on which NMEA messages are transmitted) and prevent row scrolling.
P.S. I think I finally figured out I can submit updates without having to generate a new pull request.
There was a problem hiding this comment.
Submitted updated lcdproc_client.php
| } else { | ||
| // Show IP of the source and the stratum | ||
| $lcd_cmds[] = "widget_set {$name} time_st_wdgt 1 1 {$lcdpanel_width} 2 h 4 \"NTP: {$ntp_hms} Strtm: {$peerinfo[2]}\""; | ||
| $lcd_cmds[] = "widget_set {$name} ref_wdgt 1 2 {$lcdpanel_width} 2 h 4 \"Ref: {$peerinfo[1]}\""; |
There was a problem hiding this comment.
There is a similar question here as above but this one does seem like it should be $peerinfo[0] so it prints the IP address. Right now when testing it's showing me things like .PPS. and .XMIS. and not the IP address of the active peer.
There was a problem hiding this comment.
I think you are right. Now that I think about it, my recollection is the server column is the actual IP address of the polled NTP server, but the reference column is the source the polled NTP server is actually using as its reference, i.e. next higher stratum level. I think I would have add another elseif test for a Stratum 0 .PPS. source to display $peerinfo[1], then the switch the last else to use $peerinfo[0] for everything else. Maybe change the abbreviation from Ref: to Src:
I was also considering unpacking the rest of the peerinfo[n] variables to make it more readable.
There was a problem hiding this comment.
You are correct, the "ref" column that has .GPS. will show the reference clock from which a source derives its time. For servers that are on higher strata they would show the IP address of whichever server they are peered with for time data. So it could have an IP address there but it's not the one most users would want to see in that case.
There was a problem hiding this comment.
Submitted updated lcdproc_client.php
|
Submitted updated lcdproc_client.php for requested changes. 
|
There was a problem hiding this comment.
Functionally it looks good, there is just one cosmetic (in the code) issue to take care of.
Also if possible it would be ideal to squash the commits so it appears as a single change if you can, so it's easier to get the changes into all the appropriate branches for release. If you can't, that's OK, I can merge it manually as a single change but it's a little trickier.
| if ($ntpq_counter == 0) { | ||
| $lcd_cmds[] = "widget_set {$name} time_st_wdgt 1 1 {$lcdpanel_width} 2 h 4 \"NTP: No active peers\""; | ||
| $lcd_cmds[] = "widget_set {$name} ref_wdgt 1 2 {$lcdpanel_width} 2 h 4 \"\""; | ||
| $lcd_cmds[] = "widget_set {$name} text_wdgt 1 3 {$lcdpanel_width} 2 h 4 \"\""; | ||
| $lcd_cmds[] = "widget_set {$name} stats_wdgt 1 4 {$lcdpanel_width} 2 h 4 \"\""; |
There was a problem hiding this comment.
The indentation here seems wrong, it should be one level deeper. Otherwise it doesn't line up properly with the braces above/below
|
I submitted a new PR with a single commit for each file, not sure if that makes it easier to merge. Let me know if I still did something incorrectly. Not sure where to find squash and merge on web interface. |
@jim-p @georou
More condensed NTP Screen
https://redmine.pfsense.org/issues/14625