5

I am developing firmware for an embedded application with memory constraints. I have a set of commands that need to processed as they are received. Each command falls under different 'buckets' and each 'bucket' gets a range of valid command numbers. I created two ENUMs as shown below to achieve this.

enum
{
  BUCKET_1 = 0x100, // Range of 0x100 to 0x1FF
  BUCKET_2 = 0x200, // Range of 0x200 to 0x2FF
  BUCKET_3 = 0x300, // Range of 0x300 to 0x3FF
  ...
  ...
  BUCKET_N = 0xN00 // Range of 0xN00 to 0xNFF
} cmd_buckets;

enum 
{
  //BUCKET_1 commands
  CMD_BUCKET_1_START = BUCKET_1,
  BUCKET_1_CMD_1,
  BUCKET_1_CMD_2,
  BUCKET_1_CMD_3,
  BUCKET_1_CMD_4,
  //Add new commands above this line
  BUCKET_1_CMD_MAX,

  //BUCKET_2 commands
  CMD_BUCKET_2_START = BUCKET_2,
  BUCKET_2_CMD_1,
  BUCKET_2_CMD_2,
  BUCKET_2_CMD_3,
  //Add new commands above this line
  BUCKET_2_CMD_MAX,

  //BUCKET_3 commands
  ...
  ...
  ...

  //BUCKET_N commands
  CMD_BUCKET_N_START = BUCKET_N
  BUCKET_N_CMD_1,
  BUCKET_N_CMD_2,
  BUCKET_N_CMD_3,
  BUCKET_N_CMD_4,
  //Add new commands above this line
  BUCKET_N_CMD_MAX,
}cmd_codes

When my command handler function receives a command code, it needs to check if the command is enabled before processing it. I plan to use a bitmap for this. Commands can be enabled or disabled from processing during run-time. I can use an int for each group (giving me 32 commands per group, I realize that 0xN00 to 0xN20 are valid command codes and that others codes in the range are wasted). Even though commands codes are wasted, the design choice has the benefit of easily telling the group of the command code when seeing raw data on a console.

Since many developers can add commands to the 'cmd_codes' enum (even new buckets may be added as needed to the 'cmd_buckets' enum), I want to make sure that the number of command codes in each bucket does not exceed 32 (bitmap is int). I want to catch this at compile time rather than run time. Other than checking each BUCKET_N_CMD_MAX value as below and throwing a compile time error, is there a better solution?

#if (BUCKET_1_CMD_MAX > 0x20)
#error ("Number of commands in BUCKET_1 exceeded 32")
#endif

#if (BUCKET_2_CMD_MAX > 0x20)
#error ("Number of commands in BUCKET_2 exceeded 32")
#endif

#if (BUCKET_3_CMD_MAX > 0x20)
#error ("Number of commands in BUCKET_3 exceeded 32")
#endif
...
...
...
#if (BUCKET_N_CMD_MAX > 0x20)
#error ("Number of commands in BUCKET_N exceeded 32")
#endif

Please also suggest if there is a more elegant way to design this.

Thanks, I appreciate your time and patience.

Lundin
  • 195,001
  • 40
  • 254
  • 396
CCoder
  • 63
  • 5
  • Comments can be made; however, I think you question is more apt for Stack Overflow's _Code Review_ forum. Can anyone move this to code Review? – Paul Ogilvie Jan 07 '16 at 08:42
  • 1
    @PaulOgilvie The question seems better suited for Stack Overflow. Code Review requires complete, working code examples. – Lundin Jan 07 '16 at 08:47
  • 1
    This example doesn't make any sense. You have a constant `BUCKET_1 = 0x100` which you then assign `CMD_BUCKET_1_START = BUCKET_1`. The trailing enums will therefore get values 0x101, 0x102, ... and `BUCKET_1_CMD_MAX` will be 0x106. Since 0x106 is always larger than 0x20, your static assert will always trigger. Please post a working example. – Lundin Jan 07 '16 at 08:55
  • You are correct Lundin, its a code bug I oversaw. But basically, I would mask the right bits to test the max count for each group. Your answer below seems to be doing just that. Thanks. – CCoder Jan 07 '16 at 20:52

2 Answers2

3

First fix the bug in the code. As mentioned in comments, you have a constant BUCKET_1 = 0x100 which you then assign CMD_BUCKET_1_START = BUCKET_1. The trailing enums will therefore get values 0x101, 0x102, ... and BUCKET_1_CMD_MAX will be 0x106. Since 0x106 is always larger than 0x20, your static assert will always trigger.

Fix that so that it actually checks the total number of items in the enum instead, like this:

#define BUCKET_1_CMD_N (BUCKET_1_CMD_MAX - CMD_BUCKET_1_START)
#define BUCKET_2_CMD_N (BUCKET_2_CMD_MAX - CMD_BUCKET_2_START)
...

Assuming the above is fixed, then you can replace the numerous checks with a single macro. Not a great improvement, but at least it reduces code repetition:

#define BUCKET_MAX 32 // use a defined constant instead of a magic number

// some helper macros:    
#define CHECK(n) BUCKET_ ## n ## _CMD_N
#define STRINGIFY(n) #n

// the actual macro:
#define BUCKET_CHECK(n) \
  _Static_assert(CHECK(n) <= BUCKET_MAX, \
                 "Number of commands in BUCKET_" STRINGIFY(n) "_CMD_N exceeds BUCKET_MAX.");


// usage:
int main (void)
{
  BUCKET_CHECK(1);
  BUCKET_CHECK(2);
}

Output from gcc in case one constant is too large:

error: static assertion failed: "Number of commands in BUCKET_1_CMD_N exceeds BUCKET_MAX."
note: in expansion of macro 'BUCKET_CHECK'

EDIT

If combining the bug fix with the check macro, you would get this:

#define BUCKET_MAX 32

#define CHECK(n) (BUCKET_##n##_CMD_MAX - CMD_BUCKET_##n##_START)
#define STRINGIFY(n) #n
#define BUCKET_CHECK(n) \
  _Static_assert(CHECK(n) <= BUCKET_MAX, \
                 "Number of commands in BUCKET " STRINGIFY(n) " exceeds BUCKET_MAX.");

int main (void)
{
  BUCKET_CHECK(1);
  BUCKET_CHECK(2);
}
Lundin
  • 195,001
  • 40
  • 254
  • 396
  • I suppose you could also merge the bug fix with the assertion macro, but then the macro gets messier. – Lundin Jan 07 '16 at 09:27
  • I don't see the reason of introducing the CHECK helper macro. It decreases readability by not adding anything. Also, I think you solution actually increases code repetition, because now one has to have both the additional list of `BUCKET_1_CMD_N` #defines AND the list of `BUCKET_CHECK` macro invocations. – kfx Jan 07 '16 at 09:54
  • @kfx There is no "additional list", it is necessary functionality to make the program work. But as I wrote in the comment above, it could be merged with the BUCKET_CHECK macro. As for readability: if you think `BUCKET_ ## n ## _CMD_N <= BUCKET_MAX` is more readable, then by all means use that form... Writing everything on a single line works too... – Lundin Jan 07 '16 at 11:01
  • Updated with a single macro which integrates the bug fix. – Lundin Jan 07 '16 at 11:07
  • I do think that writing the macro condition explicitly is more readable than writing just `BUCKET_CHECK(1)`. Being unfamiliar with that code I would have no idea what is checked. – kfx Jan 07 '16 at 11:26
  • I think this is way to go, but I also think that macro would be more descriptive in the form of `BUCKET_CHECK (CMD_BUCKET_1_START, BUCKET_1_CMD_MAX)`. It's a bit more typing, but more easy on the eyes. – user694733 Jan 07 '16 at 11:38
  • `_Static_assert` macro has been added in C11, if you use previous version, you can try this to replace `_Static_assert` accordingly to this post: http://stackoverflow.com/questions/3385515/static-assert-in-c – Felipe Lavratti Jan 09 '16 at 14:19
  • @fanl It's a keyword not a macro (unlike `static_assert` in assert.h, which is a macro that expands to `_Static_assert`, for C++ compatibility). Most common way to do this in old C would be something like `#define CT_ASSERT(cond) typedef int ct_assert [cond?1:0];`. However, I would recommend to upgrade to a modern compiler instead, if possible. – Lundin Jan 11 '16 at 07:42
  • @Lundin, true, my fault. – Felipe Lavratti Jan 11 '16 at 19:16
1

First of all, preprocessor commands do not work that way. The C preprocessor is able to "see" only names instruced by the #define statement or passes as compiler flags. It is not able to see constants defined as part of an enum or with the const keyword. You should use _Static_assert to validate the commands instead of the preprocessor.

As for the commands, I would suggest to have all the commands numbered in the range 0..0x20:

enum {
  BUCKET_1_CMD_1,
  BUCKET_1_CMD_2,
  ...
  BUCKET_1_CMD_MAX,
};
enum {
  BUCKET_2_CMD_1,
  BUCKET_2_CMD_2,
  ...
  BUCKET_2_CMD_MAX,
};

Then you need only a single guard value to check if all the commands are in valid range:

#define MAX_COMMAND 0x20
_Static_assert(BUCKET_1_CMD_MAX <= MAX_COMMAND, "too many bucket 1 commands");
_Static_assert(BUCKET_2_CMD_MAX <= MAX_COMMAND, "too many bucket 2 commands");

To use the commands, bitwise-or them together with the bucket "offset":

enum {
BUCKET_1 = 0x100,
BUCKET_2 = 0x200,
};
...
int cmd = BUCKET_2 | BUCKET_2_CMD_1;
kfx
  • 8,136
  • 3
  • 28
  • 52
  • This works, but has a side-effect that you lose `cmd_codes` type, and any compiler type checking with it (which may or may not be an issue). – user694733 Jan 07 '16 at 11:36
  • Thanks for your response, I am using `_Static_assert()` as suggested and using @Lundin's approach – CCoder Jan 07 '16 at 22:00