framebuffer: add configurable frame lifetime and drop stale frames#87
framebuffer: add configurable frame lifetime and drop stale frames#87maxwelllls wants to merge 1 commit into
Conversation
Add a configurable frame lifetime for buffered CAN/CAN FD frames. This change introduces the `-g` command-line option to configure the maximum frame age in microseconds (default: 1000000, `0` disables the check), and passes that setting to both the network and CAN frame buffers. FrameBuffer now stores an enqueue timestamp for each frame using a monotonic clock and drops expired frames when reading from the main buffer. It also removes expired frames from the intermediate buffer before they can be forwarded. This avoids replaying large amounts of stale buffered traffic after the bus or transport path recovers.
mguentner
left a comment
mguentner
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Can you please clarify under which circumstances you encounter issues that require this feature?
I am not sure yet that the issues need to be solved by that amount of complexity.
As I have outlined, I believe that the "CAN -> Network" path may only require solving for TCP as UDP clears out frames "automatically" as it is fire and forget.
Note:
I actually I see more of an issue here with CANThread. Imagine that here we cannot send for a long time. The CANThread aggressively tries to transmit frames. I think there should be a limit, either temporal (like your PR) or attempts. Since we do not differentiate between "bus is broken" and "arbitrage not possible due to bus load", counting loops is kinda hard.
CANThread could maintain its own map of *frame -> timestamp which it uses to determine whether a frame should be retried on the next run or dropped.
| uint16_t localPort = 20000; | ||
| std::string canInterfaceName = "vcan0"; | ||
| uint32_t bufferTimeout = 100000; | ||
| uint32_t frameLifetime = 1000000; |
There was a problem hiding this comment.
This is 10x of the bufferTimeout. Since bufferTimeout < frameLifetime, this is already solved for UDP connections (at least in the default settings):
- after a maximum of 100ms, the frames will have been sent
- the framebuffer will no longer contain these frames
However since TCP connections require an active peer, here we need to deal with the TCP connection
becoming stale.
In TcpThread::flushFrameBuffer, cannelloni disconnects on write errors - this will happen after some time on a stale TCP connection. But here some frames may remain in the buffer, all the ones that could not be sent.
| ssize_t encodedBytes = encodeFrame(transmitBuffer, frame); | ||
| ssize_t bytesWritten = send(m_socket, transmitBuffer, encodedBytes, 0); | ||
| if (encodedBytes != bytesWritten) { | ||
| disconnect(); |
There was a problem hiding this comment.
As written above, here we could remove all frames from the intermediateBuffer (put them back to the pool).
|
Thanks for the detailed feedback. I have opened an issue describing the full problem scenario, including the background, the observed behavior, and the specific kind of change that is actually needed to address it. After reviewing the code and your comments again, I realized that my current PR goes beyond the minimal scope required to fix that issue. This is on my side, and is mainly due to my still limited understanding of some parts of the codebase when I first tried to implement the change. I will rework this PR, reduce the scope, and submit a narrower set of commits focused only on the specific issue. I need a bit more time to reproduce and validate the revised behavior properly on my setup before pushing the updated commits here. |
2 participants
Add a configurable frame lifetime for buffered CAN/CAN FD frames.
This change introduces the
-gcommand-line option to configure the maximum frame age in microseconds (default: 1000000,0disables the check), and passes that setting to both the network and CAN frame buffers.FrameBuffer now stores an enqueue timestamp for each frame using a monotonic clock and drops expired frames when reading from the main buffer. It also removes expired frames from the intermediate buffer before they can be forwarded.
This avoids replaying large amounts of stale buffered traffic after the bus or transport path recovers.