On Thu, Sep 8, 2011 at 9:04 PM, Mike Frysinger <[email protected]> wrote:
> On Wednesday, September 07, 2011 09:16:35 Jie Zhang wrote:
>> On Wed, Sep 7, 2011 at 12:45 AM, Mike Frysinger wrote:
>> > On Friday, September 02, 2011 18:38:35 Jie Zhang wrote:
>> >> + fflush (stdout);
>> >
>> > i dont think we should be touching stdout
>>
>> urj_tap_discovery does fflush. I just follow the existing code. I
>> think the fflush is used to ensure the log information is displayed
>> before detecting.
>
> we should fix that :). the library should not be fflushing stdout anywhere
Why the library should not be fflushing stdout anywhere?
>
>> >> --- src/cmd/cmd_discovery.c (revision 1994)
>> >> +++ src/cmd/cmd_discovery.c (working copy)
>> >>
>> >> urj_log (URJ_LOG_LEVEL_NORMAL,
>> >> - _("Usage: %s\n"
>> >> - "Discovery of unknown parts in the JTAG chain.\n"
>> >> + _("Usage: %s [current|INSTRUCTION]\n"
>> >> + "When no arguments, discovery of unknown parts in the
>> >> JTAG chain.\n"
>> >
>> > this should be with the same text that describes the
>> > [current|INSNTRUCTION] argument (i.e. at the bottom). also, it should
>> > say:
>> > When no arguments are specified, unknown parts in the JTAG chain are
>> > discovered
>>
>> I'm willing to use "When no arguments are specified". But I don't
>> think this should be with the text at the bottom without changing the
>> rest of the help text. You can try and read it in a whole, you will
>> find the case of no arguments and the case of having arguments are
>> then mixed and less clear than my patch.
>
> "Usage: %s [current|INSTRUCTION]\n"
> "Discovery of unknown parts in the JTAG chain.\n"
> "\n"
> "When no argument is specified, '%s' attempts to detect these\n"
> "parameters of an unknown JTAG chain:\n"
> " 1. IR (instruction register) length\n"
> " 2. DR (data register) length for all possible instructions\n"
> "\n"
> "When an argument is specified, '%s' attempts to detect the data\n"
> "length of the specified instruction.\n"
> "\n"
> "Warning: This may be dangerous for some parts (especially if the\n"
> "part doesn't have TRST signal).\n"
>
When an argument is specified, the command is not used to discover the
unknown parts, it's only for detecting the DR length of the specified
INSTRUCTION. So the first sentence should not be a summary description
of the command.
The Warning only applies when no argument is specified.
>> >> + "When there is an argument, discovery of the DR size of
>> >> the current or\n"
>> >> + "specified instruction.\n"), "discovery",
>> >
>> > use: When an argument is specified, the DR size of the specified
>> > instruction is discovered.
>>
>> As above, I'm willing to change it to "When an argument is specified".
>> But I think the last half of the sentence is good, especially it
>> contains the command name "discovery". Your sentence is more like an
>> English sentence but the original sentence is also OK with English
>> syntax.
>
> the original has slightly broken grammar, or is at least confusing when it
> uses the straight command name "discovery" in place of proper structure.
>
When I first read the help of this command several years ago,
"discovery of" was also strange to me. But now I'm used to it. Now I
also think it has some goodness. So I don't see a strong reason to
change it.
>> > what if someone names an instruction "current" ? seems like this would
>> > break that. also, you should add a signal completion handler ;).
>> > there's a helper to do this for you already ... see some of the
>> > existing commands that take signals.
>>
>> The INSTRUCTION could only be in two forms: 0xnnnn or 01 strings. It
>> could not be "current".
>
> reading the help made me think i could do:
> discovery BYPASS
> so the help text should explicitly mention the format it expects.
>
I also considered this. But I could not find a concise way to describe
it. So I just hope we can make it accept INSTRUCTION name in future.
>> > also, it seems like "discovery FOO" will implicitly change the active
>> > insn to "FOO". should we be saving/restoring the active insn ? or am i
>> > just reading your patch wrong ?
>>
>> Yes, my patch will clobber the existing instruction. But the existing
>> command discovery clobbers even much more. It resets the TAP.
>
> yes, i missed that part of the recent refactoring of the discovery command.
> we should document these side-effects in tap.h above the func prototype.
>
Not in the scope of my patch.
Regards,
Jie
------------------------------------------------------------------------------
Why Cloud-Based Security and Archiving Make Sense
Osterman Research conducted this study that outlines how and why cloud
computing security and archiving is rapidly being adopted across the IT
space for its ease of implementation, lower cost, and increased
reliability. Learn more. http://www.accelacomm.com/jaw/sfnl/114/51425301/
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
