2

I was writing a little source file function for my Pic32 and I got stucked on one thing. It's basically an utility that should store incomming char data into buffer and then, if '\r' is recieved, it compares the buffer against list of commands (in array names), and if match is found, the index of the item is returned.

This part is from header:

#define NAMECNT 6    
static const char names[NAMESCNT][10] = {   // 6commands, max 10 char each
        "korr",         // 1
        "adc",          // 2
        "fft",          // 3
        "data",         // 4 
        "pr",           // 5
        "prsc"};        // 6

/* functions */
extern int comm(char cdata);

At the main file, there is one big switch: 

switch( comm(recieved_ch) ){
case 1: foo1(); break; 
case 2: foo2(); break;
...
}

Now, for the better clarity, I wanted to use instead of 1, 2, ... the original names (like case KORR: case ADC:) so I wrote deffinitions for each one of them

#define KORR 1
#define ADC 2

But I don't like that solution, because I want to use this source file in more projects and there is gonna be different list of commands for each. Is there any way how to do this? Best thing would be to create the array names in preprocessor, but I doubt that's even possible. I was thinking about using enum type (which would have same items as list of commands names), but I am not sure how would that go.

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
L. Zeda
  • 123
  • 1
  • 1
  • 10
  • If you do make array/enum pairs, [this post](http://stackoverflow.com/q/10446827/694733) could be helpful. – user694733 Sep 05 '16 at 13:48
  • 1
    A `static const` array _defined_ in a header? Please move that to a source file and replace it with an `extern` _declaration_ before someone loses an eye. I doubt that all your clients need their own copies of constants. – sendaran Sep 05 '16 at 14:06
  • @sendaran It's for my own use(HW research only for R&D) and that array is in header on purpouse, since it acts as list of commands that can and should be changable... – L. Zeda Sep 05 '16 at 14:19

2 Answers2

2

The preprocessor could make things clearer here, I think, using the concatentation operator ##, but it'll not yield a performance advantage. A switch statement could be optimized by the compiler, but that's implementation-dependent.

Instead of the "one big switch," use an array of function pointers. Something like

func_ptrs[comm(received_ch) - 1]();

will call the corresponding function, where foo1 is at index 0, foo2 at 1, etc. To add a command, simply append a command name to the command list and a function pointer func_ptrs.

After all, you kill two birds with one stone: you create an easy way to add commands and improve performance.

Besides, a linear search through an array of strings is pretty inefficient. A hash table would yield a performance advantage.

cadaniluk
  • 15,027
  • 2
  • 39
  • 67
  • 1
    I agree with the function pointers. Not sure how you expected the `##` to help though. Also, I don't think there's much to gain for using hashtable, considering how short the list and the strings are. If linear search over list is too slow, simple `bsearch` could be good enough as long as list is sorted. – user694733 Sep 05 '16 at 13:44
  • That's an good idea, thanks. I was thinking about something like that, but I generaly don't like calling too many functions (something I got from old uCs with shallow stacks). And most of those commands are gonna be one liners (setting constants, inc/dec sfrs. swapping bits etc.) And to that point with hash table, there is gonna be max. 20 commands with max 10 char each, so 200 comparisons, so this is IMHO quick enough, it's an human to uC interface after all :D – L. Zeda Sep 05 '16 at 13:53
  • I think you mean the stringification operator (`#`), not the token-pasting operator (`##`). With regard to the performance of `switch` versus indexing into an array of function pointers, let me mention [this answer](http://programmers.stackexchange.com/a/305601) of mine from a while ago. – 5gon12eder Sep 05 '16 at 14:03
2

You can use X-macros to build an enum and fill the array, then you can use the enum values in the switch:

#define VARS \
    X(korr) \
    X(adc)  \
    X(fft)  \
    X(data) \
    X(pr)   \
    X(prsc)

static const char names[][10] = {   // 6commands, max 10 char each
#define X(name) #name,
    VARS
#undef X
};

enum evars {
#define X(name) name, 
    VARS
#undef X
};

extern int comm(char cdata);

int main(void)
{
    char x = 1;

    switch (comm(x)) {
        case korr:
            printf("korr");
            break;
        case adc:
            printf("adc");
            break;
        /* ... and so on */
    }
    return 0;
}

The expansion of X is:

static const char names[][10] = {

 "korr", "adc", "fft", "data", "pr", "prsc",

};

enum evars {

 korr, adc, fft, data, pr, prsc,

};

Edit: As pointed out by @5gon12eder, you don't need to hardcode 6 in the first dimension of the array (you can leave it unspecified).

David Ranieri
  • 39,972
  • 7
  • 52
  • 94
  • 1
    This is probably the best solution in this case. Only improvement I see is that `NAMESCNT` need not be hard-coded to 6 but can be determined from the array which could be declared with the first dimension unspecified. The X-macro could (if the functions are named systematicaly) also be used to generate an array of function pointers as suggested by the other answer. In this case, you'd actually need the token-pasting operator. – 5gon12eder Sep 05 '16 at 14:07
  • 1
    This is the way I am going to do it, thank you very much. `NAMESCNT` is hardcoded because it was used outside, in the main source file and originaly, I had that `names` list as static in .c file. – L. Zeda Sep 05 '16 at 18:26
  • You are welcome, notice that arrays are base 0 in C, `"korr"` is the element #0 in the array, be aware of that if you want: `#define KORR 1` – David Ranieri Sep 05 '16 at 18:29
  • 1
    I am aware of that and I am going to use that "dummy" first item you had there before edit; since I want all commands to have non 0 value, so I can use `if (comm(recieved_ch)){} else{}` in some other cases. I am in fact HW engineer and this is the easiest way how to control on board stuff trought serial port. I am using putty to send and recieve packages of data for debuging. – L. Zeda Sep 05 '16 at 18:38
  • 1
    You can use [X-macros](http://stackoverflow.com/documentation/c/628/x-macros) for the cases in a switch too, but to access the string ... just put NAMECNT at the end of the enum (not in the X-macro because that would cause problems), so then you can do printf("%s\n", ((unsigned)index < NAMECNT) ? names[index] : "invalid name" ); ... that also allows you to use -1 to indicate an invalid value instead of (or in addition to) 0 – technosaurus Sep 07 '16 at 21:01