[edid-decode] [PATCH 2/2] Calculate DisplayID checksums. Refactor do_checksum.

[walter harms]

Am 13.12.2016 11:39, schrieb Mark Ferry:
> Thanks for the feedback Walter. Comments below.
> 
> On Mon, 12 Dec 2016 20:02:08 +0100, walter harms wrote:
>>
>>
>> Am 10.12.2016 20:44, schrieb Mark Ferry:
>>> +    printf("Checksum: 0x%hx", x[len -1]);
>>> +
>>> +    for (i = 0; i < len; i++)
>>> +        sum += x[i];
>>> +
>>> +    if (sum) {
>>> +        printf(" (should be 0x%hx)", (unsigned char)(x[len-1] - sum));
>>> +    } else printf(" (valid)");
>>> +
>>>      printf("\n");
>>
>> 	any reason not to move the \n to the printf in front ?
>> 	just thinking of it ..
>> 	perhaps this can be reduced to invalid/valid ? i do not know anyone
>> 	seeing more in that information.
>> 	then you can simply return 0/-1.	
>>
> 
> Agree about the newline and a boolean return value.
> 
> I definitely want to keep the "should be 0xxx" output.
> Hacking EDIDs and having to recalculate the checksum myself is what
> motivated this patch. Not sure if that's what you meant.

But your are printing (x[len-1] - sum), where  x[len-1]== checksum
perhaps you wanted to print -sum ? (I assume the "checksum" is just
the diff to 0).

INHO the code should be like this:

exp_chk= x[len -1]; // expected sum

for (i = 0; i < len-1; i++) sum += x[i];  // real sum, notice len-1

if (exp_chk+sum != 0 )
	printf("expected %02x found 0x02x\"n, 255-exp_chk, sum);

/*
 i have no idea about EDID checksum nor tested the code, take with a pinch of salt
*/

re,
 wh

> 
>>>  
>>> -    do_checksum(edid);
>>> +    (void) do_checksum(edid, 128);
>>>  
>> in General i am a fan of checking return values,
>> why not here ?
> 
> Thanks you're quite right, and I've failed to set has_valid_checksum.
> 
>> and why the magic 128 ?
>> Perhaps we can avoid the global ?
>>
> 
> There are plenty of magic numbers throughout which I'm not about to
> clean up in this patch. But I'll take the opportunity to create
> EDID_PAGE_SIZE for 128.
> 
> Updated patch to follow.
> 
> - Mark
> 
> 
> 
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel