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