On Tue, Jul 12, 2011 at 14:07, Jie Zhang wrote:
> On Tue, Jul 12, 2011 at 2:49 AM, Mike Frysinger wrote:
>> On Monday, July 11, 2011 14:15:17 Jie Zhang wrote:
>>> On Wed, Jul 6, 2011 at 5:48 PM, Mike Frysinger wrote:
>>> > if we tweak the name and merge your older idea, we could have:
>>> > urj_log_error_describe (level)
>>> > now that the message prefix issue is sorted out in the core, we dont need
>>> > to worry about that anymore.
>>>
>>> As I said before, there is no meaning to pass levels other than
>>> WARNING or ERROR to urj_log_error_describe. That function describes an
>>> error.
>>
>> urj_error_io() is akin to the C library strerror() and errno. so while they
>> usually describe warnings/errors, the fact that they're set/read is not
>> always. think of code which uses C library functions where you look for
>> multiple files ... some not being found is not an error or warning, but
>> common
>> behavior. or perhaps reading from some data source was interrupted
>> (signal/ptrace/etc...). the errno would be EINTR, but would not be shown as
>> an error (if at all). or the data source is set as nonblocking and so you
>> get
>> back EAGAIN since there is currently no data available.
>>
>> if the user had a noisier debug level set, they might want to see all of
>> those
>> hits, but they wouldnt get logged at the warning/error level. they'd get
>> logged at "detail" or perhaps "debug".
>
> When a user program calls C library to open a file, if the file cannot
> be open, it's definitely an error for C library.
exactly right
> But how to deal with this error is up to the user program
> according to user program's verbose level, which might be
> different to the library's verbose level.
not quite. there is only one verbose level (urj_log_state.level), and
while the library holds onto it, it's under the user's control. this
is the level the user has to control what messages they care to
actually see.
the leaf functions of liburjtag set the error (i.e. "errno = xxx").
the functions that call those errors get to determine how much of an
"error" those errors actually are. perhaps it's fatal (so we log at
"error" level), or perhaps it's recoverable but generally not a good
thing (so we log at "warning" level), or perhaps it's generally
harmless and can be ignored (so we log at "detail" level).
urj_log_state.level has no bearing at what level the library uses when
it calls urj_log(). that comes into play internally as a check
internally to see whether it actually gets logged.
> So why embed library's log level into the error message? Log
> level and error message are two different things.
correct
> We should not
> require user to pass a log level to urj_log_error_describe () to get
> the error message. For this sake, this function would be better to be
> renamed to urj_error_describe (), as akin to the C library.
the point of urj_log_error_describe(level) is simply an optimization
to the common idiom:
urj_log (level, "%s\n", urj_error_describe());
urj_error_reset ();
and to make to keep people from doing the wrong thing (as some of our
code shows now):
urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n", urj_error_describe());
logging an "error" at the "normal" level is incorrect imo.
so keeping urj_error_describe() makes sense for the reasons you
highlight, but we still want urj_log_error_describe(level) for the
reasons i mention above.
>>> > i think this part should read:
>>> > if (level == URJ_LOG_LEVEL_WARNING || level == URJ_LOG_LEVEL_ERROR ||
>>> > urj_log_state.level <= URJ_LOG_LEVEL_DEBUG)
>>> > r += log_printf (p, "%s: ", urj_log_level_string (level));
>>> > if (urj_log_state.level <= URJ_LOG_LEVEL_DETAIL)
>>> > r += log_printf (p, "%s:%i %s(): ", file, line, func);
>>>
>>> Yes, that's your idea in your previous email. I tried that. But I
>>> found always adding a level prefix would make output ugly and unclear
>>> to read. There is little use to indicate the level of a message when
>>> they are not errors or warnings. For the same reason, I chose to only
>>> output source line information for WARNING and ERROR.
>>
>> i dont understand ... the prefix isnt normally seen, only when the user
>> manually increases the debug level. and when you increase the debug level,
>> you want the extra output. this snippet i actually tested and the output
>> from
>> doing like "cable probe; detect" was unchanged at the default debug level of
>> "normal". only when i did "debug detail" did i see the extra prefixes.
>
> Did you test by programing a flash or update the firmware for ICE-100B
> cable under detail log level.
>
> cable/ice100.c:658 ice_send_flash_data(): .cable/ice100.c:658
> ice_send_flash_data(): .cable/ice100.c:658 ice_send_flash_data():
>
> vs with my patch
>
> ............
so your issue is with progress bars. all the other lines looked fine to me.
i'd say that this is "logging" vs "user output" and is a general
problem. on one hand, we have the library code (the ice100b driver)
which only exists to perform operations. on the other hand, we have
an application (`urjtag`) which is merely an interface to the library.
progress bars are part of the app, not the library.
in this regard, i think we want to update urj_log_state_t to take
another func pointer for progress bars. that way the app can add a
hook for properly updating things itself (whether it be a gui bar that
fills up, or a spiny wheel, or something else). then we'd have
helpers that the ice100b firmware code would use to post updates about
its progress. either it's a 0% to 100% completion, or it's a simple
"yes, i'm still alive, please wait".
for now, i know this has ballooned way beyond what Chad expected, so
i'm fine with the display that mine proposal ends up with at more
verbose levels. and i can take care of implementing the progress bar
idea so you can move on :). but you'll have to hook me up with some
firmware files so i can flash my ice100b cable.
-mike
------------------------------------------------------------------------------
AppSumo Presents a FREE Video for the SourceForge Community by Eric
Ries, the creator of the Lean Startup Methodology on "Lean Startup
Secrets Revealed." This video shows you how to validate your ideas,
optimize your ideas and identify your business strategy.
http://p.sf.net/sfu/appsumosfdev2dev
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
