2

The title may not be clear so I'll give an example.

I am trying to make a system of "data streams" in C.

Type STREAM:

typedef struct {
    void (*tx) (uint8_t b);
    uint8_t (*rx) (void);
} STREAM;

I have a file uart.h with uart.c which should provide a STREAM for UART.

I decided it'll be best to expose it as a pointer, so it can be passed to functions without using ampersand.

This is the kind of functions I want to use it with (example):

/** Send signed int */
void put_i16(const STREAM *p, const int16_t num);

Here's my UART files:

uart.h

extern STREAM* uart;

uart.c

// Shared stream instance
static STREAM _uart_singleton;
STREAM* uart;

void uart_init(uint16_t ubrr) {
    // uart init code here

    // Create the stream
    _uart_singleton.tx = &uart_tx; // function pointers
    _uart_singleton.rx = &uart_rx;

    uart = &_uart_singleton; // expose a pointer to it
}

I'm not sure about this. It works, but is it the right way to do it? Should I just use Malloc instead?

Why I ask this, it's a library code and I want it to be as clean and "correct" as possible

MightyPork
  • 18,270
  • 10
  • 79
  • 133
  • 1
    No need to use `malloc`. You code looks fine to me. – Jabberwocky Apr 30 '15 at 12:06
  • so using heap variable this way is OK practice? Just making sure, I'm not very experienced with this more advanced C – MightyPork Apr 30 '15 at 12:07
  • 1
    There is no heap variable in your code, only global variables. – Jabberwocky Apr 30 '15 at 12:08
  • Oh well I thought `_uart_singleton` lives on heap. Anyway that's the one I meant – MightyPork Apr 30 '15 at 12:09
  • Only variables allocated with the `malloc` family of functions are living on the heap. And your code is fine. Juste like any global variable`_uart_singleton` will live for ever throughout the execution of the program. – Jabberwocky Apr 30 '15 at 12:09
  • Right then I misunderstood how the memory works, so there's separate part of the memory for those.. "data memory"? – MightyPork Apr 30 '15 at 12:11
  • 1
    Google `data vs bss` for more information. – Jabberwocky Apr 30 '15 at 12:13
  • If there can be only one instance of the struct (singleton), why expose it at all? It makes sense for functions to have a struct pointer as first parameter if there can be several different streams, but if there can be only one... then you can keep the singleton `static` and just use it in `put_i16`. Did I misunderstand? – Gauthier May 05 '15 at 08:42
  • The point of streams here is that I have different stream implementations and I want to use one set of functions to work with them. There's eg. UART stream, LCD stream, keyboard stream (obviously read-only) etc that all can be accessed the same way. Here the AVR has only one UART, so it doesn't make sense to pass it around, that's why I decided to make it a global variable – MightyPork May 05 '15 at 08:50

1 Answers1

3

The global pointer is unnecessary (as are all globals), and unsafe - it is non-const; any code with access to the pointer could modify _uart_singleton.

uart.h

const STREAM* getUart() ;
...

uart.c

// Shared stream instance
static STREAM _uart_singleton = {0} ;

const STREAM* getUart()
{
    // Return singleton if initialised, 
    // otherwise NULL
    return _uart_singleton.rx != 0 && 
           _uart_singleton.tx != 0 ? _uart_singleton :
                                     NULL ;
}

void uart_init(uint16_t ubrr) 
{
    // uart init code here

    // Create the stream
    _uart_singleton.tx = &uart_tx; // function pointers
    _uart_singleton.rx = &uart_rx;
}

So long as all the functions that access STREAM members are defined withing uart.c, then you can also benefit from making STREAM an opaque type (Lundin's suggestion in comment) by using an incomplete struct declaration in the header thus:

uart.h

struct sStream ;
typedef struct sStream STREAM ;

const STREAM* getUart() ;
...

uart.c

// Shared stream instance
struct sStream 
{
    void (*tx) (uint8_t b);
    uint8_t (*rx) (void);

} _uart_singleton = {0} ;

const STREAM* getUart()
{
    // Return singleton if initialised, 
    // otherwise NULL
    return _uart_singleton.rx != 0 && 
           _uart_singleton.tx != 0 ? _uart_singleton :
                                     NULL ;
}

...

This prevents any code outside of uart.c from calling the rx and tx functions directly or accessing any other members.

Clifford
  • 88,407
  • 13
  • 85
  • 165
  • `getUart()` could hand it back as a reference, rather than a pointer. (A const reference if that suits your needs). – GrahamS Apr 30 '15 at 12:47
  • @GrahamS what do you mean? Isn't pointer the same as a reference? – MightyPork Apr 30 '15 at 13:08
  • Not quite @MightyPork, a reference can never be null and cannot be changed to refer to something else after it has been created. Generally speaking these restrictions mean references are a little "safer" to use than pointers. But they are a C++ type and I've just noticed this question only relates to C, so pointers it is :) – GrahamS Apr 30 '15 at 13:16
  • See http://stackoverflow.com/questions/57483/what-are-the-differences-between-a-pointer-variable-and-a-reference-variable-in for more about references vs pointer if you are interested. – GrahamS Apr 30 '15 at 13:16
  • 1
    @GrahamS In practice, a reference is just a constant pointer equal to `type* const`. It just comes with a bit easier syntax so that C++ programmers need not worry their pretty heads about operators like `*` and `->` :) – Lundin Apr 30 '15 at 13:26
  • In this case, I believe the STREAM type can be declared as incomplete type, so that you'll hide away its internal implementation entirely. – Lundin Apr 30 '15 at 13:29
  • Yeah that's my understanding too @Lundin though a reference can't (legally) be null whereas a constant pointer could be. – GrahamS Apr 30 '15 at 13:29
  • @GrahamS Your compiler should give you a warning if you don't initialize a `type* const` to point at something. – Lundin Apr 30 '15 at 13:30
  • @Lundin yup but you _can_ initialise it to point to NULL or any old address you fancy, i.e. `const char* const myptr = NULL;` is legal. – GrahamS Apr 30 '15 at 13:38
  • Actually uninitialised is fine by GCC 4.9.2 as well. See https://ideone.com/j2LeFY – GrahamS Apr 30 '15 at 13:40
  • 1
    @Lundin : Elaborated answer with your incomplete type suggestion. – Clifford May 01 '15 at 01:15
  • @Clifford Take spaghetti coding with globals and turn it into an object-oriented program design, good stuff :) (As a parenthesis, the h file declaration could be written as just `typedef struct STREAM STREAM;` and then you can still name the struct STREAM internally in the C file.) – Lundin May 04 '15 at 06:21
  • I wonder about the benefit of OO design in this specific case, since the struct instance is a singleton. What's the point of exposing it at all, if all module functions are intended to use the same unique instance of `struct STREAM`? – Gauthier May 05 '15 at 08:45
  • @Gauthier : Perhaps no great advantage but I would question the use of the singleton in any case, except that is probably beyond the scope of the original question. It is common to have multiple UARTs, and may be better to support multiple devices and pass the device as well as the baud-rate to the init function – Clifford May 05 '15 at 08:54