This library code seems better than the worst I have seen at first glance, but it still have some issues. The most severe is its Final result code handling.
The sim900_check_with_cmd function is conceptually almost there, however only checking for OK is in no way acceptable. It should check for every single possible final result code the modem might send.
From your output examples you have the following final result codes
- OK
- ERROR
- NO CARRIER
- NO ANSWER
but there exists a few more as well. You can look at the code for atinout for an example of a is_final_result_code function (you can also compare to isFinalResponseError and isFinalResponseSuccess1 in ST-Ericsson's U300 RIL).
The unconditional return true; at the end of GPRS::callUp is an error, but it might be deliberate due to lack of ideas for implementing a better API so that the calling client could check the intermediate result codes. But that is such a wrong way to do it.
The library really should do all the stateful command line invocation and final result code parsing with no exceptions. Just doing parts of that in the library and leaving some of it up to the client is just bad design.
When clients want to inspect or act on intermediate result codes or information text that comes between the command line and the final result code, the correct way to do it is to let the library "deframe" everything it receives from the modem into individual complete lines, and for everything that is not a final result code provide this to the client through a callback function.
The following is from an unfinished update to my atinout program:
bool send_commandline(
const char *cmdline,
const char *prefix,
void (*handler)(const char *response_line, void *ptr),
void *ptr,
FILE *modem)
{
int res;
char response_line[1024];
DEBUG(DEBUG_MODEM_WRITE, ">%s\n", cmdline);
res = fputs(cmdline, modem);
if (res < 0) {
error(ERR "failed to send '%s' to modem (res = %d)", cmdline, res);
return false;
}
/*
* Adding a tiny delay here to avoid losing input data which
* sometimes happens when immediately jumping into reading
* responses from the modem.
*/
sleep_milliseconds(200);
do {
const char *line;
line = fgets(response_line, (int)sizeof(response_line), modem);
if (line == NULL) {
error(ERR "EOF from modem");
return false;
}
DEBUG(DEBUG_MODEM_READ, "<%s\n", line);
if (prefix[0] == '\0') {
handler(response_line, ptr);
} else if (STARTS_WITH(response_line, prefix)) {
handler(response_line + strlen(prefix) + strlen(" "), ptr);
}
} while (! is_final_result(response_line));
return strcmp(response_line, "OK\r\n") == 0;
}
You can use that as a basis for implementing proper handling. If you want to
get error responses out of the function, add an additional callback argument and change to
success = strcmp(response_line, "OK\r\n") == 0;
if (!success) {
error_handler(response_line, ptr);
}
return success;
Tip: Read all of chapter 5 in the V.250 specification, it will teach you almost everything you need to know about command lines, result codes and response handling. Like for instance that a command line should also be terminated with \r only, not \r\n-
1 Note that CONNECT is not a final result code, it is an intermediate result code, so the name isFinalResponseSuccess is strictly speaking not 100% correct.
