Mapping enum to function - request / response library encapsulation

Question

I'm writing code for an embedded device - specifically, a camera. The idea is different camera manufacturers can implement the code so it properly works with their camera.

Here, I'm encapsulating the request/response library we're using, because it's complicated to deal with, and we want to use ease of implementation as a selling point. Some of the requests require a response or parameters. Others, however, act like your TV would: you press a button, that sends a command code, and that triggers an action. No return code needed, no parameters. I'm gonna have a lot of functions like that.

Relevant types provided by the library we're using:

typedef unsigned char bool;
typedef unsigned char uint8_t;

My enum definition...

typedef enum {
    //auth - 1xx
    //move - 2xx
    //snapshot - 3xx
    //stream - 4xx
    //zoom - 5xx
    //infrared - 6xx
    ZOOM_CAPABLE = 500,
    ZOOM_IN = 501,
    ZOOM_OUT = 502,
    ZOOM_STOP = 503,
    INFRARED_CAPABLE = 600,
    INFRARED_ON = 601,
    INFRARED_OFF = 602,
    WRITE_DEMO = 1000,
    READ_DEMO = 1001
} application_requestID;

These values are shared by some javascript client that communicates with my embedded device. I have the authority to change the values and the javascript client. I'm developing a new system, so I don't need to adhere to any previously defined values.

Currently it's ZOOM_IN, ZOOM_OUT, ZOOM_STOP, INFRARED_ON, INFRARED_OFF that are functions that require no input arguments and no output arguments. As you can see in the comments, I got more stuff planned.

The functions look like this:

bool canZoom();//ZOOM_CAPABLE
void startZoomIn();//ZOOM_IN
void startZoomOut();//ZOOM_OUT
void stopZoom();//ZOOM_STOP

And (as a demo), I have implemented them like this:

static bool zoomingIn = false;
static bool zoomingOut = false;
static const uint8_t ZOOM_CAP = 200;
static uint8_t currentZoom = 100;

bool canZoom(){
    return true;
}
void startZoomIn(){
    zoomingIn = true;
    zoomingOut = false;
}
void startZoomOut(){
    zoomingOut = true;
    zoomingIn = false;
}
void stopZoom(){
    zoomingIn = false;
    zoomingOut = false;
}

void tick_cameraInterface(){
    //do stuff here, like moving a camera?
    //this simulates the idea that between requests, a camera is constantly moving (until it is asked to stop)
    if (zoomingIn && currentZoom < ZOOM_CAP){
        currentZoom++;
    }
    if (zoomingOut && currentZoom > 0){
        currentZoom--;
    }
}

Now, the last section of code, the encapsulation of the responses and requests:

application_event_result application_event(application_request* applicationRequest, buffer_read_t* readBuffer, buffer_write_t* writeBuffer){
    if (applicationRequest->queryId == WRITE_DEMO){
        if (!unabto_query_write_uint8(writeBuffer, getDemoValue())){
            return AER_REQ_RSP_TOO_LARGE;
        }
        return AER_REQ_RESPONSE_READY;
    } else if (applicationRequest->queryId == READ_DEMO){
        uint8_t x;
        if (!unabto_query_read_uint8(readBuffer, &x)){
            return AER_REQ_TOO_SMALL;
        }
        setDemoValue(x);
        return AER_REQ_RESPONSE_READY;
    } else if (applicationRequest->queryId == ZOOM_CAPABLE){
        if (!unabto_query_write_uint8(readBuffer, canZoom())){
            return AER_REQ_RSP_TOO_LARGE;
        }
        return AER_REQ_RESPONSE_READY;
    } else if (applicationRequest->queryId == ZOOM_IN){
        startZoomIn();
        return AER_REQ_RESPONSE_READY;
    } else if (applicationRequest->queryId == ZOOM_OUT){
        startZoomOut();
        return AER_REQ_RESPONSE_READY;
    } else if (applicationRequest->queryId == ZOOM_STOP){
        stopZoom();
        return AER_REQ_RESPONSE_READY;
    }

    return AER_REQ_NO_QUERY_ID;
}

I'm seeing a massive instance of repetition here. ZOOM_IN? Call startZoomIn() function. ZOOM_OUT? Call startZoomOut() function. These things sound like they could be mapped to one another. But here's the catch...

• I'd like to keep my enum values. Altering them to be sequential makes it trivial to create an array of function pointers, but I lose out on maintainability; if I have to add ZOOM_RESET later, it's gonna have a weird number.
• I don't want to have a large memory (ROM and RAM) footprint. The cameras already use a certain amount of memory. The more memory I use, the higher the chance I force the camera manufacturer to use a larger ROM or RAM chip. If integration of our interface were to force the manufacturer to upgrade, I'm sure we'll get those costs billed somehow (that, or camera manufacturers will say "hmm... nope, that won't fit").

Ideally I'd like to be ANSI C compliant, as I will be altering my code to be ANSI C compliant later (I have to test on Win32 for now, so I can't compile as ANSI C since Windows.h is filled with C++ code). Being ANSI C compliant would be another selling point, since the code would compile on a large amount of compilers (and thus easily be adaptable to a broad range of embedded devices).

How would I refactor this to use a enum to function mapping? Is it possible to do so without having to traverse the array?

Answer 1

ANSI C

Don't use // comments but /**/.

Design away invalid states

It probably will never happen and is only in your mockup code but your design allows for the state

zoomingIn = true;
zoomingOut = true;

These two are mutually exclusive so you might consider to use a tristate (enum) like:

typedef enum {
    ZOOMING_IN,
    ZOOMING_STOPPED,
    ZOOMING_OUT,
} zooming_state;

And having the state managed in one variable zooming (or something with a better name then).

Switch to the rescue

Why don't you use a switch for the commandmapping?

switch(applicationRequest->queryId) {
case WRITE_DEMO:
    if (!unabto_query_write_uint8(writeBuffer, getDemoValue())){
        return AER_REQ_RSP_TOO_LARGE;
    }
    return AER_REQ_RESPONSE_READY;
case READ_DEMO: {
    uint8_t x;
    if (!unabto_query_read_uint8(readBuffer, &x)){
        return AER_REQ_TOO_SMALL;
    }
    setDemoValue(x);
    return AER_REQ_RESPONSE_READY;
}
case ZOOM_CAPABLE:
    if (!unabto_query_write_uint8(readBuffer, canZoom())){
        return AER_REQ_RSP_TOO_LARGE;
    }
    return AER_REQ_RESPONSE_READY;
case ZOOM_IN:
    startZoomIn();
    return AER_REQ_RESPONSE_READY;
case ZOOM_OUT:
    startZoomOut();
    return AER_REQ_RESPONSE_READY;
case ZOOM_STOP:
    stopZoom();
    return AER_REQ_RESPONSE_READY;
default:
    return AER_REQ_NO_QUERY_ID;
}

That looks much cleaner to me and it can handle the sparse indices as well (and with automatic compileroptimization!).

Answer 2

if I have to add ZOOM_RESET later, it's gonna have a weird number.

Enums are an excellent way to map function pointers and makes maintenance easier not harder, all you have to do is insert the new function(s) at the end; whereas using some randomly defined numbering scheme requires tracking and documentation - I would reconsider

Another recommendation would be to use a single struct pointer for the data (properties, buffers and actions) to simplify functions and allow for multiple devices without polluting the global namespace with static variables for each property

The combination of those 2 things will allow you to implement multi-camera functionality simply by adding a dimension to the function pointer array and creating another instance of camera_t.

#include <stdio.h>
typedef struct{
    unsigned properties; /* mask of all tracked boolean properties */
    unsigned action;
    unsigned maxzoom;
    unsigned minzoom;
    unsigned zoom;
    FILE *readbuf;
    FILE *writebuf;
    /* you could put the array of function pointers here too */
}camera_t;
typedef int (*Fi)(camera_t *);

enum{ /* flags to be set in camera properties mask */
  zoomCapable = 1,
  zoomingIn = 2,
  zoomingOut = 4,
  infraredCapable = 8,
  infraredOn = 16, /* off == not on, so omitting */
  /* more camera properties here (powers of 2) */
};

/* sets properties and buffer pointers */
int cameraInit(camera_t *cam);//CAM_INIT 
int startZoomIn(camera_t *cam);//ZOOM_IN
int startZoomOut(camera_t *cam);//ZOOM_OUT
int stopZoom(camera_t *cam);//ZOOM_STOP
int infraredOn(camera_t *cam);//INFRA_ON
int infraredOff(camera_t *cam);//INFRA_OFF

/* use these to access the function in array,
 * insert new ones before NUM_FUNCS */
enum{ CAM_INIT,ZOOM_IN,ZOOM_OUT,ZOOM_STOP,INFRA_ON,INFRA_OFF, NUM_FUNCS};

//assign the corresponding functions in same order as enum above 
Fi run[] = {cameraInit,startZoomIn,startZoomOut,stopZoom,infraredOn,InfraredOff};

int application_event(camera_t *cam){
  if (cam->action >= NUM_FUNCS)
    return -1;
  else return run[cam->action](cam);
}

if (0) /* your example templates here */
int cameraInit(camera_t cam){
  if (cam->readbuf = fopen("/dev/superawesomecam/video0", "r") == NULL)
    return -1; /* or return appropriate error enum based on errno */
  if (cam->writebuf = fopen("/dev/superawesomecam/control", "w") == NULL)
    return -1; /* or return appropriate error enum based on errno */
  cam->properties = zoomCapable & infraredCapable; /* == 9 */
  cam->maxzoom = 1000;
  cam->minzoom = 10;
  cam->zoom = 100; /* may be a vendor specific function */
  return 0;
}
}