Indeed this is not a good design since it doesn't use private encapsulation. It is using a global variable which means it is pretty much by definition spaghetti code.

I don't see why you would use callbacks in this case either, since the copying of data is no business of the program outside the driver (unlike lets say a timer driver where it can make perfect sense).

For a normal project you'll probably want to separate everything in 3 separated parts:

• The raw "dumb" reception of data through a MCU-specific hardware peripheral. uart.h + uart.c
• The protocol decoder that knows about the nature of the data - the protocol, but not necessarily anything about the specific hardware peripheral driver. Either it calls the driver to ask for data, or data is passed between the driver and the protocol handler by a middle man ie some application-tier code from main().
• The application that uses the payload of the packet but otherwise doesn't care less about the protocol format, let alone the underlying UART hardware.

For certain smaller projects or hard real-time projects, you might want to merge the driver and protocol handling into one though. In case you want to instantly reject something upon reception, or instantly act on something etc. So in hard real-time projects you might want to decode the protocol on the fly as it is getting received.

As for how to design the driver and ring buffer:

• The interrupt and ISR should be internal to the driver uart.c and how it moves data from hardware buffers to RAM is no business of the rest of the code. That's an entirely internal affair inside the UART driver.

  Depending on system, other parts of the program may need to register the ISR in a vector table etc, but that's about it. ARM systems typically solve this by having default ISRs with "weak linkage", meaning that if you define a ISR with the same name then it takes precedence and that's resolved at link-time. Then everything about the ISR can be encapsulated. You should document that the driver comes with a live ISR though.

• The UART driver can declare the ring buffer type internally or otherwise include a separate ring buffer ADT. It is common that a ring buffer struct knows specifics about the hardware peripheral so that the safe access of data shared with an ISR can be handled from the ring buffer itself.

  For example if you know the baudrate in advance then you know how long a byte transmission takes and there's probably plenty of time to copy that byte until the next one arrives, meaning that you can temporarily disable interrupts while copying in order to protect against race conditions.

  An example of a generic ring buffer from an old project of mine, where the ring buffer has the possibility to set/clear interrupt flags through int_mask in an 8-bit register int_reg:

  typedef struct
  {
    void*              buffer;
    uint16_t           objects_n;
    uint16_t           object_size;
    uint16_t           object_count;
    uint8_t*           begin;
    uint8_t*           end;
    volatile uint8_t*  int_reg;
    uint8_t            int_mask;
    uint8_t*           end_of_buf;
  } rb_buf_t;
  

  This is rather over-engineered though, you might not need to make it this advanced and type-generic - this is from a gateway type of project that handled multiple UART and CAN peripherals all at once.

  But you should probably at least consider storing the size of valid data in the ring buffer struct, so that you can grab the size quickly, and so that you can tell the difference between a completely full and completely empty buffer.

• From inside uart.c you have a place in RAM where the received data is stored. Either it is the ring buffer, or it could be another buffer not connected to the ISR ("double-buffering"), if you for example want to separate the packet getting received right now from the previously complete packet.

  You can chose to expose this internal RAM buffer/ring buffer to the caller, but that should be a read-only access then, since no other part of the program has any business writing to the UART rx buffer.

• If you don't solve race condition bugs by enabling/disabling the specific interrupt, then yeah you can do it with a "poor man's semaphore" ie static volatile bool. Poor as in: in itself it is not guaranteed to be thread-safe/interrupt safe like the higher level concept of semaphores in an OS. No access in C code is otherwise ever atomic unless you actually use C11 atomic types.

  The idea behind this is that the access to the "poor man's semaphore" bool won't be atomic either, it might get interrupted right in the middle of updating the value. But we don't care, the data is either 1 or 0 and so data corruption isn't going to happen. We only care if the data transfer we are trying to protect gets interrupted while updating. The poor man's semaphore is always either on/off and at that point we can access the data. Basically we move the race condition problem away from the data to this bool, where it doesn't cause any harm. volatile variable access is not allowed to be re-ordered either.

  Some more info about volatile and "poor man's semaphore" here: Using volatile in embedded C development (In fact that answer probably answers this whole question in itself?)

• DMA is often far less of a head ache to work with than this "old school" interrupt approach, and it is not nearly as interrupt-intense. So if your MCU supports DMA then give it some serious consideration.

posted 7 days ago

CC BY-SA 4.0

7d ago

Lundin‭

10542 reputation 48 146 1227 200