0

Edit: The reason queue is 2d is because I need a pointer of Command so that cmd can equal NULL. NULL == (void *). This is where I get confused though, and why I've come here. :)

To help try and figure out another problem I have in Python, I'm implementing a small test program in C. While I know a little, apparently I'm confused. I'm trying to write a simple queue to be used in asynchronous USB transfers. Something's not right with queue, as every command popped from the queue is the same. If I write queue[1024][0] as queue[1024][1] instead, the command alternates between two distinct commands, and the program crashes in command_thread_main. Apparently it doesn't notice that cmd should be NULL. Changing [1] any higher has no effect as far as I can tell. Any hints?

typedef struct Command {
    void (*cb) (char *data, int size);
    unsigned char *data;
    int size;
} Command;

struct Command queue[1024][0];

int queueEnd = 0;
int queueStart = 0;

static void queue_push(void (*cb), unsigned char *data, int size) {
    if (queueEnd >= 1024)
        return;
    queue[queueEnd]->cb = cb;
    queue[queueEnd]->data = data;
    queue[queueEnd]->size = size;
    queueEnd++;
}

struct Command * queue_pop(void) {
    if( queueStart > queueEnd )
        return NULL;
    return queue[queueStart++];
}

static void *command_thread_main(void *arg) {
    struct Command *cmd;
    while (!do_exit) {
        if(locked) continue;
        locked = 1;
        cmd = queue_pop();
        if(cmd != NULL)
            cmd->cb(cmd->data, cmd->size);
    }
}
Improve this question
1
  • 2
    Why is the array 2D? that looks wrong Commented Jul 6, 2009 at 8:47

4 Answers 4

Reset to default
3

I think you have a bug you need to fix before anything else. You have a 2D array of commands and have set one of those dimensions to zero! 

struct Command queue[1024][0];

When you access queue you seem to treat it as a 1D structure. Should you declare it as :

struct Command queue[1024];
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Either have queue_pop return &queue[queueStart++] or change the array to be Command *
2
  • Don't you mean struct Command queue[1024];? (That is, no [0] or [1] or whatever.)
  • In queue_pop I think you should test for queueStart >= queueEnd.
  • You should implement a circular array.

Right now you store the struct itself in an array, not pointers to it. That is sensible. You'll need to change -> to . though:

queue[queueEnd].cb = cb;
queue[queueEnd].data = data;
queue[queueEnd].size = size;

(And hence queue_pop should return a variable of type struct Command (not struct Command *), and the main code should also be updated accordingly.)

Of course you can also pass pointers around, but with such a small struct/queue there's no actual need.

Improve this answer

4 Comments

1) I need a pointer. 2) Once queueStart passes queueEnd, the queue's considered empty. When queueStart == queueEnd, queue has at least one member to pop. 3) This queue's going to be used once, with 65 commands at most passed through it. It's a simple test program to satisfy someone who's helping me debug an issue in Python. No circular array needed!
Initially queueStart = queueEnd = 0, so then queueStart > queueEnd is false, while the queue is in fact empty. Your code will never pop the last command.
Makes a little more sense. :)
I'm sorry, I'm saying it the wrong way around: your code pops one value to many (which will result in undefined behavior).
2

As others have pointed out, that 2D queue is definitely wrong. You need a 1D queue, and I suspect that what you want is an array of pointers:

Command * queue[1024];

I reommend you go a way and think about the problem a bit, draw some diagrams and then come back with clearer code and question.

Improve this answer

Comments

1

Another problem is that you've declared queue as an array of structs, but you're using it as an array of pointers to structs by using the dereferencing -> operator instead of the membership . one.

I don't mean to sound snarky, but compiler warning flags (-Wall for gcc) are your friends.

Improve this answer

5 Comments

1) This is why I'm here. It's apparent I'm doing something wrong, but I wasn't sure what that might be. How would you suggest I do this differently? 2) Ah, yeah I'm new to C and compiling C. Thanks for the compiler tip.
Since it's a simple program, the queue is a global variable, and no space in the queue is ever reused I'd just let pop return an index to the popped value, or -1 if the queue is empty. Also, you should use proper mutexes, really.
Well, there's absolutely no need for mutexes. The queue's filled before the threads are started. Like I said, it's a very simply program. I just needed a queue to hold about 65 USB transfers for command_thread_main to consume.
My mistake; I was confused by the use of a 'locked' variable in your example.
Ah yes :) That state changes with each USB transfer (true) and it's corresponding callback (false).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.