Refactoring memap thoughts

[andrewparlane]

I've found an issue in the memap read and write functions when using LIBSWD_MEMAP_CSW_ADDRINC_SINGLE. I'm going to make a fix and create a pull request. Give me a few hours.

However looking at this code, I think it could do with a big refactor, but I want to talk it over first, so that I'm clear on how to do this.

At the moment we have:
int libswd_memap_read_char(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, char *data);
int libswd_memap_read_char_csw(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, char *data, int csw);
int libswd_memap_read_char_32(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, char *data);
int libswd_memap_read_int(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, int *data);
int libswd_memap_read_int_csw(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, int *data, int csw);
int libswd_memap_read_int_32(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, int *data);
int libswd_memap_write_char(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, char *data);
int libswd_memap_write_char_csw(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, char *data, int csw);
int libswd_memap_write_char_32(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, char *data);
int libswd_memap_write_int(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, int *data);
int libswd_memap_write_int_csw(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, int *data, int csw);
int libswd_memap_write_int_32(libswd_ctx_t *libswdctx, libswd_operation_t operation, int addr, int count, int *data);

Which is a bit OTT.

So the _CSW functions let you set up CSW first
the _int_32 functions are the same as the _int_csw functions but internally specifying 32 bit ints with auto increment.

and the _char and _int functions are the same but store the result in a char or int array respectively. There is however a difference here where the _int functions don't check the CSW accsize and just assume it's 4. What happens if we have CSW set to 8 bit accesses and use the _int function? Do we read / write one byte at a time and store that in an int array. IE if the data at the target address (in bytes) was 0,1,2,3,4, 5, 6, 7, and we read_int with 8 bit accesses, is our read data int array {0, 1, 2, ... } or { 0x03020100, 0x07060504 } Looking at the code for manual tar increment in _int we have
for (i=0;i<count;i++) { loc=addr+i*4; ... res=libswd_ap_write(libswdctx, LIBSWD_OPERATION_EXECUTE, LIBSWD_MEMAP_TAR_ADDR, &loc);
So to me this says it's readying the LSB of every word and storing it into a int array. IE, you int array would be { 0, 4 }

Does that even make sense? Is there a use case for this? I mean I can imagine a possible use case, but it'd be easy enough to just and out the extra bits in user code.

Finally if you use auto increment single in this case, the docs say the address is incremented by the access size. So your int array would be { 0, 1, 2, 3, ... }. Which is different to the manual increment version.

So refactoring ideas:

1. do we really need int and char functions? Can we not make all of them char functions. If you want to write an int, you just cast your int buffer to a char buffer?
2. make all functions _csw functions, either keeping the _csw suffix or dropping it. We already cache the current CSW value, so we only update CSW if it's different to the cached version.
3. ditch the _int_32 functions as they're now unnecessary.

This should leave us with one read and one write function. This would remove a bunch of code duplication, and simplify which function to use and when.

Any thoughts,
Andy

[cederom]

Thank you Andrew! Will be back from long weekend on monday so I could get back to details :-)

char and int functions were created to return two types of data which is more convenient to the user (i.e. int for automation code, char for storage).

if there is csw function then this is also created for convenience or to mark specific behavior using csw. i am sure you can do the same without this csw function but with more manual work.

Will get back to you with details next week! :-)

[andrewparlane]

Sure thing, enjoy your long weekend. Let's chat next week.

I think if you want an _int function, it should just call the relevant _char function, to minimise code duplication. What we need to define is what it should do, since as I mentioned it seems to be different depending on auto and manual tar increment. I could set up a test to prove that, but ...

As for the CSW stuff, I think it's worthwhile merging those with the default functions, since it's possible users could make a mistake by assuming CSW will already be set to x when actually it's set up as y. I think being explicit per transfer makes sense here. Then adding some comments to the function declarations that say how and when to use each option.

Andy

[cederom]

ACK :-)