Convert PLUART to queue for sensor lines (#16)#361
Convert PLUART to queue for sensor lines (#16)#361keenanjohnson wants to merge 2 commits intobristlemouth:developfrom
Conversation
|
Just ran across this project in open source and seems like a great effort! I've done a few ocean robotics projects in the past and always looking for ways to give back. I saw that this issue was open and seemed straightforward, but let me know if it's not helpful or I am headed in the wrong direction. |
|
Thanks @keenanjohnson! Please work on fixing the CI failure, and we'll help with some hardware validation. |
|
Great! The latest push fixes the tests I think |
|
Hey @keenanjohnson CI is still failing — you can see the ❌ next to the latest commit and the "All checks have failed" notice at the bottom fo the PR. Here's a screenshot of the build failure: |
matt001k
left a comment
matt001k
left a comment
There was a problem hiding this comment.
Nice, I like this change a lot!
I will give it a shot on actual hardware here soon (give me at most a week)!
| _user_line.len = len; | ||
| _user_line.ready = true; | ||
| xSemaphoreGive(_user_line.mutex); | ||
| QueuedLine_t queued_line; |
There was a problem hiding this comment.
Ah this may be a problem here. Every time a line ends we create a queue_line item, this struct has a buffer of 2048 bytes in it. Meaning, 2048+ bytes are allocated to the `serialGenericRxTask. Looking at that task the stack is currently 2048 words (32bit) wide which should be enough for this operation.
Note: I tested this on an application and it seems to be running ok.
There was a problem hiding this comment.
Also to note, this is not occurring from an ISR. We do not need the xQueueSendFromISR function here.
The xQueueSend function will also copy the above struct (queue_line) and heap allocate it. Meaning every time a line is created >4096 bytes of allocation happen (until queued_line goes out of scope).
There was a problem hiding this comment.
I would recommend creating some memory pool that the queue items could point to.
There is a module I created exactly for something like this called q.c located in the bm_core submodule of this project (the submodule might have to be updated). This could be a good alternative as:
- There is no max queue size, it is all dependent of how much memory in the pool has been used
- Elements in the queue can be of different sizes
- It is much faster in terms of operation than the FreeRTOS queue
The only downfall is:
- It is not thread safe and will need mutex protection
Let me know your thoughts on that solution!
| BaseType_t xHigherPriorityTaskWoken = pdFALSE; | ||
| if (xQueueSendFromISR(_line_queue, &queued_line, &xHigherPriorityTaskWoken) != pdTRUE) { | ||
| // Queue is full - increment dropped line counter | ||
| // Use assignment to avoid C++20 deprecated volatile increment |
There was a problem hiding this comment.
Wow did not know about this one... that is a crazy deprecation
3 participants
What changed?
Implements queue-based line buffering for PLUART to prevent data loss when multiple lines arrive before being read.
How does it make Bristlemouth better?
Issue #16 raised by the devs seems to indicate a need and makes sense to me! As mentioned in the issue, many sensors issue multiple lines and this change seems to only make the project more robust to timing jitter without changing the user API.
Where should reviewers focus?
src/lib/common/payload_uart.cpp contains the important bits. I have added a set of basic unit tests as well.
Checklist