Hi Tomas,
On 12/14/16 16:23, Tomas Melin wrote:
> Hi Simon, Igor,
>
> On 12/14/2016 02:53 PM, Igor Grinberg wrote:
>> On 12/13/16 22:29, Simon Glass wrote:
>>>>>>
>>>>>> I think two above debug() are very legitimate - no need to shout if no
>>>>>> FIT image
>>>>>> or no splash in it...
>>>>>>
>>>>>>> + res = fit_image_get_data_offset(fit_header, node_offset,
>>>>>>> + &splash_offset);
>>>>>>> + if (res < 0) {
>>>>>>> + debug("Could not find 'data-offset' property in FIT\n");
>>>>>>> + return res;
>>>>>>> + }
>>>>>>> +
>>>>>>> + res = fit_image_get_data_size(fit_header, node_offset,
>>>>>>> &splash_size);
>>>>>>> + if (res < 0) {
>>>>>>> + debug("Could not find 'data-size' property in FIT\n");
>>>>>>> + return res;
>>>>>>> + }
>>>>>>
>>>>>> Now regarding these two, I'm not sure.
>>>>>> Since we have found a valid FIT and also a node with a correct splash
>>>>>> name,
>>>>>> probably the intent is that we show the splash, right?
>>>>>> But in the two above checks, we find inconsistencies that do not allow
>>>>>> us to
>>>>>> show the splash - meaning the FIT is not actually good (am I right
>>>>>> here?).
>>>>>> So may be we should report it to the 'user' and allow correcting the FIT?
>>>>>> Otherwise, it is impossible to debug the image w/o a debug version of
>>>>>> U-Boot...
>>>>>> Do I make sense, or do I miss something?
>>>>>
>>>>> Yes that makes some sense, but the problem is that then you are
>>>>> including error messages always which would never happen in a working
>>>>> system (i.e. it just bloats the code).
>>>>
>>>> Unless, there a kind of corruption or a user mistake and then that same
>>>> user can't even understand what happened because we do not help him with
>>>> an error message.
>>>> You cannot know that these error messages will never happen...
>>>> This is a generic code which can be used by a wide variety of platforms -
>>>> I don't think you can foresee all the possible use cases.
>>>>
>>>> We are talking about several tens of bytes vs. usability.
>>>> If there is an error, it should be stated as such. It should not just
>>>> exit silently...
>>>
>>> I agree with that, there should definitely be an error printed. It
>>> should say something like 'Failed to load splash screen (err=-4)' or
>>> something like that. The error number should provide some clues and
>>> people can dig in.
>>
>> Great idea!
>
> splash_load_fit() currently fails silently but still reports the error in
> the return value. And this function is used so that board.c calls
> splash_source_load()->splash_load_fit().
> The board function call will get notified of the error value as provided
> by the return value for splash_load_fit(). In our board implementation that
> is
> actually exactly how it is done, the board function call checks the return
> value and prints ("Failed to load splash screen image, error: %d\n", ret)
> in case there was and error.
>
> IMHO this is quite good behaviour as is, leaving it up to the implementation
> in the board.c if there should be a error message or not (and if it should
> bloat the code with another printf or not).
Well, yes this makes sense if you care to do the work in the board code.
Although, I would expect that sometime this code will be called from
a generic board independent place (e.g. init array, etc.).
>
>>>>>
>>>>> So long as the error is reported (even if it is not a very specific
>>>>> error), people can add DEBUG and track it down.
>>>>
>>>> That depends who 'people' are? Devs? Users?
>>>
>>> Well in this case the user will never see the problem, unless someone
>>> has screwed up the splash screen image. Mostly I'm talking about devs.
>>>
>>> Better would be to have an expanded debug() system which lets you turn
>>> debugging on globally when hunting for a problem. That would be a nice
>>> project for someone...
>>
>> Yes, indeed that sounds like a nice project.
>> It would be great to be able to specify the debug verbosity on per build
>> basis (e.g. Kconfig).
>>
>
> Indeed, that would be a great feature.
>
> Regards,
> Tomas
>
--
Regards,
Igor.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
