Re: [RFC PATCH v2 7/8] ratp: new md and mw commands

[Aleksander Morgado <aleksander@aleksander.es>]

>> read and write memory files without needing to do custom string
>> parsing on the data returned by the console 'md' and 'mw' operations.
>>
>> The request and response messages used for these new operations are
>> structured in the same way:
>>
>>  * An initial fixed-sized section includes the fixed-sized
>>    variables (e.g. integers), as well as the size and offset of the
>>    variable-length variables.
>>
>>  * After the initial fixed-sized section, the buffer is given, which
>>    contains the variable-length variables in the offsets previously
>>    defined and with the size previously defined.
>>
>> The message also defines separately the offset of the buffer
>> w.r.t. the start of the message. The endpoint reading the message will
>> use this information to decide where the buffer starts. This allows to
>> extend the message format in the future without needing to break the
>> message API, as new fields can be appended to the fixed-sized section
>> as long as the buffer offset is also updated to report the new
>> position of the buffer.
>>
>> E.g. testing with ratp-barebox-cli:
>>
>>   $ ratp-barebox-cli -t /dev/ttyUSB2 --md "/dev/pic_eeprom_rdu,0x107,5" --timeout 1000
>>   Sending md request: read '/dev/pic_eeprom_rdu': 0x0107 (+5 bytes)
>>   00:00:00:00:00
>
> It would be good to have to pointer to libratp and ratp-barebox-cli in
> Documentation/user/remote-control.rst.
>

I'll add the info, ok.

> What's your plan for the bbremote tool? It's a bit unfortunate to have
> the new features only available in an external tool.
>

Yeah, I knew you were going to say that :) So, don't know. Didn't want
to spend much time on it because the new commands (md, mw, reset)
could directly be run with bbremote as "bbremote run ..." and you
would get the same output just with a different format. The benefit of
the binary API is clear in libratp-barebox, i.e. to integrate it into
applications that would make use of those operations without requiring
formatting output for the human eye. The ratp-barebox-cli and bbremote
support of the commands with the binary API would just be a
convenience. I actually only developed the support for the new
commands in the cli to make sure the library worked, as a way of
testing it. That said, if you want I can try to implement them in
bbremote as well and provide the same kind of output that you'd see in
ratp-barebox-cli; it would at least be a way of testing the API
directly within barebox without requiring any external tool.

>> +static int do_ratp_mem_md(const char *filename,
>> +                       loff_t start,
>> +                       loff_t size,
>> +                       uint8_t *output)
>> +{
>> +     int r, now, t;
>> +     int ret = 0;
>> +     int fd;
>> +     void *map;
>> +
>> +     fd = open_and_lseek(filename, O_RWSIZE_1 | O_RDONLY, start);
>
> It would be nice to support different read/write widths. Not every
> memory is readable in byte size chunks. But this could be added later
> aswell, I just saw that the way you defined the messages allows it to
> add additional fields later.

Yes, the binary message API should allow extending the format with
additional fields. But now that you said that, I may actually include
the width in the format, will check that.

>> +     if (fd < 0)
>> +             return 1;
>
> Is '1' the right return value here? It goes into a variable named
> 'errno' which looks wrong.
>

Oh, probably not. I'll review that.

-- 
Aleksander
https://aleksander.es

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox