The birth of my intelligent assistant: Khronos

Question

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 <ctype.h>
#include <errno.h>
#include <fcntl.h>
#include <limits.h>
#include <pocketsphinx.h>
#include <portaudio.h>
#include <sphinxbase/err.h>
#include <stdlib.h>
#include <stdbool.h>
#include <time.h>
#include <tritium/tritium.h>
#include <unistd.h>

#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, "Failed to open file '%s' for reading", fileName);
    }

    // verify .wav file?  I trust libsndfile to make a valid one
    ps_start_utt(ps);
    bool uttStarted = false;

    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;
        }
    }
    ps_end_utt(ps);

    if (uttStarted)
    {
        hyp = ps_get_hyp(ps, NULL);
    }

    fclose(file);
    return hyp;
}


int runKhronos(PaStream *stream, AudioData *data, AudioSnippet *sampleBlock, ps_decoder_t *ps)
{
    int err = 0;
    bool sampleComplete = false;
    int fd = createSafeFileDescriptor(getTmpDir());
    const char *fileName = getPathFromDescriptor(fd);
    if ((err = processStream(stream, data, sampleBlock, fd, &sampleComplete)))
    {
        fprintf(stderr, "Error recording FLAC file: %d\n", err);
        return err;
    }
    else if (sampleComplete)
    {
        const char *text = recognizeFromFile(ps, fileName);
        if (text)
        {
            fprintf(stdout, "Recognized text: %s\n", text);
        }
        else puts(RED_TEXT("No text recognized."));
        if (text)
        {
            bool said = false;
            for (size_t i = 0; i < ARRAY_SIZE(commands); ++i)
            {
                const Command *p = commands + i;
                if (strstr(text, p->cmd))
                {
                    const char* out = p->fn();
                    fprintf(stdout, "%s\n", out);
                    say(out);
                    said = true;
                }
            }
            // there was some text, but a response was unknown
            if (!said) say("I could not understand what you said.");
        }
        sampleComplete = false;
    }
    close(fd);
    unlink(fileName);
    free((void*) fileName);
    return err;
}

static int parseArgs(int argc, char **argv)
{
    while (argc--)
    {
        if (streq("--help", argv[argc]) || streq("-h", argv[argc]) || streq("help", argv[argc]))
        {
            puts(printHelp());
            exit(EXIT_SUCCESS);
        }
        if (streq("--version", argv[argc]) || streq("-v", argv[argc]) || streq("version", argv[argc]))
        {
            puts(printVersionInfo());
            exit(EXIT_SUCCESS);
        }
        if (streq("--say", argv[argc]) || streq("-s", argv[argc]) || streq("say", argv[argc]))
        {
            say(argv[argc + 1]);
            exit(EXIT_SUCCESS);
        }
    }
    return 0;
}

int main(int argc, char **argv)
{
    int err = 0;
    srand ((unsigned) time(NULL));

    // turn off pocketsphinx output
    err_set_logfp(NULL);
    err_set_debug_level(0);

    // handle command line arguments
    if (argc > 1)
    {
        if ((err = parseArgs(argc, argv)))
        {
            fprintf(stderr, "Error parsing command line arguments.\n");
            return err;
        }
    }
    // initialize pocketsphinx stuff
    cmd_ln_t *config = cmd_ln_init(NULL, ps_args(), true,
                                   "-hmm", MODELDIR "/en-us/en-us",
                                   "-lm", MODELDIR "/en-us/en-us.lm.bin",
                                   "-dict", MODELDIR "/en-us/cmudict-en-us.dict",
                                   NULL);
    if (!config)
    {
        fprintf(stderr, "Failed to create config object, see log for details\n");
        return -1;
    }
    ps_decoder_t *ps = ps_init(config);
    if (!ps)
    {
        fprintf(stderr, "Failed to create recognizer, please update CMU Sphinx software\n");
        return -1;
    }

    // initialize stuff for PortAudio
    AudioData *data = allocAudioData();
    AudioSnippet *sampleBlock = &((AudioSnippet)
                                  {
                                      .size = 0,
                                      .snippet = NULL
                                  });

    PaStream *stream = NULL;
    err = init(&stream, data, sampleBlock);

    // main program loop
    while (!err)
    {
        err = runKhronos(stream, data, sampleBlock, ps);
    }

    //cleanup
    freeAudioData(&data);
    free(sampleBlock->snippet);
    free(stream);
    ps_free(ps);
    cmd_ln_free_r(config);
    Pa_Terminate();
    puts(Pa_GetErrorText(err));
    return err;
}

---

Test Run*:

There is more output visible right now just so the end user can see things are happening internally; hopefully this is reduced in the future. You can see I startup the program, say "test" and wait for the program to process and respond, then say "quit" where the program then exits. Very basic since few commands are implemented, but it shows how the program works.

_{*You can find the code hosted on Github if you would like to try out the code for yourself}

$ ./Khronos
Listening: 0
Listening: 1
Listening: 2
Listening: 3
Listening: 4
Listening: 5
Listening: 6
Listening: 7
Listening: 8
Listening: 9
Wrote to file!!!!
Recognized text: test
Listening: 0
Listening: 1
Listening: 2
Listening: 3
Listening: 4
Listening: 5
Listening: 6
Listening: 7
Listening: 8
Listening: 9
Wrote to file!!!!
Recognized text: quit
$

Answer 1

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?

Answer 2

This isn't a full review, just a couple of points:

Help Text

You have a method printHelp, not included in your code. I'm assuming that it returns a formatted help text that tells the user how to run the program. To get this output currently you need to know the command line arguments for help. Rather than having the user guess until they get it right, why not have this as the default behaviour if the args can't be parsed correctly?

Stdout/Stderr

You've got several methods that refer to stderr and stdout (puts, fprintf). This may be OK if you only ever intend to run the application from the command line. If however, there is a chance that you might want it in a library, or to put a GUI in front of it then decoupling it now may be worthwhile. Putting a wrapper around puts/fprintf something like handleError or sendMessageToUser would minimally change the code, allow the output method to be easily changed in one position.