fix(ymodem): 修正发送 ACK 重试并统一异常清理 与串口V2在控制台下使用SY失败问题#11162
fix(ymodem): 修正发送 ACK 重试并统一异常清理 与串口V2在控制台下使用SY失败问题#11162wdfk-prog wants to merge 2 commits intoRT-Thread:masterfrom
Conversation
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
📌 Code Review Assignment🏷️ Tag: componentsReviewers: Maihuanyi Changed Files (Click to expand)
🏷️ Tag: components_driver_serial_v2Reviewers: Ryan-CW-Code Changed Files (Click to expand)
🏷️ Tag: components_utilities_ymodemReviewers: wdfk-prog Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-02-01 17:06 CST)
📝 Review Instructions
|
- Accumulate short reads, mark finishing on EOF - Track begin/end callback states and cleanup on error paths - Add ACK handling with retry/error counting in send flow
wdfk-prog
changed the title
There was a problem hiding this comment.
Pull request overview
Changes:
- Added YMODEM session lifecycle tracking with begin_called, end_called, and last_err fields to prevent resource leaks and allow callbacks to distinguish success from failure
- Implemented unified cleanup function (_rym_cleanup) that properly frees buffers and invokes end callbacks on all exit paths
- Added ACK retry logic with error counting (up to RYM_MAX_ERRORS) in _rym_do_send_trans to handle transient transmission failures
- Fixed _rym_send_data to correctly handle short reads and EOF by looping until buffer is filled or end-of-file is reached
- Removed redundant rt_device_set_rx_indicate calls from internal transfer functions (already set in outer functions)
- Modified serial V2 driver to only perform CRLF conversion when RT_DEVICE_FLAG_STREAM is set, removing special handling for console devices
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| components/utilities/ymodem/ymodem.h | Added session lifecycle fields (begin_called, end_called, last_err) to rym_ctx structure with documentation |
| components/utilities/ymodem/ymodem.c | Implemented _rym_cleanup function, added ACK retry logic, unified error handling with goto cleanup pattern, removed redundant rx_indicate calls |
| components/utilities/ymodem/ry_sy.c | Fixed file read loop to handle short reads and EOF correctly |
| components/drivers/serial/dev_serial_v2.c | Removed console device special case from _serial_poll_tx CRLF conversion logic |
| ctx->last_err = err; | ||
| if (ctx->on_end && ctx->begin_called && !ctx->end_called) | ||
| { | ||
| ctx->on_end(ctx, ctx->buf + 3, 128); |
There was a problem hiding this comment.
Potential issue with on_end callback parameters / on_end 回调参数可能存在问题
English: The _rym_cleanup function always calls on_end with ctx->buf + 3 and a fixed size of 128 bytes. However, ctx->buf may contain invalid or incomplete data if an error occurred before any packets were received. Consider either passing NULL/0 or ensuring the buffer contains valid data before invoking the callback. Additionally, the callback is documented to allow inspection of ctx->last_err to distinguish success from failure, but calling it with potentially invalid buffer data could be misleading.
中文：_rym_cleanup 函数总是使用 ctx->buf + 3 和固定的 128 字节大小调用 on_end。然而，如果在接收任何数据包之前发生错误，ctx->buf 可能包含无效或不完整的数据。考虑传递 NULL/0 或在调用回调之前确保缓冲区包含有效数据。此外，回调被记录为允许检查 ctx->last_err 以区分成功和失败，但使用可能无效的缓冲区数据调用它可能会产生误导。
| ctx->on_end(ctx, ctx->buf + 3, 128); | |
| if ((err == RT_EOK) && (ctx->buf != RT_NULL)) | |
| { | |
| /* Successful transfer, pass metadata buffer to callback */ | |
| ctx->on_end(ctx, ctx->buf + 3, 128); | |
| } | |
| else | |
| { | |
| /* Failed or no valid buffer, callback inspects ctx->last_err */ | |
| ctx->on_end(ctx, RT_NULL, 0); | |
| } |
| if (getc_ack == RYM_CODE_CAN) | ||
| return -RYM_ERR_CAN; |
There was a problem hiding this comment.
Missing braces around multi-statement if block / 多语句 if 块缺少大括号
English: The if statement on line 501 should use braces for the return statement to follow RT-Thread coding standards. While the code is functionally correct, RT-Thread style guide requires braces on separate lines for control structures.
中文：第 501 行的 if 语句应该为 return 语句使用大括号，以遵循 RT-Thread 编码标准。虽然代码功能正确，但 RT-Thread 风格指南要求控制结构的大括号独占一行。
| code = ctx->on_data(ctx, ctx->buf + 3, data_sz - 5); | ||
| have_packet = 1; | ||
| } | ||
| code = ctx->on_data(ctx, ctx->buf + 3, data_sz - 5); | ||
|
|
||
| _rym_send_packet(ctx, code, index); | ||
| index++; | ||
| rt_device_set_rx_indicate(ctx->dev, _rym_rx_ind); | ||
| if (_rym_send_packet(ctx, code, index) != RT_EOK) |
There was a problem hiding this comment.
Missing error handling for callback return value / 缺少对回调返回值的错误处理
English: The code retrieves the callback return value at line 489 but doesn't check if it's an error code (like RYM_ERR_FILE) before attempting to send the packet. If the callback returns an error code such as RYM_ERR_FILE (0x77), it will be passed to _rym_send_packet which only handles RYM_CODE_SOH and RYM_CODE_STX in its switch statement, causing it to return -RT_ERROR. This should be handled explicitly by checking if the code is an error value before line 493.
中文：代码在第 489 行获取回调返回值，但在尝试发送数据包之前没有检查它是否是错误代码（如 RYM_ERR_FILE）。如果回调返回错误代码如 RYM_ERR_FILE (0x77)，它将被传递给 _rym_send_packet，而该函数的 switch 语句只处理 RYM_CODE_SOH 和 RYM_CODE_STX，导致返回 -RT_ERROR。应该在第 493 行之前明确检查代码是否为错误值。
| if (++errors > RYM_MAX_ERRORS) | ||
| return -RYM_ERR_ACK; |
There was a problem hiding this comment.
Missing braces around multi-statement if block / 多语句 if 块缺少大括号
English: The if statement on line 504 should use braces for the return statement to follow RT-Thread coding standards. While the code is functionally correct, RT-Thread style guide requires braces on separate lines for control structures.
中文：第 504 行的 if 语句应该为 return 语句使用大括号，以遵循 RT-Thread 编码标准。虽然代码功能正确，但 RT-Thread 风格指南要求控制结构的大括号独占一行。
| while (size) | ||
| { | ||
| if (serial->parent.open_flag & RT_DEVICE_FLAG_STREAM || (dev == rt_console_get_device())) | ||
| if (serial->parent.open_flag & RT_DEVICE_FLAG_STREAM) |
There was a problem hiding this comment.
Potential breaking change for console device behavior / 控制台设备行为的潜在破坏性更改
English: This change removes the special handling for console devices in _serial_poll_tx. Previously, console devices would get CRLF conversion (CR insertion before LF) regardless of the STREAM flag. Now, CRLF conversion only happens when RT_DEVICE_FLAG_STREAM is set. While this fix correctly allows YMODEM to disable line ending conversion by clearing the STREAM flag, it could affect other code paths where console devices are used without explicitly setting the STREAM flag. Verify that all console device usage paths properly set the STREAM flag when CRLF conversion is expected.
中文：此更改删除了 _serial_poll_tx 中对控制台设备的特殊处理。以前，无论 STREAM 标志如何，控制台设备都会进行 CRLF 转换（在 LF 之前插入 CR）。现在，只有当设置了 RT_DEVICE_FLAG_STREAM 时才会进行 CRLF 转换。虽然此修复正确地允许 YMODEM 通过清除 STREAM 标志来禁用行尾转换，但它可能影响其他未明确设置 STREAM 标志的控制台设备使用路径。请验证所有控制台设备使用路径在需要 CRLF 转换时都正确设置了 STREAM 标志。
1 participant
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
本 PR 主要修复/增强 YMODEM 发送与会话清理流程，并调整串口 poll tx 的 CRLF 行为：
你的解决方案是什么 (what is your solution)
YMODEM 发送：改为循环读取文件数据，正确处理短读/EOF
YMODEM 会话：新增 begin/end 回调状态并统一 cleanup，避免 buf 泄漏
YMODEM 发送：增加 ACK 错误计数与重试逻辑
串口：poll tx 不再对 console 设备做额外 CR 插入，仅在 STREAM 模式下转换
\n优化发送一帧\r;导致sy 一帧1024的数据包长度超出标准协议与CRC校验失败请提供验证的bsp和config (provide the config and bsp)
sy file_path [uartX]发送大文件与小文件各一次ry file_path [uartX]接收，验证异常中断后资源释放/文件句柄关闭是否正常\n的日志，验证终端是否仍显示正常换行（确认 console open_flag 是否含 STREAM）]
当前拉取/合并请求的状态 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