Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 have to have a large memory (ROM and RAM) footprint. The camera's 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?

share|improve this question
    
There was a bug in this code, by the way. I'm accidentally writing to a readBuffer... whoops. –  Pimgd Jul 24 at 9:53

2 Answers 2

up vote 6 down vote accepted

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!).

share|improve this answer
    
True, I forgot about the comments. Regarding the switch though, won't that cause an evaluation of each case (just like a chain of else-if)? –  Pimgd Jul 23 at 11:27
1  
No, the switch will evaluate the variable and jump only to the case that has the matching value. switch allows for Duff's device techniques but the returns avoid it in this case (hence no breaks). –  Nobody Jul 23 at 11:29
    
So using a switch in C is O(1), whereas a if-elseif chain is O(n). Interesting... –  Pimgd Jul 23 at 11:32
    
@Pimgd: I am not entirely sure if the switch is always \$O(1)\$ (they seldom have enough cases to matter for that fact) if the mapping is not dense. –  Nobody Jul 23 at 11:38
1  
@Pimgd: It is forbidden to conditionally declare a variable in a case as others might access it as well altough it is logically not declared there. By wrapping it into {} it has its own scope and can't be accessed by the other cases. –  Nobody Jul 23 at 15:05

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;
}
}
share|improve this answer
    
The problem is that when coding the other side of the connection, I have to put the same enum values in XML. I also don't have much bandwidth to play with - I can't send more than 1310 bytes back per request. Also, it's likely that I won't be the one to put those enum values in the mobile apps, so documentation will be required anyway. Writing to the write and read buffers can only be done with signed/unsigned 1,2 and 4 byte values or strings. I admire the ideas, they're quite good, but the scale and limitations of my project as they are now doesn't make this a feasible option. –  Pimgd Jul 29 at 11:38
    
Additionally, there won't be "multiple devices"; Each camera is a physical instance upon which the program runs. There's an actual camera involved. The only one who would have this problem of multiple instances would be the client, but the client is on a phone or a webbrowser and there's plenty of resources there so getting that to work nicely is no problem at all. I appreciate the effort, though - will be looking into using the & operator to combine wifi-encryption compatibility (like WEP/WPA/WPA2) into a single uint8_t, should save me some space. –  Pimgd Jul 29 at 11:43
    
@Pimgd combining the boolean types into a single char/short/int will help with the bandwidth as would using rest type requests vs xml ... ?C=<clientid>&P=<property_mask_in_hex>&A=<action_id>&... then you can let standard <form>s do most of the work –  technosaurus Jul 29 at 11:57
    
On the client's side, a javascript library sends and retrieves raw bytes - it uses the xml as a descriptor for what the raw bytes mean. –  Pimgd Jul 29 at 12:00
    
<noun>xml</noun>&nbsp;<verb>is</verb>&nbsp;<article>a</article>&nbsp<adjective>b‌​ad</adjective>&nbsp<noun>choice</noun><punctuation>.</punctuation> ?javascript+can+build+a+proper+query+string+with&key=value in the http header to control actions and the raw stream bytes can be the entire body {"msg":"even json would be better"} –  technosaurus Jul 29 at 17:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.