C; ring buffer; is this "doable" approach?

[mtbro]

I needed to create ring buffer for my keyboard driver. While I need only to buffer bytes(chars) I was wondering if I can implement more versatile approach if I need it for something else. I did create this but I'm not sure if I'm not pushing it too much.
Here's the implementation:

Code: Select all

#ifndef HAVE_TYPES_H
#define HAVE_TYPES_H

#include <stdint.h>

#define	RINGBUF32_SIZE		32

#define	DEFINE_RING32(name, type) \
	struct rbuf32 {                   \
		uint8_t ri:5;                 \
		uint8_t ci:5;		        \
		type buf[RINGBUF32_SIZE];\
	} rbuf32_t;				 \
	typedef struct rbuf32 name;

#define STORE_RING32(rb, item) do {			\
		(rb)->buf[(rb)->ci++] = (item);		\
	} while(0)

#define	FETCH_RING32(rb)	 (rb)->buf[(rb)->ri++]

#endif /* ifndef HAVE_TYPES_H */

With test program:

Code: Select all

#include <stdio.h>
#include <string.h>
#include "types.h"

#define	SANITY_CHECK	2048

DEFINE_RING32(dbuf32, int)

void dumpbuf(void* b);

int main() {
	dbuf32 buf;
	dbuf32* pbuf = &buf;	// XXX: due to way macros are defined
	int d,i;

	memset(&buf, 0, sizeof(buf));
	i =0;
	while(1) {
		scanf("%d", &d);
		printf("storing: %d\n", d);

		STORE_RING32(pbuf, d);
		if (i++ > SANITY_CHECK || d == 42) {
			break;
		}
	}
	dumpbuf(pbuf);
	for (i=0; i < 6; i++) {
		printf("fetch: %d\n", FETCH_RING32(pbuf));
	}
	dumpbuf(pbuf);
	return 0;
}

void dumpbuf(void* b) {
	dbuf32* buf = (dbuf32*)b;
	printf("ri: %d, ci: %d\n", buf->ri, buf->ci);
	for(int i =0; i < 32; i++) {
		printf("%d ", buf->buf[i]);
	}
	printf("\n");
}

Is this acceptable form of such definition ? I never used macros like these. I mean, it does compile, it doesn't complain. But I'm just not sure if that's a good idea.

[Demindiro]

My preference is to have one macro that generates the struct type as well as corresponding functions, e.g.

Code: Select all

#ifndef HAVE_TYPES_H
#define HAVE_TYPES_H

#include <stdint.h>

#define   RINGBUF32_SIZE      32

#define   DEFINE_RING32(name, type) \
   typedef struct { \
      uint8_t ri:5; \
      uint8_t ci:5; \
      type buf[RINGBUF32_SIZE]; \
   } name; \
   \
   void name##_init(name *rb) { \
	   memset(rb, 0, sizeof(*rb)); \
   } \
   \
   void name##_store(name *rb, type item) { \
      (rb)->buf[(rb)->ci++] = item; \
   } \
   \
   type name##_fetch(name *rb) { \
	   return (rb)->buf[(rb)->ri++]; \
   }

#endif /* ifndef HAVE_TYPES_H */

Code: Select all

#include <stdio.h>
#include <string.h>
#include "types.h"

#define   SANITY_CHECK   2048

DEFINE_RING32(dbuf32, int)

void dumpbuf(dbuf32 *b);

int main() {
   dbuf32 buf;
   int d, i;

   dbuf32_init(&buf);

   i = 0;
   while(1) {
      scanf("%d", &d);
      printf("storing: %d\n", d);

      dbuf32_store(&buf, d);
      if (i++ > SANITY_CHECK || d == 42) {
         break;
      }
   }
   dumpbuf(&buf);
   for (i = 0; i < 6; i++) {
      printf("fetch: %d\n", dbuf32_fetch(&buf));
   }
   dumpbuf(&buf);
   return 0;
}

void dumpbuf(dbuf32 *buf) {
   printf("ri: %d, ci: %d\n", buf->ri, buf->ci);
   for(int i =0; i < 32; i++) {
      printf("%d ", buf->buf[i]);
   }
   printf("\n");
}

You may want to make the generated functions static or weakly linked, depending on how exactly you will use the macro.

[nullplan]

Well, in principle I always have a bit of a problem with fixed sizes with no obvious justification. The buffer is 32 elements. Depending on the use case, this may be too much or too little, and does not appear to be tunable. But for the most part, this will work. However, some details:

Code: Select all

#define   DEFINE_RING32(name, type) \
   struct rbuf32 {                   \
      uint8_t ri:5;                 \
      uint8_t ci:5;              \
      type buf[RINGBUF32_SIZE];\
   } rbuf32_t;             \
   typedef struct rbuf32 name;

This creates an instance of "struct rbuf32" named "rbuf32_t". I believe you wanted to make a new type here, although I also fail to see what use those names are given that you do everything else in macros, and those names prevent reuse (if you define two different ring buffers, they will get the same name, and the compiler will complain about that). Also, the base type of a bitfield can only be signed int or unsigned int or int (yielding a signed, unsigned, or implementation-defined bitfield, respectively), everything else is a GCC extension. So putting it all together, I'd go with

Code: Select all

#define DEFINE_RING32(name, type) \
  typedef struct { \
    unsigned ri:5; \
    unsigned ci:5; \
    type buf[32]; \
  } name

That also makes the macro not end in a semicolon, so you can add those to the macro calls again, which will likely be less confusing for readers.

[mtbro]

Thanks both for the input and corrections.

This creates an instance of "struct rbuf32" named "rbuf32_t".

That's incorrect design choice on my side. I like the way you and Demindiro set it.

When it comes to static buffer size I agree, it's not ideal. I don't have memory management set yet. I know I should probably focus on more essential parts of kernel first. I liked the idea of creating custom gdb stub first, even at the very beginning of my mios project. That's why I have to have static buffer size.