Add ISR Queue#2
Conversation
fauniashen
force-pushed
the
feature/can_isr_queue
branch
from
b9cb535 to
46fcc91
Compare
| void handleRxFrame(CanardCANFrame *rxFrame) { | ||
| // TODO: this rx frame is deleted after this function returns. If/when adding a queue, make sure to handle this correctly | ||
| canardHandleRxFrame(data.canard, rxFrame, getUptimeMs() * 1000U); | ||
| //make vrb of canardcanframe in pinecanboard.c make it equal to the frame im going to pop(make a peek), then pass into handlerxframe |
There was a problem hiding this comment.
Just take a look through and remove any comments you wrote during development
Penguronik
left a comment
Penguronik
left a comment
There was a problem hiding this comment.
Great work! Nice job making sure the queue didn't have race conditions that can be tricky but I couldn't find any issues with it. Mostly some nits to follow how we have the internal headers set up, and a few small suggestions.
| // queue functions for circular buffer, included in .h | ||
|
|
||
| bool enqueueRxQueue(const CanardCANFrame *frame) | ||
| { | ||
| if (count >= RX_QUEUE_SIZE) | ||
| return false; // queue full | ||
|
|
||
| rxQueue[head] = *frame; // copy entire frame | ||
| head = (head + 1) % RX_QUEUE_SIZE; | ||
| count++; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool dequeueRxQueue(CanardCANFrame *frame) | ||
| { | ||
| if (count == 0) | ||
| return false; // queue empty | ||
|
|
||
| *frame = rxQueue[tail]; | ||
| tail = (tail + 1) % RX_QUEUE_SIZE; | ||
| count--; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| CanardCANFrame* peekRxQueue() | ||
| { | ||
| if (count == 0) | ||
| return NULL; // queue empty | ||
| return &rxQueue[tail]; | ||
| } | ||
|
|
||
| void processCanardRxQueue() | ||
| { | ||
| CanardCANFrame *nextRxframe = peekRxQueue(); | ||
| if (nextRxframe != NULL) | ||
| handleRxFrame(nextRxframe); | ||
| return; | ||
| } No newline at end of file |
There was a problem hiding this comment.
I think move this stuff to be on board agnostic instead of board specific part of pinecan. looks like they all use the CanardCANFrame type which is common throughout all boards (if they were using CAN_RxHeaderTypeDef for example we'd have to keep it in board specific as that type is different between boards)
There was a problem hiding this comment.
Also file missing newline at end of file, you'll see github show that with a little red circle with line through it at the bottom of the file view. It's not gonna break anything but its best practice to avoid
|
|
||
| void processCanardRxQueue() | ||
| { | ||
| CanardCANFrame *nextRxframe = peekRxQueue(); |
There was a problem hiding this comment.
any reason we peek here and dequeue inside of the function instead of dequeuing here and passing it to handleRxFrame?
There was a problem hiding this comment.
especially since dequeue does a copy of the value anyways so no issue with race conditions i can think of there, and also since we call dequeue later we don't save any cycles by doing a peek here instead
There was a problem hiding this comment.
I agree. handleRxFrame shouldn't be popping the queue
|
|
||
| // queue functions for circular buffer, included in .h | ||
|
|
||
| bool enqueueRxQueue(const CanardCANFrame *frame) |
There was a problem hiding this comment.
Maybe it's worth adding pinecan error convention so that each function returns a specific error code rather than a boolean
There was a problem hiding this comment.
Out of scope for this PR but should think about this now rather than later @SpiroJyro @Penguronik
There was a problem hiding this comment.
Yea wont block this pr but can def create an enum with at least just PC_OK PC_ERROR or smthng and add more as we see fit
There was a problem hiding this comment.
Added PINECAN_ERROR and PINECAN_OK in a new PR, can change this depending on order of PRs that get merged
|
|
||
| void processCanardRxQueue() | ||
| { | ||
| CanardCANFrame *nextRxframe = peekRxQueue(); |
There was a problem hiding this comment.
I agree. handleRxFrame shouldn't be popping the queue
Co-authored-by: Roni Kant <35043400+Penguronik@users.noreply.github.com>
4 participants
Replaces current message processing (happens in ISR) and adds a frame (from hardware queue) to a (software) queue instead. Queue and messages are processed outside of ISR.