patterncMinor
Mapping enum to function - request / response library encapsulation
Viewed 0 times
enumrequestresponsefunctionencapsulationlibrarymapping
Problem
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:
My enum definition...
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
The functions look like this:
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;
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_STOPAnd (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;
Solution
ANSI C
Don't use
Design away invalid states
It probably will never happen and is only in your mockup code but your design allows for the state
These two are mutually exclusive so you might consider to use a tristate (enum) like:
And having the state managed in one variable
Switch to the rescue
Why don't you use a
That looks much cleaner to me and it can handle the sparse indices as well (and with automatic compileroptimization!).
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!).
Code Snippets
zoomingIn = true;
zoomingOut = true;typedef enum {
ZOOMING_IN,
ZOOMING_STOPPED,
ZOOMING_OUT,
} zooming_state;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;
}Context
StackExchange Code Review Q#57765, answer score: 7
Revisions (0)
No revisions yet.