[driver][serial V1]: fix correct data loss logic when RX ring buffer is full #10590
Conversation
… is full In the serial ISR (`rt_hw_serial_isr`), the previous logic for handling a full RX FIFO was flawed. When the buffer was filled, it would increment `get_index` (`get_index += 1`). This had two negative consequences: 1. It effectively discarded the oldest byte of data prematurely. 2. It reduced the usable capacity of a buffer of size N to N-1. For example, a 64-byte buffer could only ever hold 63 readable bytes after becoming full. This patch corrects the behavior by implementing a standard overwriting ring buffer strategy. When the buffer is full, the logic is changed to `get_index = put_index`. This ensures that: - When new data arrives, it correctly overwrites the oldest data. - The `get_index` is advanced along with the `put_index`, correctly marking the new start of the buffer. - The full N-byte capacity of the buffer is utilized, always storing the N most recent bytes. This change resolves the unexpected data loss and makes the buffer behavior correct and predictable.
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-08-09 09:50 CST)
📝 Review Instructions
|
wdfk-prog
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a bug in the serial driver's receive interrupt handling where the ring buffer logic was incorrectly discarding data and reducing usable buffer capacity. The fix changes from incrementing get_index to setting it equal to put_index when the buffer is full, implementing proper overwriting ring buffer behavior.
- Corrects data loss logic in the RX ring buffer when full
- Ensures full buffer capacity utilization (N bytes instead of N-1)
- Implements standard overwriting ring buffer strategy
| if (rx_fifo->put_index == rx_fifo->get_index) | ||
| { | ||
| rx_fifo->get_index += 1; | ||
| rx_fifo->get_index = rx_fifo->put_index; |
There was a problem hiding this comment.
Setting get_index equal to put_index when the buffer is full creates an empty buffer state (get_index == put_index typically indicates empty). This contradicts the is_full flag being set to RT_TRUE on the next line and may cause inconsistent buffer state interpretation elsewhere in the code.
| rx_fifo->get_index = rx_fifo->put_index; | |
| rx_fifo->get_index = (rx_fifo->get_index + 1) % serial->config.bufsz; |
2 participants
In the serial ISR (
rt_hw_serial_isr), the previous logic for handling a full RX FIFO was flawed. When the buffer was filled, it would incrementget_index(get_index += 1).This had two negative consequences:
This patch corrects the behavior by implementing a standard overwriting ring buffer strategy. When the buffer is full, the logic is changed to
get_index = put_index.This ensures that:
get_indexis advanced along with theput_index, correctly marking the new start of the buffer.This change resolves the unexpected data loss and makes the buffer behavior correct and predictable.
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up