2
\$\begingroup\$

I have a program that monitors real-time air waves and demodulates them into useable data. The program runs fine and with very little issues over time. The only thing I am really interested in is whether the calls to memcpy can be handled differently.

I have tried using valgrind and this code does not seem to be leaking memory.

static output_buffer o_buffers[MAX_OUTPUT_BUFF_COUNT]; //MAX_OUTPUT_BUFF_COUNT = 100
static int o_buffer_count = 0;

struct output_buffer
{
    int16_t buf[MAXIMUM_BUF_LENGTH]; //= 262144
    int len;
};

static void rtlsdr_data_callback(unsigned char *buf, uint32_t len, void *ctx)
{
    if (do_exit)
        return;
    rtl_dev *dev_ctx = (rtl_dev *)ctx;

    demod_state *d = &dev_ctx->demod;

    rotate_90(buf, len);

    int16_t buf16[MAXIMUM_BUF_LENGTH];

    //normalize the data
    for (int i = 0; i < (int)len; i++)
        buf16[i] = (int16_t)buf[i] - 127;

    //create a new output buffer
    output_buffer o;
    memcpy(o.buf, buf16, 2 * len);
    o.len = len;

    if (do_exit)
        return;

    //notify the demodulation thread
    pthread_rwlock_wrlock(&d->rw);
    o_buffers[o_buffer_count] = o;
    o_buffer_count++;
    pthread_rwlock_unlock(&d->rw);
}

static void *demod_thread_fn(void *arg)
{
    rtl_dev *ctx = (rtl_dev *)arg;

    demod_state *d = &ctx->demod;

    int fbuf_cnt = 0;
    float fbuf[MAX_DEMOD_BUFFERSIZE];
    int16_t *sp;

    d->lowpassed[0] = 0;

    while (!do_exit)
    {
        if (do_exit)
        {

            return 0;
        }
        if (!o_buffer_count)
        {
            continue;
        }

        //copy the buffers and reset the counter.. TODO: make quicker, watch for leaks
        pthread_rwlock_wrlock(&d->rw);
        int old_buffer_count = o_buffer_count;
        output_buffer *old_buffers = (output_buffer *)malloc(sizeof(output_buffer) * MAX_OUTPUT_BUFF_COUNT);
        memcpy(old_buffers, o_buffers, sizeof(output_buffer) * MAX_OUTPUT_BUFF_COUNT);

        o_buffer_count = 0;
        pthread_rwlock_unlock(&d->rw);

        for (int i = 0; i < old_buffer_count; i++)
        {
            output_buffer o = old_buffers[i];

            memcpy(d->lowpassed, o.buf, 2 * o.len);
            d->lp_len = o.len;
            //do work with the data...
        }
        free(old_buffers);
    }

    return 0;
}

Now the actual program is quite large so a working sample will be very hard to create.

However, some notes:

  1. pthread_rwlock_wrlock(&d->rw); is referencing a pthread_rwlock_t rw;
  2. rtlsdr_data_callback is called as soon as a buffer is ready. This method needs to exit as quick as possible to prevent data loss.
  3. demod_thread_fn is not as important for performance as we are on another thread not responding to real-time airwaves however the block pthread_rwlock_wrlock(&d->rw); to pthread_rwlock_unlock(&d->rw); will consequently block the rtlsdr_data_callback
  4. The "air-waves" are simple FLEX pager waves and transmissions may be within milliseconds.
\$\endgroup\$
  • 2
    \$\begingroup\$ malloc and free? This should really be tagged C and not C++ imo. \$\endgroup\$ – const_ref Apr 18 '17 at 10:08
  • \$\begingroup\$ Bah. I've tried a couple of times now to mouse over stuff so IntelliSense will tell me what it is. :P A sign VS might be spoiling me. \$\endgroup\$ – cHao Apr 18 '17 at 16:24
  • \$\begingroup\$ "I have a program that monitors real-time air waves and demodulates them into useable data". Would that be a "tuner"? \$\endgroup\$ – Barry Carter Apr 18 '17 at 18:02
1
\$\begingroup\$
  • This loop looks odd...

    static void *demod_thread_fn(void *arg)
    {
        while (!do_exit)
        {
            if (do_exit)
            {
                return 0;
            }
    
            ...
        }
    
        return 0;
    }
    

    Looks to me like you can get rid of the if. If do_exit can change between the first check and the second, and that's important, then you have a race condition -- it could change after the second check as well. No amount of rechecking can provide you any assurances.

  • Consider using a semaphore rather than busy-waiting for o_buffer_count to change.

  • Decide whether this is to be C or C++. :P If it is supposed to be C++, then you should be using new and delete rather than malloc and free. If it's not, then you ought to either typedef your struct types or actually say struct output_buffer etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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