The birth of my intelligent assistant: Khronos

I've done a lot of reworking to this main file recently, in regards to using a new speech recognition engine and integrating the last reviews suggestions. My question before is going to be phrased very similarly to how it was previously.

How Khronos Works


The process starts by recording a .wav file with LibSndFile and
PortAudio. PortAudio finds the default input device, and starts
recording audio. Once finished, PortAudio passes along the raw PCM
data to LibSndFile, which stores it in a .wav file.


This .wav file is passed to pocketsphinx for speech processing,
which then returns the spoken text. Khronos takes this text and
responds with its own speech synthesis
software.

Here's what I would like reviewed:

-
Readability: Is everything easily understood?

-
Bottlenecks: Are there places to improve speed?

-
Memory management: In general I avoid manually managing memory, since it's easy to screw up and better for the machine to handle it. Do I go too far with that concept here?

-
Portability: Am I constraining this in any way?

There are parts of code that I haven't included (method calls, structure definitions, etc.). Posting them here would make the question larger, and I would like the reviews to be more specific to the given code.

Code:

```
/**
* @file main.c
* @brief Khronos client
* @version 0.4
*/
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include

#include "audio.h"
#include "command.h"
#include "color.h"
#include "util.h"

cst_voice register_cmu_us_rms(const char str);

static void say(const char *str)
{
cst_voice* v = register_cmu_us_rms(NULL);
tritium_textToSpeech(str, v, "play");
}

static const char recognizeFromFile(ps_decoder_t ps, const char* fileName)
{
int16 adbuf[2048];
FILE *file = NULL;
const char *hyp = NULL;
size_t k = 0;

if ((file = fopen(fileName, "rb")) == NULL)
{
fprintf(stderr

Readability

Inconsistencies in coding style:

• Decide where You put an asterisk. Now You have things like this: ps_decoder_t ps, const char fileName.

• A few unreasonable var-names. What's size_t k? cst_voice v? const Command p?

• Mixing cases. Now You can't really recognize if it's a function or variable name, just by bluntly looking at the code. That's not entirely Your fault, but still it looks a bit messy. EDIT: to clarify myself, APIs You're using apparently has a different coding style. So, You have API calls like register_cmu_us_rms(), tritium_textToSpeech() but recognizeFromFile(). Sadly, don't have any good solutions for that :( Also, adbuf, but fileName, sampleComplete, etc. I would use different casing for function names and variables. But that's for You to decide or to pick some already existing coding style :)

Why would You put Your variable definitions all over place? It's messy. EDIT: I would debate that putting all the variable definitions on top of Your function body makes it waaaay more easier to read. Having in mind that functions must be short enough and with reasonable nesting. But yeah, that's debatable.

Multiple exit points. This is one of the readability issues I personally hate the most. 4 return statements in Your main()? 3 exit() in Your parseArgs? Really? Also, why wouldn't You reuse Your err variable in Your main() function, if You still insists having multiple exit points?

EDIT:
More on multiple exit points --- one of the ways would be something like this:

static const int recognizeFromFile(ps_decoder_t *ps,
    const char* fileName, char **outputBuffer)
{
    FILE *file = NULL;
    int retVal = 0;
    int tmpErr = 0;

    /* Sanitize inputs. */
    if (ps == NULL)
    {
        KHRONOS_LOGGER("Invalid (NULL) ps_decoder handle provided.");
        retVal = KHRONOS_ERR_INVALID;
        goto done;
    }

    if (fileName == NULL)
    {
        KHRONOS_LOGGER("Invalid (NULL) fileName provided.");
        retVal = KHRONOS_ERR_INVALID;
        goto done;
    }

    if (outputBuffer == NULL)
    {
        KHRONOS_LOGGER("Invalid (NULL) outputBuffer provided.");
        retVal = KHRONOS_ERR_INVALID;
        goto done;
    }

    /* Do the magic. */
    file = fopen(fileName, "rb");
    if (file == NULL)
    {
        KHRONOS_LOGGER("Failed to open file %s for reading.",
            fileName);
        retVal = KHRONOS_ERR_IO;
        goto done;
    }

    tmpErr = ps_start_utt(ps);
    if (tmpErr != 0)
    {
        KGRONOS_LOGGER("ps_start_utt failed with status: %d.",
            tmpErr);
        retVal = KHRONOS_ERR_API;
        goto done;
    }

    /* All the magic succeeded */
    /* So, fill the output buffer, or just set some return values */
    ...
    ...
done:
    /* Clean the stuff up. */
    if (file != NULL)
    {
        fclose(file)
    }

    return retVal;
}

Single exit point and single centralized space for all Your function's clean-up. Also added some more logging.
END-of-EDIT

Memory management

I can't see const char text and const char out = p->fn(); being freed anywhere. Not sure how those functions work, but it looks like a blunt malloc beneath. Isn't it?

Functionality

Despite failing to open the file (line #41) You still insist trying to read it.

Why would You check for same recognizeFromFile() twice (line #87, #92)? As far as I recall, puts() does not change string itself, does it?

This part:

while ((k = fread(adbuf, sizeof(int16), 2048, file)) > 0)
{
    ps_process_raw(ps, adbuf, k, false, false);
    bool inSpeech = ps_get_in_speech(ps);
    if (inSpeech && !uttStarted) uttStarted = true;
    if (!inSpeech && uttStarted)
    {
        ps_end_utt(ps);
        hyp = ps_get_hyp(ps, NULL);
        ps_start_utt(ps);
        uttStarted = false;
    }
}

Looks fishy. Why not just do something like this:

while ((k = fread(adbuf, sizeof(int16), 2048, file)) > 0)
{
    ps_process_raw(ps, adbuf, k, false, false);
    bool inSpeech = ps_get_in_speech(ps);

    uttStarted = inSpeech && !uttStarted;
    if (!inSpeech && uttStarted)
    {
        ps_end_utt(ps);
        hyp = ps_get_hyp(ps, NULL);
        ps_start_utt(ps);
        uttStarted = false;
    }
}

Also, I would really comment parts like this. Doesn't exactly trivial to get, what's actually happening beneath.

Generic

On line #109 You have sampleComplete = false; despite the fact You never use this value afterwards.

Why wouldn't You use proper argument parser (i.e. usual getopt) instead of writing Your own cumbersome parser?

You're not sanitizing any of Your inputs. I imagine this piece of code being used as a library. User of that library might pull his/her hairs off trying to debug his/her code and pin-point the location of some NULL pointer being passed on.

ctype.h include is unused in Your code, as far as I see.

Why would You cast pointer while freeing the memory?

StackExchange Code Review Q#128004, answer score: 16