[PATCH v2 5/5] libtest: Add packet processor
Sebastian Huber
sebastian.huber at embedded-brains.de
Mon Feb 5 10:13:32 UTC 2024
Hello Chris,
thanks for having a look at it.
On 02.02.24 00:14, Chris Johns wrote:
> Hi,
>
> Thanks for the updated documentation, protocol and use cases. It has helped. Now
> I understand some of the context I have raised further questions about it I feel
> we should resolve. Without a protocol version number being exchanged it limits
> how we can grow and develop this protocol beyond where it is.
>
> On 26/1/2024 6:23 am, Sebastian Huber wrote:
>> The RTEMS Test Framework packet processor provides a simple mechanism to
>> exchange reliable and in-order data through transmitting and receiving
>> one character at a time.
>>
>> The packet processor does not buffer data. The processor uses a
>> stop-and-wait automatic repeat request method. There is at most one packet
>> in transmission. The data transfer is done using a single character input
>> and output method. The protocol uses 12-bit sequence numbers, so a host
>> could use a sliding window method to increase throughput. All integers and
>> data are base64url encoded. A 24-bit CRC is used to ensure the data
>> integrity. The '{' character starts a packet. The '}' character terminates
>> a packet. The '#' character prefixes a 24-bit CRC value. The ':' character
>> separates fields. The '+' character prefixes data fields.
>
> Is the line encoding a subset of the ASCII character set? Is the line disciple raw?
yes, only ASCII (7-bit) is used. The processor uses a character
input/output handler. It is up to the user to do the transfer. It could
be for example also CAN or UDP packets.
>
> I have reviewed the protocol as it is and I have some suggestions. The data link
> layer messages are mixed with the pay load messages. If the messages followed:
>
> {<12-bit seq><12-bit ack>:<packet>[:[data]]]#<24-bit CRC>}
>
> where:
>
> <packet> is `H`, `A`, `R` or `P` are data link packets and `P` for payload can
> contain `J`, `L`, `S` etc for the test and chain loader protocol. For example:
>
> {<12-bit seq><12-bit ack>:P:J:<64-bit address>#<24-bit CRC>}
In my proposal, the payload has its own CRC so that you can first
validate the header before you do something with the payload.
>
> Note, `P` could be a protocol id and so `T` could denote "test and chain loader".
>
> [:] denotes this is optional depending on pay load data being present or it
> could mean data link vs payload packets, what ever works here.
>
>> The following packets are defined:
>>
>> * hello: {<12-bit seq><12-bit ack>:H#<24-bit CRC>}
>
> Are you able to have the `H` be a query and return data? A hello that contains
> protocol versioning data is always useful for long lived protocols. Version
> numbering should be 1, 2, 3, 4 etc and 0 is reserved.
Adding a protocol version to the hello message is a good idea.
>> * acknowledge: {<12-bit seq><12-bit ack>:A#<24-bit CRC>}
>
> Does this ack all seq numbers up to this number?
Yes, it is like in TCP, however, currently only a stop-and-wait
automatic repeat request method is implemented so that we don't need
buffers.
>
>> * reject: {<12-bit seq><12-bit ack>
>> :R:<12-bit seq of rejected packet>#<24-bit CRC>}
>
> Are the `<>` fields binary and so base64 encoded?
Yes, everything in <> is base64 encoded.
>
>> * jump: {<12-bit seq><12-bit ack>:J:<64-bit address>#<24-bit CRC>}
>>
>> * load: {<12-bit seq><12-bit ack>:L:<64-bit load address>:
>> <64-bit load size in bytes>#<24-bit CRC>+<data to load>#<24-bit CRC>}
>>
>> * signal: {<12-bit seq><12-bit ack>:S:<64-bit signal number>:
>> <64-bit signal value>#<24-bit CRC>}
>
> Are the signals defined? If so maybe a reference to the enum or whatever?
No, the signal and channel numbers are user-defined.
>
>> * channel: {<12-bit seq><12-bit ack>:C:<64-bit channel number>:
>> <64-bit data size in bytes>#<24-bit CRC>+<channel data>#<24-bit CRC>}
>
> How would I add a packet type to the protocol? For example a 'D' packet could
> transport GDB remote protocol for use on targets with limited resources and the
> need to share this channel.
You don't need a new packet type for this. You can use a channel. We
have a 64-bit channel number, so plenty of channels.
>
> The data link and framwwork is useful for our project beyond the test use case
> so it would be good to see if something more useful is within reach. :)
>
>> The intended use case are boot loaders and test runners. For example, test
>> runners may interface with an external test server performing equipment
>> handling on request using the packet processor.
>>
>> Use T_packet_initialize() to initialize the packet processor. Use
>> T_packet_process() to drive the packet processing. You can enqueue
>> packets for transmission with T_packet_enqueue(). You can reliably send
>> signals with T_packet_send(). You can reliably transmit and receive
>> channel data with T_packet_channel_push() and
>> T_packet_channel_exchange().
>>
>> A simple boot loader could be implemented like this:
>
> If this is RTEMS running the loader and then jumping to something loaded would
> be a chain loader and not a boot loader? The use of the term boot loader
> confused me. Chain loading is tricky because you need to handle the address
> spaces to avoid clashing or you need to have a way to relocate the loaded data.
> How is this handled?
I don't know what a chain loader is. If more than one program is
involved in the boot process, I would call this first-stage,
second-stage, etc. boot loader.
It is up to the user to deal with memory space collisions. The event
handler can check the load address for example. I used this packet
processor to run a boot loader in internal flash and load an application
to external SDRAM.
>
> A chain loader package in libmisc (or where ever) would be a very nice addition.
>
> Given this I question the prefixing with T_ for the protocol. Again I see it as
> more useful than just testing. :) I am sure testing has been the leading use
> case for you but the T_ fingerprint assumes it is just for testing. :)
In this case, you can think of 'T' as transfer.
[...]
>> +static void do_type(T_packet_control* self, uint8_t ch) {
>> + update_crc(self, ch);
>> + self->packet_type = ch;
>> +
>> + switch (ch) {
>> + case 'C':
>> + self->packet_done_event = T_PACKET_EVENT_CHANNEL_END;
>> + self->state = T_PACKET_STATE_COLON;
>> + break;
>> + case 'S':
>> + self->packet_done_event = T_PACKET_EVENT_SIGNAL;
>> + self->state = T_PACKET_STATE_COLON;
>> + break;
>> + case 'L':
>> + self->packet_done_event = T_PACKET_EVENT_LOAD_END;
>> + self->state = T_PACKET_STATE_COLON;
>> + break;
>> + case 'J':
>> + self->packet_done_event = T_PACKET_EVENT_JUMP;
>> + self->state = T_PACKET_STATE_COLON;
>> + break;
>> + case 'H':
>> + self->packet_done_event = T_PACKET_EVENT_HELLO;
>> + self->state = T_PACKET_STATE_HASH;
>> + break;
>> + case 'A':
>> + self->packet_done_event = T_PACKET_EVENT_ACKNOWLEDGE;
>> + self->state = T_PACKET_STATE_HASH;
>> + break;
>> + default:
>> + self->packet_done_event = T_PACKET_EVENT_REJECT;
>> + self->state = T_PACKET_STATE_REJECT;
>> + break;
>> + }
>> +}
>
> You have handlers for the time, ie clock_monotonic, so why not one to handle the
> payload? My quick review would indicate the code is clean enough to handle this
> but I am not sure.
There is a handler to deal with the payload. This is the event handler.
For example, for a channel receive, the event handler has to set a
target address for the data. In the test code:
+ case T_PACKET_EVENT_CHANNEL_BEGIN: {
+ output_char(base, 'C');
+ T_eq_u64(T_packet_get_channel_number(base),
UINT64_C(0x1234567887654321));
+ size_t size = T_packet_get_channel_size(base);
+
+ if (size != 0) {
+ T_packet_set_channel_target(base, &self->load_buf[0]);
+ T_eq_sz(size, 0xd);
+ }
+
+ break;
+ }
[...]
>> +static void idle_processing(T_packet_control* self) {
>> + event(self, T_PACKET_EVENT_NOTHING);
>> +
>> + if (self->state != T_PACKET_STATE_START) {
>> + return;
>> + }
>> +
>> + T_packet_packet* head = self->snd_head;
>> +
>> + if (head == NULL) {
>> + return;
>> + }
>> +
>> + uint32_t now = (*self->clock_monotonic)(self);
>> +
>> + if (self->snd_pending != NULL) {
>> + if ((self->snd_timeout - now) <= UINT32_MAX / 2) {
>> + return;
>> + }
>
> The now value is 32bits so not nano-seconds and UINT32_MAX / 2 seems like a long
> time? There are no commants about this timeout handling and I would have to
> guess what is happening here. I would prefer having comments to guide me.
The time is defined by the clock monotonic handler of the user. This
code detects an unsigned overflow. I will clarify this code block a bit.
[...]
--
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
More information about the devel
mailing list