Code review part:

Design (important!)

• Global variables/external linkage are to be avoided (Why is global evil?).

• You don't actually have private encapsulation in this code since the internals of the struct are exposed to the caller. There are better ways to design this with "opaque types" and private encapsulation, see How to do private encapsulation in C? As a bonus, such a design allows for multiple instances of each object instead of "singleton" which is what you have currently.

  In embedded systems, multiple instances can for example be convenient when you have multiple hardware peripherals in a MCU, all behaving the same.

• It's a contrived design that both type1 and type2 seem to know about each other and share the same members, and that they know of the interface. This is not a proper program design, it's just 3 files glued together with tight coupling between them. type1 shouldn't know about type2 and vice versa. The interface, which I suppose is equivalent of a hardware abstraction layer (HAL) in this case, should know about which type1, type2 etc it uses but the user of the interface need not know/care about that.

  The program dependencies should go like:
  main.c uses interface, which contains type1 and type2

Performance/memory

• Type ought to have been an enum instead, unless you have very good reasons to avoid them on the target system (ie 8-bitter archaic MCU or the like).
• Using function pointers in a struct as means of inheritance is not necessarily wrong, but a bit clunky. They are hard for the compiler to optimize and adding them to every struct wastes a bit of RAM. We could as well have ordinary functions taking a struct member as parameter, the syntax is a bit different but the meaning is the same.

Style & best practices

• The variable names could be better than "type1", "type2" etc but I take it this is just a simplified, artificial example.
• Similarly, stdio.h is to be avoided in (embedded) systems but I take it this is just a simplification as well.
• It's a good idea to use source code prefixes for your code, so if you have a module hwinterface.h, then you could prefix all identifiers belongint to that one as hwinterface or maybe hwif if that's too long. So don't name a function init, name it hwinterface_init.
• Avoid pointless comments like // INITIALIZATION when commenting the line init(). That's already self-documenting code, the comment fills no purpose.

C language issues

• Note that a function declared in C as void func() is a function accepting any parameter. This is inconvenient for type safety reasons. And besides, this is getting changed in the upcoming C23 standard where empty parenthesis will become equivalent to (void), just like C++ has always worked.
• Identifiers beginning with an underscore followed by a capital letter are reserved for the compiler and standard library. So don't name header guards _INTERFACE_H.

Alternative design (draft)

Ok so I cooked up an alternative design. The program does nonsense stuff, but it is more an example of how you can use opaque type and private encapsulation. I also wrote a simple memory pool for shared static allocation, which may or may not be desired in an embedded application. I'll post each source file then comment on it below each file.

//hwinterface.h

#ifndef HW_INTERFACE_H
#define HW_INTERFACE_H

#include <stdbool.h>
#include <stdint.h>

#include "mempool.h"
#include "type1.h"
#include "type2.h"

typedef union hwinterface hwinterface;

typedef enum
{
  HW_ANALOG,
  HW_DIGITAL,
} hw_t;

hwinterface* hwinterface_init (hw_t hw, int stuff1, int stuff2);

void hwinterface_run (hwinterface* hwi);

#endif

• Bit of renaming and clean-up, source code prefixes, added an enum etc etc.
• typedef union hwinterface hwinterface; This is a forward declaration of an opaque type. I'll explain why I used a union further below.
• hwinterface_init This is a constructor and it uses allocation internally (which isn't a must). It creates a type1 and type2 object internally but the caller doesn't really need to care. Apart from the enum, I made up some nonsense input parameters.
• hwinterface_run is some function to call cyclically.

// hwinterface.c
#include "hwinterface.h"

union hwinterface
{
  struct
  {
    hw_t hw;
    type1* t1;
    type2* t2;
  };
  uint8_t raw [sizeof(hw_t) + sizeof(type1*) + sizeof(type2*)];
};

hwinterface* hwinterface_init (hw_t hw, int stuff1, int stuff2)
{
  hwinterface* hwi = (hwinterface*) mempool_alloc(sizeof(hwinterface));
  if(hwi == NULL)
    for(;;){} // error handling here

  hwi->hw = hw;

  hwi->t1 = type1_alloc();
  if(hwi->t1 == NULL)
    for(;;){} // error handling here
  type1_set_stuff(hwi->t1, stuff1);

  hwi->t2 = type2_alloc();
  if(hwi->t2 == NULL)
    for(;;){} // error handling here
  type2_set_stuff(hwi->t2, stuff2);

  return hwi;
}

void hwinterface_run (hwinterface* hwi)
{
  if(hwi->hw == HW_DIGITAL)
  {
    int stuff1 = type1_get_stuff(hwi->t1);
    int stuff2 = type2_get_stuff(hwi->t2);
    type1_set_stuff(hwi->t1, (bool)stuff1);
    type2_set_stuff(hwi->t2, (bool)stuff2);
  }

  type1_run(hwi->t1);
  type2_run(hwi->t2);
}

• The union looks a bit peculiar here, but it is made this way in order to utilize a memory pool without invoking undefined behavior. The mempool_alloc function will return an uint8_t* and the only way we can safely type pun that one into a larger type is by using a type where an uint8_t[] array exists as one of the members. Normally the (hwinterface*) would have been very fishy otherwise.
• hwinterface_init allocates an instance of the interface itself, then call constructors of the type1 and type2, all by using the shared memory pool (which again is totally optional).
• hwinterface_run does some nonsense thing by translating the values obtained by each of type1 and type2 to 1 or 0 in case of HW_DIGITAL. This isn't important.

// mempool.h
#ifndef MEMPOOL_H
#define MEMPOOL_H

#include <stdint.h>
#include <stddef.h>

#define MEMPOOL_SIZE  96
#define MEMPOOL_ALIGN  4

void mempool_init (void);

uint8_t* mempool_alloc (size_t bytes);

#endif // MEMPOOL_H

Very straight-forward and simple memory pool. You can set a maximum size and a normal alignment if applicable (otherwise 1).

// mempool.c
#include "mempool.h"

static uint8_t mempool [MEMPOOL_SIZE];
static uint8_t mempool_used;

void mempool_init (void)
{
  mempool_used = 0;
}

uint8_t* mempool_alloc (size_t bytes)
{
  uint8_t* result;

  uint8_t remain = bytes % MEMPOOL_ALIGN;
  if(remain)
  {
    bytes += MEMPOOL_ALIGN - remain;
  }

  if(mempool_used + bytes > MEMPOOL_SIZE)
    return NULL;

  result = &mempool[mempool_used];
  mempool_used += bytes;
  return result;
}

This just comes with some crude alignment handling and returning a pointer into a memory pool, if there is room. In a real-world application we have to make sure that mempool is aligned correctly, through some non-standard compiler extension or linker script. Basically this is just to get rid of dynamic allocation, which shouldn't be used in embedded systems. Why should I not use dynamic memory allocation in embedded systems?

We should note that there are two serious problems when trying to allocate a larger type onto a byte array like this. One is the mentioned alignment, the other is the "strict aliasing rules". I won't get into that here, but with by union trick we dodged strict aliasing too. It is still highly recommended to compile using -fno-strict-aliasing on all gcc-like compilers, particularly when doing embedded systems.

// type1.h
#ifndef TYPE1_H
#define TYPE1_H

#include <stdint.h>
#include <stdio.h>
#include "mempool.h"

typedef union type1 type1;

type1* type1_alloc (void);

int  type1_get_stuff (const type1* t1);
void type1_set_stuff (type1* t1, int stuff);

void type1_run (type1* t1);

#endif // TYPE1_H

Generic class implementation with some nonsense setter/getter functions. I made type1 and type2 100% identical apart from the name, though in a real application these would naturally be different, meaningful things.

// type2.h
#ifndef TYPE2_H
#define TYPE2_H

#include <stdint.h>
#include <stdio.h>
#include "mempool.h"

typedef union type2 type2;

type2* type2_alloc (void);

int  type2_get_stuff (const type2* t2);
void type2_set_stuff (type2* t2, int stuff);

void type2_run (type2* t2);

#endif // TYPE2_H

Same deal as type1.

// type1.c
#include "type1.h"

static void type1_print (const type1* t1);


union type1
{
  struct
  {
    int stuff;
  };

  uint8_t raw [sizeof(int)];
};


type1* type1_alloc (void)
{
  type1* t1 = (type1*) mempool_alloc(sizeof(type1));
  if(t1 == NULL)
    for(;;){} // error handling here

  t1->stuff = 0;

  return t1;
}

int type1_get_stuff (const type1* t1)
{
  return t1->stuff;
}

void type1_set_stuff (type1* t1, int stuff)
{
  t1->stuff = stuff;
}

void type1_run (type1* t1)
{
  type1_print(t1);
}

static void type1_print (const type1* t1)
{
  printf("%s %d\n", __func__, t1->stuff);
}

Very similar to the hwinterface opaque type. Some setter/getter functions. A private function for printing. type2.c will be identical

// type2.c
#include "type2.h"

static void type2_print (const type2* t2);


union type2
{
  struct
  {
    int stuff;
  };

  uint8_t raw [sizeof(int)];
};


type2* type2_alloc (void)
{
  type2* t2 = (type2*) mempool_alloc(sizeof(type2));
  if(t2 == NULL)
    for(;;){} // error handling here

  t2->stuff = 0;

  return t2;
}

int type2_get_stuff (const type2* t2)
{
  return t2->stuff;
}

void type2_set_stuff (type2* t2, int stuff)
{
  t2->stuff = stuff;
}

void type2_run (type2* t2)
{
  type2_print(t2);
}

static void type2_print (const type2* t2)
{
  printf("%s %d\n", __func__, t2->stuff);
}

Same deal as type1.c.

This will print:

type1_print 1
type2_print 1

posted 9 months ago

CC BY-NC-SA 4.0

9mo ago

Lundin‭

9041 reputation 38 124 1039 167