Author Topic: Parsing UART packets / state machines / packed structs  (Read 2083 times)

Ted/KC9LKE and 3 Guests are viewing this topic.

Online uer166

  • Frequent Contributor
  • **
  • Posts: 645
  • Country: us
Re: Parsing UART packets / state machines / packed structs
« Reply #25 on: December 05, 2022, 06:13:16 pm »
You cannot counter someone's argument by simply insinuating they are incompetent, ignoring "facts" and so on.

Unlike you, nctnico is highly competent and I deeply respect his competence. He just has a few weird "blind spots" where his opinions, which are just matter of taste which I could accept as opinions, like disliking structs in networking and preferring bit manipulation reconstruction, make him close his eyes from objective facts. I'm sure he still does excellent job.

I have offered all the relevant information, unlike you; in other words, please get out of our argument, your comments are completely unhelpful. We have enough trolls on this forum already. Thank you in advance.

I see through this Sir, when you speak it is to be regarded as "facts" as "objectively true" but the words of another (who disagrees with you) are merely "opinions" as the words of one who has "closed" their eyes, has "blind spots".

You are a brave man indeed, hiding behind a keyboard and unhesitatingly insulting me and my presumed lack of competence. Is that how you speak to coworkers who dare to disagree with you? Is that really the way you'd communicate in person? I doubt it, I bet you're as nice as pie under those circumstances, nobody who bullies as you do here, would last long in the workplace.

If you have nothing technical to add to the discussion, feel free to avoid the thread. I don't mind technical discourse but this is not useful.
 

Online Siwastaja

  • Super Contributor
  • ***
  • Posts: 6115
  • Country: fi
Re: Parsing UART packets / state machines / packed structs
« Reply #26 on: December 05, 2022, 06:45:09 pm »
The above opened my eyes a little bit, since the first few bytes:
|0x55|0xAA|LENGTH|ADDRESS|0x01|CMD|

Are actually predictable and are all the same given a specific CMD. the CMD defines the length, and the ADDRESS is always a fixed number. This means I can easily sync on the first 6 bytes, which gives a nice 48-bit sequence. This would avoid the need for a 255-byte buffer and any re-sync should be pretty quick. This is kinda like having a CRC since the length and address is always a unique fixed pair in the messages that I care about.

One can use something like this:
Code: [Select]
typedef struct
{
   uint8_t id;
   uint8_t len;
   void (*handler)(void* msg);
} msg_meta_t;

#define MSG(id, prefix) {.id=(id), .len=sizeof (prefix ## _msg_t), .handler = prefix ## _handler}

static const msg_meta_t MSGS[] =
{
   MSG(42, measure_voltage),
   MSG(57, self_destruct)
};

Or, even better, make the id contiguous from 0 (not a sparse table) and use the meta table index directly as msgid.

But with such sparse table (current project of mine uses this approach because message type space is 16 bits wide and we may have many hundreds of message types eventually, where each product only implements subset of):

When receiving msgid, loop through the meta table:
Code: [Select]
for(int i=0; i<NUM_ELEM(MSGS); i++)

Now you get the length from the table. If you still prefer to send it over wire - not a bad idea, now you can do a sanity check if these two match. But if you trust yourself to keep both parties up-to-date, no need to transfer the len field.

When done, and CRC checks out, you just call the handler through the function pointer, giving them the other two of the buffers. (In this example, we used void* pointer that we cast back to the desired type. I know it is safe to do so because the original buffer type is an union with the type in it, but if this concerns you, feel free to make that meta function pointer another union, with all possible argument types, then you can pass the actual correct type without casting it.)

I prefer to utilize preprocessor copy-and-paste to
1) reduce typing the same things over and over again, reducing human errors when changing things
2) force to use unified naming scheme, like
self_destruct_handler()
which uses
self_destruct_msg_t
as payload input type.

This is not super-fancy and not super-high level, but it's some kind of sweet spot for a simple language like C.
« Last Edit: December 05, 2022, 06:58:56 pm by Siwastaja »
 

Online NorthGuy

  • Super Contributor
  • ***
  • Posts: 2885
  • Country: ca
Re: Parsing UART packets / state machines / packed structs
« Reply #27 on: Yesterday at 03:10:27 am »
Yup, in fact I am in control for the request packets. I.e. I can expect silence until the time where I send out a formatted "give me cell voltages" or whatever. I can see your point about efficiency loss here being similar to overhead, maybe something like Consistent Overhead Byte Stuffing would help.

Syncing to 0xaa55, given that 0xaa55 may occur in the middle of the packet, complicates things, so try to avoid it.

If you ask for cell voltages, you know what are you going to receive. The length is probably fixed too. So, you do very simple thing - you send your request, and then you use DMA (or interrupt if you don't have DMA) to collect data into a buffer until you receive the needed number of bytes or until the operation times out. Then you parse the received data - verify the preamble, length, message type, checksum etc. You don't need BMS data very often, so you have plenty of time to do so. No need for a state machine or anything of that sort. Don't complicate things without a need.

 

Online NorthGuy

  • Super Contributor
  • ***
  • Posts: 2885
  • Country: ca
Re: Parsing UART packets / state machines / packed structs
« Reply #28 on: Yesterday at 03:17:56 am »
I already explained precisely why mapping structs on buffer is bad: not all platforms support it  AND it doesn't solve byte order (many protocols use big endian byte order while many CPUs nowadays are little endian). It is not much simpler than that.

What do you mean by "not all platforms support"? If platform supports pointers, then you certainly can interpret such pointers as pointers to structs. Do you have any example of a platform which wouldn't support it?

If a field contains a big endian (e.g. a port number in IP protocol) you can easily invert it, or you can keep all your port numbers in big endiam format. Why is that a big deal?
 
The following users thanked this post: Siwastaja

Online SiliconWizard

  • Super Contributor
  • ***
  • Posts: 10665
  • Country: fr
Re: Parsing UART packets / state machines / packed structs
« Reply #29 on: Yesterday at 05:26:32 am »
Any platform I've ever used in C from 8-bit to 64-bit had compilers allowing struct packing. The only problem to be aware of is again alignment, which may be a problem on some platforms for accessing certain ill-aligned struct members directly. That's the case in particular for the 16-bit PIC MCUs for accessing 16-bit or 32-bit struct members. They need to be aligned.

As to endianness, yeah it's not a big deal. If you make your own protocols don't bother with big-endian as it's pretty rare nowadays on processors. Buf if you have to deal with it, you can just swap bytes "in place" as NorthGuy said, for the members that require it. Still usually eons simpler, faster and more readable than hand-decoding packets byte per byte.

The real potential issue is the alignment, and that's something you have to consider for the target you're programming for. And for the protocol you're designing as well, unless you have no choice and have to use an existing one. Otherwise, defining your data packets to have "fields" that are all naturally aligned will pretty much cover all targets, and it's not rocket science.

 

Offline Nominal Animal

  • Super Contributor
  • ***
  • Posts: 4472
  • Country: fi
    • My home page and email address
Re: Parsing UART packets / state machines / packed structs
« Reply #30 on: Yesterday at 05:35:58 am »
If you want to use function pointers, then a pattern where each character received is handled by a function which returns the pointer to the function that handles the next character, should work:
Code: [Select]
#include <stdint.h>
#include <stddef.h>

// Known states:
static void *wait_55(uint8_t rx);
static void *wait_AA(uint8_t rx);
static void *wait_len(uint8_t rx);
static void *wait_addr(uint8_t rx);
static void *wait_01(uint8_t rx);
static void *wait_cmd(uint8_t rx);
static void *wait_data(uint8_t rx);
static void *wait_check(uint8_t rx);
static void *wait_skip(uint8_t rx);
static void *wait_skipcheck(uint8_t rx);
static void *in_error(uint8_t rx);

// Handler for the next incoming byte
void *      (*BMS_UART_State)(uint8_t) = wait_55;

// Handler for the currently received command (unused if skipped)
void *      (*BMS_UART_Handler)(void) = NULL;

uint_fast16_t BMS_UART_Checksum = 0;
uint_fast16_t BMS_UART_Datasum = 0;
uint8_t      BMS_UART_Data[255];  // Or whatever is the longest supported command
uint8_t      BMS_UART_Have = 0;
uint8_t      BMS_UART_Length = 0;
uint8_t      BMS_UART_Address = 0;
uint8_t      BMS_UART_Command = 0;

uint8_t      BMS_UART_Error = 0;
enum {
    BMS_UART_ERROR_NO_AA = 1,
    BMS_UART_ERROR_NO_01,
    BMS_UART_ERROR_CHECKSUM,
};

// State machine, in all its glory
void BMS_UART_StateMachine_Rx(uint8_t rx)
{
    BMS_UART_State = BMS_UART_State(rx);
}

// Implementation of individual states
static void *wait_55(uint8_t rx)
{
    if (rx == 0x55)
        return wait_AA;
    else
        return wait_55;
}

static void *wait_AA(uint8_t rx)
{
    if (rx == 0xAA)
        return wait_len;
    else {
        BMS_UART_Error = BMS_UART_ERROR_NO_AA;
        return in_error;
    }
}

static void *wait_len(uint8_t rx)
{
    BMS_UART_Length = rx;
    return wait_addr;
}

static void *wait_addr(uint8_t rx)
{
    BMS_UART_Address = rx;
    return wait_01;
}

static void *wait_01(uint8_t rx)
{
    if (rx == 0x01) {
        return wait_cmd;
    } else {
        BMS_UART_Error = BMS_UART_ERROR_NO_01;
        return in_error;
    }
}

static void *wait_cmd(uint8_t rx)
{
    BMS_UART_Command = rx;
    BMS_UART_Datasum = BMS_UART_Length + 0x26 + BMS_UART_Command;
    BMS_UART_Checksum = 0;
    BMS_UART_Have = 0;

    if (BMS_UART_Length == 0xCA && BMS_UART_Address == 0xFE && BMS_UART_Command == 0x64) {
        // Command 0x64, address 0xFE, length 0xCA is handled by function NULL:
        BMS_UART_Handler = NULL;
        return wait_data;

    } else
    if (BMS_UART_Length == 0xB0 && BMS_UART_Address == 0x0B && BMS_UART_Command == 0x1E) {
        // Command 0x1E, address 0x0B, length 0xB0 is handled by function NULL:
        BMS_UART_Handler = NULL;
        return wait_data;

    } else {
        // Everything else is skipped.
        return wait_skip;
    }
}

static void *wait_skip(uint8_t rx)
{
    if (BMS_UART_Have < BMS_UART_Length) {
        BMS_UART_Datasum += rx;
        BMS_UART_Have++;
        return wait_skip;
    } else {
        BMS_UART_Checksum = rx;
        BMS_UART_Datasum = 0xFFFF - BMS_UART_Datasum;
        return wait_skipcheck;
    }
}

static void *wait_skipcheck(uint8_t rx)
{
    BMS_UART_Checksum = (BMS_UART_Checksum << 8) | rx;
    if (BMS_UART_Checksum == BMS_UART_Datasum) {
        // No error, but we ignored this command.
        return wait_55;
    } else {
        BMS_UART_Error = BMS_UART_ERROR_CHECKSUM;
        return in_error;
    }
}

static void *wait_data(uint8_t rx)
{
    if (BMS_UART_Have < BMS_UART_Length) {
        BMS_UART_Datasum += rx;
        BMS_UART_Data[BMS_UART_Have++] = rx;
        return wait_data;
    } else {
        BMS_UART_Checksum = rx;
        BMS_UART_Datasum = 0xFFFF - BMS_UART_Datasum;
        return wait_check;
    }
}

static void *wait_check(uint8_t rx)
{
    BMS_UART_Checksum = (BMS_UART_Checksum << 8) | rx;
    if (BMS_UART_Checksum == BMS_UART_Datasum) {
        if (BMS_UART_Handler)
            BMS_UART_Handler();
        return wait_data;
    } else {
        BMS_UART_Error = BMS_UART_ERROR_CHECKSUM;
        return in_error;
    }
}

// Error state
static void *in_error(uint8_t rx)
{
    if (rx == 0x55) {
        return wait_AA;
    } else {
        return in_error;
    }
}


Take a note at the wait_cmd() function.  In it, you detect if the command, address, and length is something you are interested in, and if so, assign a handler function to be called when it is received with a matching checksum.  Otherwise, the data is ignored.

I'm not sure I like this over the switch (state) { ... } idiom, though.

The key feature of this state machine is that it detects errors, and has a dedicated error state (from which it will continue when a valid 0x55 0xAA sequence is received).  And, of course, that it does not store all data, only data from commands you want to support, while still handling all commands of the stated form correctly (and even checking their checksum).
Obviously, you do not need to use function pointers to implement this exact state machine.
 

Online Siwastaja

  • Super Contributor
  • ***
  • Posts: 6115
  • Country: fi
Re: Parsing UART packets / state machines / packed structs
« Reply #31 on: Yesterday at 08:48:14 am »
Any platform I've ever used in C from 8-bit to 64-bit had compilers allowing struct packing. The only problem to be aware of is again alignment, which may be a problem on some platforms for accessing certain ill-aligned struct members directly. That's the case in particular for the 16-bit PIC MCUs for accessing 16-bit or 32-bit struct members. They need to be aligned.
But, once again, with the packed attribute, compiler uses correct instructions to access unaligned 16- or 32-bit member; e.g. 8-bit loads and shifting + orring to combine the result into a register. Just like what you do manually in C by writing val = (uint32_t)in[1]<<24 | ... and so on. This is obviously slower than aligned loads, and the reason not to pack structs "just because".

It only becomes a problem if pointer to a member is used, but then the compiler will warn you, as I found out, even with the minimal default warnings on GCC. Casting the pointer to the whole packed struct is fine because compiler knows the struct is packed and does not assume any alignment.

But, when unsure, use unions instead of pointer casting. Then the language itself guarantees alignment, and nothing can go wrong. By thinking about your datatypes and all their intended uses, you get almost completely rid of casting buffers into something else. Especially if you don't have to deal with libraries.
« Last Edit: Yesterday at 08:52:22 am by Siwastaja »
 

Offline westfw

  • Super Contributor
  • ***
  • Posts: 3904
  • Country: us
Re: Parsing UART packets / state machines / packed structs
« Reply #32 on: Yesterday at 09:43:22 am »
Quote
the language itself guarantees alignment
While I'm a big fan of mapping packets unto structures, it is possible to end up with the start of your packet at an unaligned address due to encapsulation or options or whatever coming before it.  In which case you may need to use accessor functions/macros that are not more complicated than using similar functions all the time, but (IMO) more readable.
"src = GETLONG(pak->ipsrcaddr)" vs "src = get_ip_srcaddr(pak)"

OTOH, there's IP's "Fragment Offset", which is substantially inconvenient on little-endian CPUs. :-)
 

Offline Nominal Animal

  • Super Contributor
  • ***
  • Posts: 4472
  • Country: fi
    • My home page and email address
Re: Parsing UART packets / state machines / packed structs
« Reply #33 on: Yesterday at 01:13:14 pm »
"src = GETLONG(pak->ipsrcaddr)" vs "src = get_ip_srcaddr(pak)"
Why not
Code: [Select]
// Generic accessors
static inline uint_fast16_t  get_u16be(const unsigned char *src)
{
    return ((uint_fast16_t)(src[0]) << 8) | src[1];
}

static inline uint_fast32_t  get_u32be(const unsigned char *src)
{
    return ((uint_fast32_t)(src[0]) << 24)
         | ((uint_fast32_t)(src[1]) << 16)
         | ((uint_fast32_t)(src[2]) << 8)
         | src[3];
}

// Dedicated accessors
static inline uint32_t  get_ipv4_length(const void *gram)
{
    return get_u16be((unsigned char *)gram + 2);
}

static inline uint32_t  get_ipv4_srcaddr(const void *gram)
{
    return get_u32be((unsigned char *)gram + 24);
}

static inline uint32_t  get_ipv4_dstaddr(const void *gram)
{
    return get_u32be((unsigned char *)gram + 28);

}
where the inline is just a reminder to us humans that these are accessor functions intended to be put into a header file?

The generic accessor is named according to the data type, whereas the dedicated accessor is named after the logical field, and internally "encodes" the size, type, and offset of that field.

On x86-64, where unaligned accesses are supported, get_ipv4_srcaddr() compiles to just one mov and a bswap.  On ARM Thumb, to one ldr and a rev.  On RISC-V, it generates a bit more code, but no more than any unaligned 32-bit load would.

There is only a small conceptual jump into memory-safe accessor primitives from here, too.
 
The following users thanked this post: nctnico

Online nctnico

  • Super Contributor
  • ***
  • Posts: 24198
  • Country: nl
    • NCT Developments
Re: Parsing UART packets / state machines / packed structs
« Reply #34 on: Yesterday at 02:04:12 pm »
Any platform I've ever used in C from 8-bit to 64-bit had compilers allowing struct packing. The only problem to be aware of is again alignment, which may be a problem on some platforms for accessing certain ill-aligned struct members directly. That's the case in particular for the 16-bit PIC MCUs for accessing 16-bit or 32-bit struct members. They need to be aligned.
And ARM platforms have a habit of needing alignment as well. I have run into the situation where an Atmel SOC would silently corrupt unaligned data. And this was due to mapping a type directly onto the buffer.

Quote
As to endianness, yeah it's not a big deal. If you make your own protocols don't bother with big-endian as it's pretty rare nowadays on processors. Buf if you have to deal with it, you can just swap bytes "in place" as NorthGuy said, for the members that require it. Still usually eons simpler, faster and more readable than hand-decoding packets byte per byte.
I never suggested hand decoding packet byte-per-byte. Use get / set routines for specific field types that work with offsets. In my experience that is much more practicle and less error prone than putting a struct together because -again- fields in a protocol are typically specified with an offset and size. These get / set routines can take care of byte swapping which then wraps up everything that deals with hardware at the lowest level possible. IMHO is it utterly nuts that the BSD socket API requires the application layer to deal with endianess. That should have been handled at a much lower level.

Also keep in mind some protocols may have messages that have different fields depending on the message type so one struct typically isn't enough. Another advantage of not mapping structs is that you can use a window that moves over the incoming data without needing to care about alignment. Especially with protocols that come in over continuous streams (like UART and TCP/IP) you don't know the start & end of a packet; it makes more sense to use the receive buffer to examine the data 'in situ' rather than copying something that could be a packet but may be garbage.

This may be drivel to some but I have implemented a crapload of protocols over the years and know very well what works and what doesn't.
« Last Edit: Yesterday at 02:06:08 pm by nctnico »
There are small lies, big lies and then there is what is on the screen of your oscilloscope.
 

Online NorthGuy

  • Super Contributor
  • ***
  • Posts: 2885
  • Country: ca
Re: Parsing UART packets / state machines / packed structs
« Reply #35 on: Yesterday at 03:08:04 pm »
Another advantage of not mapping structs is that you can use a window that moves over the incoming data without needing to care about alignment. Especially with protocols that come in over continuous streams (like UART and TCP/IP) you don't know the start & end of a packet; it makes more sense to use the receive buffer to examine the data 'in situ' rather than copying something that could be a packet but may be garbage.

That is because you have used an inappropriate buffering method. If you use a circular buffer then everything gets misaligned and the buffer may wrap at any point, so the data can only be accessed byte by byte. Accessor functions create an illusion of structural approach, but do not solve the problem - they only add bloat to hide the ugliness.

Instead, create a number of aligned buffers (one, two, or many) where you can collect the "would-be-packets" from UART, then, when the packet is confirmed, pass the pointer to the buffer further for processing. Once the contents of the packet is identified pass a pointer the specific data structure within the packet to analyze the contents.
 

Online Siwastaja

  • Super Contributor
  • ***
  • Posts: 6115
  • Country: fi
Re: Parsing UART packets / state machines / packed structs
« Reply #36 on: Yesterday at 03:15:19 pm »
Any platform I've ever used in C from 8-bit to 64-bit had compilers allowing struct packing. The only problem to be aware of is again alignment, which may be a problem on some platforms for accessing certain ill-aligned struct members directly. That's the case in particular for the 16-bit PIC MCUs for accessing 16-bit or 32-bit struct members. They need to be aligned.
And ARM platforms have a habit of needing alignment as well. I have run into the situation where an Atmel SOC would silently corrupt unaligned data. And this was due to mapping a type directly onto the buffer.
... which (the type) clearly was not a packed struct.

The title here says: "packed structs".

Case closed. Can we now go forward or do we need to continue this broken record game?

Quote
Also keep in mind some protocols may have messages that have different fields depending on the message type so one struct typically isn't enough
Which is why I suggested unions.

Quote
it makes more sense to use the receive buffer to examine the data 'in situ' rather than copying something that could be a packet but may be garbage.

Which is why I suggested instead of copying, using an union so that the same buffer type can be accessed as the struct.

Quote
This may be drivel to some but I have implemented a crapload of protocols over the years and know very well what works and what doesn't.

Clearly you have. Sometimes it is a good idea to look at new, modern ideas that improve the code. But if you refuse to even read the messages you reply to, and instead build a strawman against any idea except of your own, then we can do nothing to help.
« Last Edit: Yesterday at 03:21:08 pm by Siwastaja »
 

Online nctnico

  • Super Contributor
  • ***
  • Posts: 24198
  • Country: nl
    • NCT Developments
Re: Parsing UART packets / state machines / packed structs
« Reply #37 on: Yesterday at 03:37:13 pm »
Combining a struct in a union won't help if the buffer itself isn't aligned (for example when moving a 'window' over a buffer). And mapping a type or struct onto a buffer is the same thing. It will break when alignment is required. It is that simple. And this has nothing to do with being 'modern' or something like that (C compilers have been supporting packed structs since the dawn of time) but it is all about creating robust, portable code that works as expected no matter what. Even in the hands of less experienced engineers.

Actually, you have to think about whether packed structs have been designed to be used on dealing with external protocols. Saving memory at the expense of speeds seems like a very reasonable explaination to me. Especially since you can also pack bitfields.
« Last Edit: Yesterday at 06:58:01 pm by nctnico »
There are small lies, big lies and then there is what is on the screen of your oscilloscope.
 

Online Siwastaja

  • Super Contributor
  • ***
  • Posts: 6115
  • Country: fi
Re: Parsing UART packets / state machines / packed structs
« Reply #38 on: Yesterday at 07:09:31 pm »
Portability no matter what? Sure, that's a valid reason, but it's not the only one.

If you want portability so that the MCU could one day be replaced with a big endian MCU, without code needing to be modified at all, then sure, write wrappers with both endianness options from day one. You need to do all the same work, just proactively.

(I have actually encountered one big endian machine lately, namely Teltonika RUT955 embedded PC. Interfacing it with ARM microcontrollers required taking care of endianness on one of the ends - the Teltonika. I still used structs, and no, there was nothing magical involved.)

These are design choices to make. In my MCU projects, these are not factors. Code which I write for Cortex-M4 for example, relies on some other Cortex-M features such as interrupt handlers being regular functions. I also rely on it staying little endian - and I also rely on using a compiler with packed struct available. All ARM CM capable compilers are such, have always been. I think it's a safe assumption no one wants to suddenly compile my commercial projects on some specialized scientific or toy compiler.

Life is such choices. And it has absolutely nothing to do with "robustness". A train can be robustly designed and built even if it needs certain width of tracks and cannot fly. These are limitations, and not some arbitrary limitations, but well thought out ones. Every project has those.

And I happen to be 110% sure the uer166 guy:
* will always use a compiler with packed attribute
* will only use little endian CPUs in his BMS communication

Worst case? When the unimaginable happens and uer166 directly connects his BMS to Teltonika router, then add endianness wrappers. Same work, but Just-In-Time.
« Last Edit: Yesterday at 07:15:32 pm by Siwastaja »
 

Online NorthGuy

  • Super Contributor
  • ***
  • Posts: 2885
  • Country: ca
Re: Parsing UART packets / state machines / packed structs
« Reply #39 on: Yesterday at 07:25:01 pm »
You usually don't need to use packed structs. Most of the protocols will use struct fields which are naturally aligned. There may be padding added to ensure this, which you explicitly add to your struct where needed. Thus, once you ensure that the beginning of the struct is aligned, the rest of the fields get aligned automatically.
 
The following users thanked this post: Siwastaja

Online Siwastaja

  • Super Contributor
  • ***
  • Posts: 6115
  • Country: fi
Re: Parsing UART packets / state machines / packed structs
« Reply #40 on: Yesterday at 07:35:57 pm »
You usually don't need to use packed structs. Most of the protocols will use struct fields which are naturally aligned. There may be padding added to ensure this, which you explicitly add to your struct where needed. Thus, once you ensure that the beginning of the struct is aligned, the rest of the fields get aligned automatically.
Well designed protocols do this for performance. You need to remember the ages old good trick, which is still useful: when the order of fields does not matter for you (and usually it does not), just order the fields from biggest to smallest (e.g. floats go first, chars last), for least amount of padding.

Then, for protocols, you can add padding yourself. The advantage over compiler-generated padding is also that these padding fields become natural "reserved" fields which allows you to extend the protocol later.

But I'm sure this is somehow wrong, too, definitely not professional and robust :-//
 

Online Sherlock Holmes

  • Regular Contributor
  • *
  • Posts: 176
  • Country: us
Re: Parsing UART packets / state machines / packed structs
« Reply #41 on: Yesterday at 07:54:26 pm »
You usually don't need to use packed structs. Most of the protocols will use struct fields which are naturally aligned. There may be padding added to ensure this, which you explicitly add to your struct where needed. Thus, once you ensure that the beginning of the struct is aligned, the rest of the fields get aligned automatically.
Well designed protocols do this for performance. You need to remember the ages old good trick, which is still useful: when the order of fields does not matter for you (and usually it does not), just order the fields from biggest to smallest (e.g. floats go first, chars last), for least amount of padding.

Then, for protocols, you can add padding yourself. The advantage over compiler-generated padding is also that these padding fields become natural "reserved" fields which allows you to extend the protocol later.

But I'm sure this is somehow wrong, too, definitely not professional and robust :-//

Perhaps such field ordering could be done with greater flexibility, I agree that the "largest" first is a sensible strategy sometimes but .Net does that automatically, one has to take special steps to order them oneself.

“When you have eliminated all which is impossible, then whatever remains, however improbable, must be the truth.” ~ Arthur Conan Doyle, The Case-Book of Sherlock Holmes
 

Online SiliconWizard

  • Super Contributor
  • ***
  • Posts: 10665
  • Country: fr
Re: Parsing UART packets / state machines / packed structs
« Reply #42 on: Yesterday at 08:27:33 pm »
Any platform I've ever used in C from 8-bit to 64-bit had compilers allowing struct packing. The only problem to be aware of is again alignment, which may be a problem on some platforms for accessing certain ill-aligned struct members directly. That's the case in particular for the 16-bit PIC MCUs for accessing 16-bit or 32-bit struct members. They need to be aligned.
But, once again, with the packed attribute, compiler uses correct instructions to access unaligned 16- or 32-bit member; e.g. 8-bit loads and shifting + orring to combine the result into a register. Just like what you do manually in C by writing val = (uint32_t)in[1]<<24 | ... and so on. This is obviously slower than aligned loads, and the reason not to pack structs "just because".

I have certainly run into compilers (xc16 here, don't remember the version, but it's based on GCC although the port is handled by Microchip and never made it to mainline GCC, so it's possibly very not on par with what GCC is supposed to be doing) that wouldn't automatically handle unaligned access in the way you describe, and accessing unaligned struct members would crash the CPU.

Given that the "packed" attribute AFAIK is compiler-specific and not standard C, you're of course not guaranteed any of this anyway. Let me know if this has made it to the standard though. Maybe C23, as I think they finally introduced attributes?

That said, I checked on godbolt using Clang and GCC and various targets and your statement seems to hold, so I'm gonna assume that it should be safe if you use a reasonably recent version with a reasonably well supported target. I'm again not too sure about 16-bit PICs here, but xc16 is kind of a nasty beast.
 
The following users thanked this post: Siwastaja

Offline Nominal Animal

  • Super Contributor
  • ***
  • Posts: 4472
  • Country: fi
    • My home page and email address
Re: Parsing UART packets / state machines / packed structs
« Reply #43 on: Today at 04:38:23 am »
I use both (packed) structures and (character) buffer accessors.

The structures work really well with fixed structures, like say PNG files.  When the actual layout depends on the content, the buffer accessors work much better than trying to use structures.  IPv4 datagrams are on the borderline, because the first 20 bytes of the header are fixed, and only options may vary; so I might use a packed structure (as all C compilers I use do support those) for the first 20 bytes, but accessors for the options.

I do not understand the argument here, because in my opinion, the two do not overlap that much.

I use packed structures with fixed data structures when I have ensured they work and documented the need for them to work, and buffer accessors for varying data structures and when packed structures may not be available.

Even when using structures with associated data, I tend to use the struct fields through accessors when the field deals with things like data length to ensure memory safety: that through normal use of the interface, one will not accidentally access past the known/received data, because the content happens to be buggy or malicious.

This checking is important.  The recent CVE-2022-41674 is a good example.  Code did not check that the MaxBSSID field in a WiFi packet had a sensible value (between 1 and 8, inclusive), so a buffer overrun ensued.  Oopsie.

Thus, even when using a packed struct, you should never trust its contents to be safe; and to make sure, we use helper accessors to do the checking for us.  (Well, not for pure data fields, those that just state some fact, but do not affect the layout or access to other data.)

In other words, the one wrong use case of packed structs is to avoid using accessor functions.  The two are not mutually exclusive.



For worry-free binary data stream protocols, I use prototype binary representations of known values as both the identifier as well as byte order indicator, so that the data generator does not need to care about the byte order at all.  On the host computer, the datastream can be trivially 'normalized' to local-endian, or kept as-is.  To detect the byte order, one simply tests the byte order permutations (including 'none'/'as-is') that yield a recognized value.  For example, for IEEE 754 Binary32 'float' type, one can use 0x3F2C1880 = 0.67224884f.

For structures, on a computer, the "packed" datastream can easily be expanded into a stream/array of native structures.  It is an extra copy, yes, but it turns out that due to the cache behaviour of current machines, doing it as soon as the data is read or received, is actually very, very cheap in terms of CPU or wall time used, because on typical computers we tend to be memory bandwidth limited, and not at all CPU limited.  Even in Python (when interfacing to microcontrollers) this makes sense –– for example, one should definitely convert an incoming bytes stream into NumPy.arrays via struct.unpack() for manipulation.  (On the prototype part, just try the different byte order prefixes on the struct formats, until the values match the expected, and then instantiate that using struct.Struct for mass unpacking.)

I really like the prototype approach, because not only does a single value (of each type used) ensure byte order, but they also act like protocol and format identifiers, 'magic pattern', in the Unix file command magic sense.
« Last Edit: Today at 04:40:45 am by Nominal Animal »
 
The following users thanked this post: Siwastaja, eutectique

Online Siwastaja

  • Super Contributor
  • ***
  • Posts: 6115
  • Country: fi
Re: Parsing UART packets / state machines / packed structs
« Reply #44 on: Today at 08:32:20 am »
In other words, the one wrong use case of packed structs is to avoid using accessor functions.
One could also claim that having one accessor function per member for validation is not always the best strategy. Sometimes validity is in combination of variables, for example: assert(a<10 && b<10 && (a+b)<15).

I like to do the sanity check of the len field in the protocol level handler (which is agnostic to the payloads), but when len and CRC check out and thus non-corrupted data is received (leaving logical mistakes / bugs at the sender), I use a handler function which gets the whole payload type struct, and verifies the sanity of data fields "as it goes", in the same way one usually checks function input arguments.

In fact, I don't think there is any fundamental difference in verifying the input values in a "normal" function in a program, or input values coming in a struct that was received through UART.

But as is obvious to most of sensible programmers, different paradigms work for different problems.
« Last Edit: Today at 08:46:29 am by Siwastaja »
 

Offline Nominal Animal

  • Super Contributor
  • ***
  • Posts: 4472
  • Country: fi
    • My home page and email address
Re: Parsing UART packets / state machines / packed structs
« Reply #45 on: Today at 09:42:40 am »
In other words, the one wrong use case of packed structs is to avoid using accessor functions.
One could also claim that having one accessor function per member for validation is not always the best strategy. Sometimes validity is in combination of variables, for example: assert(a<10 && b<10 && (a+b)<15).
I agree.  It is equally valid to just verify all the fields at some point.

In my own code, when the structures are accessed randomly –– so, a tree or other structured data thingy –– I sometimes unpack and validate the structures at reception/reading, because that tends to have the best cache footprint and I/O thoroughput.  Idea being, since we touch the data anyway, we could make it cheaper to access.

Similarly, when receiving data via an UART, I prefer methods that update checksums and calculate command hashes on the fly, online.

In fact, I don't think there is any fundamental difference in verifying the input values in a "normal" function in a program, or input values coming in a struct that was received through UART.

So much of the code I write is centered around working with structured data, that I find the idea that verifying input could be considered somehow special or different, completely bonkers.

I like to think of all data suspicious, because that way any oddity or issue in processing that cannot be compensated becomes an issue for the user to deal with.  I absolutely hate programs that silently corrupt data, because "those errors occur so rarely it is not worth it to check for them", because with my luck, it is my most precious data that gets silently corrupted.  No, I want programs to do their best, but also tell me if something odd or unexpected happened.

In an embedded appliance, especially microcontrollers, it can be a difficult user interface question to decide how to handle communications errors.  But, if you start with "I'll worry about that later", then you basically decide to handle them randomly, because that kind of functionality is a design issue, and not something that can be bolted on top later on.  It has never been succesfully fixed later on, only semi-patched with ugly kludges.
Security is like that: it is either designed in, or it does not exist.
 
The following users thanked this post: SiliconWizard

Online SiliconWizard

  • Super Contributor
  • ***
  • Posts: 10665
  • Country: fr
Re: Parsing UART packets / state machines / packed structs
« Reply #46 on: Today at 08:09:04 pm »
Key advice.
Validate your inputs.

A gigantic number of bugs, crashes (that can lead to death) and security exploits have come, come and will come from not validating inputs.

Even a very large number of memory-related bugs are linked to not properly validated inputs. That's the case for - I'd dare say - most buffer overflows.

Yet it looks like most quality-oriented discussions and programming language features these days obsess over memory access and management, and almost nothing about input validation (and more generally invariant checks, and "contracts".) That's probably because we tend to focus on adding safety nets rather than on prevention, and that doesn't just apply to software.
 


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf

 



Advertise on the EEVblog Forum