OSDev.org
https://forum.osdev.org/

C; ring buffer; is this "doable" approach?
https://forum.osdev.org/viewtopic.php?f=13&t=56700
Page 1 of 1

Author:  mtbro [ Mon Jan 23, 2023 10:07 am ]
Post subject:  C; ring buffer; is this "doable" approach?

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:
#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:
#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.

Author:  Demindiro [ Mon Jan 23, 2023 12:30 pm ]
Post subject:  Re: C; ring buffer; is this "doable" approach?

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

Code:
#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:
#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.

Author:  nullplan [ Mon Jan 23, 2023 12:36 pm ]
Post subject:  Re: C; ring buffer; is this "doable" approach?

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:
#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:
#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.

Author:  mtbro [ Mon Jan 23, 2023 1:22 pm ]
Post subject:  Re: C; ring buffer; is this "doable" approach?

Thanks both for the input and corrections.
nullplan wrote:
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.

Page 1 of 1 All times are UTC - 6 hours
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group
http://www.phpbb.com/