On Wed, Nov 17, 2010 at 10:11 AM, Peter Stuge <pe...@stuge.se> wrote:
>> diff --git a/src/svf/svf.c b/src/svf/svf.c
>> index 62e2324..c7401c3 100644
>> --- a/src/svf/svf.c
>> +++ b/src/svf/svf.c
>> @@ -221,6 +221,8 @@ static uint8_t *svf_tdi_buffer = NULL, *svf_tdo_buffer =
>> NULL, *svf_mask_buffer
>> static int svf_buffer_index = 0, svf_buffer_size = 0;
>> static int svf_quiet = 0;
>>
>> +static int svf_tap_is_specified = 0;
>> +
>>
>> static void svf_free_xxd_para(struct svf_xxr_para *para)
>> {
>> @@ -301,43 +303,50 @@ int svf_add_statemove(tap_state_t state_to)
>>
>> COMMAND_HANDLER(handle_svf_command)
>> {
>> -#define SVF_NUM_OF_OPTIONS 1
>> +#define SVF_NUM_OF_OPTIONS 2
>
> Aren't there three?
>
Yeah actually I had wondered about this, I simply incremented it....
and didn't come back to check it. It should probably be 3 and fix the
(1 + SVF_NUM_OF_OPTIONS) below.
>
>> int command_num = 0;
>> int ret = ERROR_OK;
>> long long time_ago;
>>
>> - if ((CMD_ARGC < 1) || (CMD_ARGC > (1 + SVF_NUM_OF_OPTIONS)))
>> + /* use NULL to indicate a "plain" xsvf file which accounts for
>> + additional devices in the scan chain, otherwise the device
>> + that should be affected
>> + */
>> + struct jtag_tap *tap = NULL;
>> +
>> + if ((CMD_ARGC < 2) || (CMD_ARGC > (1 + SVF_NUM_OF_OPTIONS)))
>> {
>> - command_print(CMD_CTX, "usage: svf <file> [quiet]");
>> + command_print(CMD_CTX, "usage: svf <device#|plain> <file>
>> [quiet]");
>
> Maybe we can find a better syntax for the command. Alternatively, I
> think it can be made backwards compatible.
>
> If jtag_tap_by_string() fails for the first parameter
> If there are only two parameters
> Assume that the first parameter is the filename
>
> This is somewhat heuristic:y however. Not nice. A more reliable
> approach might be:
>
> usage: svf [-tap device.tap] <file> [quiet]
>
> In particular I would also like to avoid "device#" because the
> parameter should not be a number at all, rather the dotted tap name.
> You mentioned that you modelled after xsvf, so that help text should
> probably also be clarified.
>
Yeah I thought the "device#" probably isn't the right name, the
command does accept the dotted name so the terminology should be
fixed. I tend to agree with you on the [-tap xxx ] idea, it is much
cleaner. I guess there's nothing special about the xsvf modules
handling, if anything it should probably be updated to support a -tap
command instead as well, and maybe quiet should be changed to -quiet
to be similarly consistent.
>
>> + if (svf_command_buffer_size < MAX_CHAIN_HEADER_COMMAND_LEN)
>> + svf_command_buffer = malloc(
>> + svf_command_buffer_size
>> + +
>> MAX_CHAIN_HEADER_COMMAND_LEN);
>
> The indenting seems off in the above. But more importantly I think it
> would be nice to avoid runtime allocation of memory here. The
> commands that this buffer is being used for are short so a char cmd[64]
> should be enough, and makes operation more reliable.
That's interesting, I actually would normally have used a basic static
buffer exactly how you suggest, it's only that the malloc'd
svf_command_buffer is used later for running commands I thought it'd
make sense to reuse it rather than adding a static ram usage to the
module (and therefore openocd).
>
>> + sprintf(svf_command_buffer, "STATE RESET ");
>> + svf_run_command(CMD_CTX, svf_command_buffer);
>
> sprintf() is not a friend. :) Yes, the commands are short, but still,
> it would feel better with snprintf(cmd, sizeof(cmd), ..).
I'll look into that, I've not used snprintf so don't really know the
difference. It's obviously only a google away. I'm just used to
sprintf from embedded devices when you often don't have a choice of
using a better alternative (depending on compiler).
>
> These commands without parameters don't even perhaps do not even need
> to be copied into the cmd buffer first, will svf_run_command() take a
> const char[]? Then just do:
>
> svf_run_command(CMD_CTX, "STATE RESET");
>
Yeah I initially had them written as such and they died big time, I'm
not sure how but they resulted in a smiley face character being
printed on console and system beep when they were run through the
command parser, so I went to the longer sprintf route. But it's
probably just a case of fixing the svf_run_command to accept const
chars would be a better option.
>
>> + sprintf(svf_command_buffer, "RUNTEST IDLE 5 TCK 200E-6 SEC ");
>> + svf_run_command(CMD_CTX, svf_command_buffer);
>
> Can you reason a little around these particular values?
>
Um... whoops I meant to cut them out, they're not needed anymore. They
were in the initial header from Actel, and are somewhat arbitrary in
that sense.
>
>> + sprintf(svf_command_buffer, "HDR %d TDI (0) ",header_dr_len);
>> + sprintf(svf_command_buffer, "HIR %d TDI (%X) ",header_ir_len,
>> ((1<<header_ir_len) - 1), ((1<<header_ir_len) - 1));
>> + sprintf(svf_command_buffer, "TDR %d TDI (0) ",trailer_dr_len);
>> + sprintf(svf_command_buffer, "TIR %d TDI (%X) ",trailer_ir_len,
>> ((1<<trailer_ir_len) - 1), ((1<<trailer_ir_len) - 1));
>
> ..
>> case HDR:
>> + if (svf_tap_is_specified)
>> + break;
>> xxr_para_tmp = &svf_para.hdr_para;
>> goto XXR_common;
>
> Would it make sense to reuse the existing handling of head/tail
> rather than creating commands to be run? This would remove the
> need for a command buffer at all, and maybe make the change even
> smaller.
>
Yeah I was initially going to do this, it's just that I found the code
somewhat obfuscated and decided to go the quick and easy to read route
rather than delving into the code much deeper. Basically it came to
lazyness which is the ideal way to create code bloat....
But yeah looking at it again now it does seem simpler than when I
initially looked though the code, amazing how a little familiarity
goes a long way. Perhaps I will fix it up, I've got a little time at
the moment and looks like maybe it'll be quite straightforward.
> Great work!
>
Thanks so much for the feedback.
Andrew
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
