Feedback on the WREN protocol
See attached my visualisation of an Ethernet Frame with the contents per the WREN protocol specified in wren-packet.h ( e293b2be);
This is my feedback to the wren-packet.h ( e293b2be);
- "pad" fields could be used in the future, they should be marked "reserved" and it is important the the transmitter sets them to zero while the receiver ignores them (made it clear in the specification)
- "cmd" -> it should be called "type"
- on wren_packet_hdr
- why cannot it be yet another TLV type, i.e. wren_packet_hdr, wren_packet_ctxt, wren_packet_event - they are all similar in the content: type(cmd)/len + some data + timestamp). Also "parameters", see below, could be simply a different TLV types with the very same header
- I'd put "source" and "seq_id" next to each other, they are together a unique identifier and can be treated as a single number
- wren_packet_ctxt and wren_packet_event
- it is not clear from the comments/text that these structures are only headers and more data follows (i.e. parameters), propose one of the following
- rename these structures to wren_packet_ctxt_hdr and wren_packet_event_hdr
- (preferred): include in the structure a placeholder for the parameter_tlv structure
- it is not clear from the comments/text that these structures are only headers and more data follows (i.e. parameters), propose one of the following
- parameters
-
Ideally, a dedicated structure would be defined. I'd make parameters yet another structure with the same TLV header as all the other structures
-
it seems that the "id" in the text below is the "value" of the TLV, i.e. it can be any data of any length and it is ID for the typ=
PKT_PARAM_U32 0x1
-> correct?/* Parameter: len: bits 11-0 typ: bits 15-12 id: bits 31-16. */
-
for other types of parameters, does the data need to always be N x 32 bits + 16 bits?
-
- currently, the order of type, length, value is different in different structures:
- in wren_packet_hdr -> now type, length in the middle, value around length
- in wren_packet_ctxt and wren_packet_event -> type (called "cmd"), pad, length, and value -> seems OK, not sure why "pad" is needed, why cannot we just have already 16b cmd/type and reserve some range of values for later
- in parameters: length, type, value -> not sure why length first
- I'd propose to have a common tlv_hdr (16b for type, 16 bit for length) that is common to all structures, including parameters. As it inflates a bit he parameters, it could be seen whether these could be considered special
- for all non-parameters
- 16b: type
- 16b: length
- Xb: data
- for parameters
- Xb: type (e.g. 4 bytes, so the first 16 type values are reserved for param and for this range, the parser knows the length field is different
- 12b: lenght
- Xb: data
- for all non-parameters