Re: [PATCH] graphs.pl: Make canvas fixed size and limit cpufreq color index

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]

Hello,

Reviewed-by: Michael Tremer <michael.tremer(a)ipfire.org>

> On 13 Mar 2023, at 07:35, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> This fixes an issue where systems with many CPU cores
> run out of usable graph colors and canvas area.
> It also unifies the canvas size for all graphs.
> 
> Discussion: https://community.ipfire.org/t/cpu-freq-graph-not-working-with-i5-1235u/9396
> 
> Fixes: #12890
> 
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
> config/cfgroot/graphs.pl | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/config/cfgroot/graphs.pl b/config/cfgroot/graphs.pl
> index 3368e5aad..cefe0b90d 100644
> --- a/config/cfgroot/graphs.pl
> +++ b/config/cfgroot/graphs.pl
> @@ -30,8 +30,11 @@ require '/var/ipfire/general-functions.pl';
> require "${General::swroot}/lang.pl";
> require "${General::swroot}/header.pl";
> 
> -# Graph image size in pixel
> -our %image_size = ('width' => 910, 'height' => 300);
> +# Approximate size of the final graph image including canvas and labeling (in pixels, mainly used for placeholders)
> +our %image_size = ('width' => 900, 'height' => 300);
> +
> +# Size of the actual data area within the image, without labeling (in pixels)
> +our %canvas_size = ('width' => 800, 'height' => 190);
> 
> # List of all available time ranges
> our @time_ranges = ("hour", "day", "week", "month", "year");
> @@ -48,15 +51,12 @@ my @GRAPH_ARGS = (
> # For a more 'organic' look
> "--slope-mode",
> 
> - # HxW define the size of the output image
> - "--full-size-mode",
> -
> # Watermark
> "-W www.ipfire.org",
> 
> - # Default size
> - "-w $image_size{'width'}",
> - "-h $image_size{'height'}",
> + # Canvas width/height
> + "-w $canvas_size{'width'}",
> + "-h $canvas_size{'height'}",
> 
> # Use alternative grid
> "--alt-y-grid",
> @@ -1090,18 +1090,19 @@ sub updatecpufreqgraph {
> "--color=SHADEA".$color{"color19"},
> "--color=SHADEB".$color{"color19"},
> "--color=BACK".$color{"color21"},
> - "COMMENT:".sprintf("%-10s",$Lang::tr{'caption'}),
> + "COMMENT:".sprintf("%-15s",$Lang::tr{'caption'}),
> "COMMENT:".sprintf("%15s",$Lang::tr{'maximal'}),
> "COMMENT:".sprintf("%15s",$Lang::tr{'average'}),
> "COMMENT:".sprintf("%15s",$Lang::tr{'minimal'}),
> "COMMENT:".sprintf("%15s",$Lang::tr{'current'})."\\j"
> );
> 
> + my $j = 11;
> for(my $i = 0; $i < $cpucount; $i++) {
> - my $j=$i+1;
> + $j++; $j = 1 if $j > 20;

I think you could have written this slightly shorter by taking the modulus of j like so: $j = $i % 20;

But since this is not changing the behaviour, I am happy to see this merged.

-Michael

> push(@command,"DEF:cpu".$i."_=".$mainsettings{'RRDLOG'}."/collectd/localhost/cpufreq/cpufreq-".$i.".rrd:value:AVERAGE"
> ,"CDEF:cpu".$i."=cpu".$i."_,1000000,/"
> - ,"LINE1:cpu".$i.$color{"color1$j"}."A0:cpu ".$i." "
> + ,"LINE1:cpu".$i.$color{"color$j"}."A0:cpu ".$i." "
> ,"GPRINT:cpu".$i.":MAX:%3.0lf Mhz"
> ,"GPRINT:cpu".$i.":AVERAGE:%3.0lf Mhz"
> ,"GPRINT:cpu".$i.":MIN:%3.0lf Mhz"
> -- 
> 2.37.1.windows.1
>