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.hu...@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/
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
