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

[Leo-Andres Hofmann <hofmann@leo-andres.de>]

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

Hi,

Am 13.03.2023 um 10:23 schrieb Michael Tremer:
> 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.

Thanks for the review. Yes of course, no idea why I didn't think of that...

Best,
Leo

>
> -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
>>