Skip to content

fix(ymodem): 修正发送 ACK 重试并统一异常清理 与串口V2在控制台下使用SY失败问题 #11162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wdfk-prog wants to merge 2 commits into
base: master
Choose a base branch
from
+153 −45
Open

fix(ymodem): 修正发送 ACK 重试并统一异常清理 与串口V2在控制台下使用SY失败问题 #11162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0453a7c
fix(serial): only apply CRLF translation in STREAM mode for poll tx
wdfk-prog Feb 1, 2026
881daf2
fix(ymodem): improve send read loop and unify session cleanup
wdfk-prog Feb 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/drivers/serial/dev_serial_v2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ rt_ssize_t _serial_poll_tx(struct rt_device *dev,

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)
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 标志。

Copilot uses AI. Check for mistakes.
{
/* If open_flag satisfies RT_DEVICE_FLAG_STREAM and the received character is '\n',
* inserts '\r' character before '\n' character for the effect of carriage return newline */
Expand Down
26 changes: 21 additions & 5 deletions components/utilities/ymodem/ry_sy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Date Author Notes
* 2019-12-09 Steven Liu the first version
* 2021-04-14 Meco Man Check the file path's legitimacy of 'sy' command
* 2026-02-01 wdfk-prog update ymodem transfer behaviors
*/

#include <rtthread.h>
Expand Down Expand Up @@ -157,20 +158,35 @@ static enum rym_code _rym_send_data(
{
struct custom_ctx *cctx = (struct custom_ctx *)ctx;
rt_size_t read_size;
int retry_read;
int rlen;

read_size = 0;
for (retry_read = 0; retry_read < 10; retry_read++)
/* Loop until we fill one YMODEM data block, hit EOF, or get a read error. */
while (read_size < len)
{
read_size += read(cctx->fd, buf + read_size, len - read_size);
if (read_size == len)
rlen = read(cctx->fd, buf + read_size, len - read_size);
if (rlen > 0)
{
read_size += rlen;
if (read_size == len)
break;
}
else if (rlen == 0)
{
/* EOF: mark finishing so sender switches to EOT after padding. */
ctx->stage = RYM_STAGE_FINISHING;
break;
}
else
{
/* Read error: abort transfer and report file error to the protocol. */
return RYM_ERR_FILE;
}
}

if (read_size < len)
{
rt_memset(buf + read_size, 0x1A, len - read_size);
ctx->stage = RYM_STAGE_FINISHING;
}

if (read_size > 128)
Expand Down
135 changes: 97 additions & 38 deletions components/utilities/ymodem/ymodem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* Date Author Notes
* 2013-04-14 Grissiom initial implementation
* 2019-12-09 Steven Liu add YMODEM send protocol
* 2026-02-01 wdfk-prog update ymodem callbacks and error handling
*/

#include <rthw.h>
Expand Down Expand Up @@ -180,6 +181,27 @@ static rt_err_t _rym_send_packet(
return RT_EOK;
}

/**
* @brief Finalize a transfer, record error state, invoke end callback, and free buffer.
*
* @param ctx Transfer context.
* @param err Transfer result (RT_EOK on success, negative on failure).
*/
static void _rym_cleanup(struct rym_ctx *ctx, rt_err_t err)
{
ctx->last_err = err;
if (ctx->on_end && ctx->begin_called && !ctx->end_called)
{
ctx->on_end(ctx, ctx->buf + 3, 128);
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 以区分成功和失败，但使用可能无效的缓冲区数据调用它可能会产生误导。

Suggested change
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);
}

Copilot uses AI. Check for mistakes.
ctx->end_called = 1;
}
if (ctx->buf)
{
rt_free(ctx->buf);
ctx->buf = RT_NULL;
}
}

static rt_ssize_t _rym_putchar(struct rym_ctx *ctx, rt_uint8_t code)
{
rt_device_write(ctx->dev, 0, &code, sizeof(code));
Expand Down Expand Up @@ -254,7 +276,13 @@ static rt_err_t _rym_do_handshake(

/* congratulations, check passed. */
if (ctx->on_begin && ctx->on_begin(ctx, ctx->buf + 3, data_sz - 5) != RYM_CODE_ACK)
{
return -RYM_ERR_CAN;
}
else
{
ctx->begin_called = 1;
}

return RT_EOK;
}
Expand Down Expand Up @@ -289,12 +317,17 @@ static rt_err_t _rym_do_send_handshake(

/* congratulations, check passed. */
if (ctx->on_begin && ctx->on_begin(ctx, ctx->buf + 3, data_sz - 5) != RYM_CODE_SOH)
{
return -RYM_ERR_CODE;
}
else
{
ctx->begin_called = 1;
}

code = RYM_CODE_SOH;
_rym_send_packet(ctx, code, index);

rt_device_set_rx_indicate(ctx->dev, _rym_rx_ind);
getc_ack = _rym_getchar(ctx);

if (getc_ack != RYM_CODE_ACK)
Expand Down Expand Up @@ -440,25 +473,43 @@ static rt_err_t _rym_do_send_trans(struct rym_ctx *ctx)
rt_size_t data_sz;
rt_uint32_t index = 1;
rt_uint8_t getc_ack;
rt_size_t errors = 0;
rt_uint8_t have_packet = 0;

data_sz = _RYM_STX_PKG_SZ;
while (1)
{
if (!ctx->on_data)
if (!have_packet)
{
return -RYM_ERR_CODE;
if (!ctx->on_data)
{
return -RYM_ERR_CODE;
}
/* Prepare the next payload only once; reuse it on retransmits. */
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)
Comment on lines +489 to +493
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 行之前明确检查代码是否为错误值。

Copilot uses AI. Check for mistakes.
{
return -RYM_ERR_CODE;
}

getc_ack = _rym_getchar(ctx);
if (getc_ack != RYM_CODE_ACK)
{
return -RYM_ERR_ACK;
if (getc_ack == RYM_CODE_CAN)
return -RYM_ERR_CAN;
Comment on lines +501 to +502
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 风格指南要求控制结构的大括号独占一行。

Copilot generated this review using guidance from repository custom instructions.

if (++errors > RYM_MAX_ERRORS)
return -RYM_ERR_ACK;
Comment on lines +504 to +505
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 风格指南要求控制结构的大括号独占一行。

Copilot generated this review using guidance from repository custom instructions.

/* Not ACK: retry the same packet until we hit the error limit. */
continue;
}
errors = 0;
have_packet = 0;
index++;

if (ctx->stage == RYM_STAGE_FINISHING)
break;
Expand All @@ -478,7 +529,10 @@ static rt_err_t _rym_do_fin(struct rym_ctx *ctx)
/* we already got one EOT in the caller. invoke the callback if there is
* one. */
if (ctx->on_end)
{
ctx->on_end(ctx, ctx->buf + 3, 128);
ctx->end_called = 1;
}

_rym_putchar(ctx, RYM_CODE_NAK);
code = _rym_read_code(ctx, RYM_WAIT_PKG_TICK);
Expand Down Expand Up @@ -538,7 +592,6 @@ static rt_err_t _rym_do_send_fin(struct rym_ctx *ctx)
rt_uint8_t getc_ack;

data_sz = _RYM_SOH_PKG_SZ;
rt_device_set_rx_indicate(ctx->dev, _rym_rx_ind);

_rym_putchar(ctx, RYM_CODE_EOT);
getc_ack = _rym_getchar(ctx);
Expand All @@ -564,7 +617,13 @@ static rt_err_t _rym_do_send_fin(struct rym_ctx *ctx)
}

if (ctx->on_end && ctx->on_end(ctx, ctx->buf + 3, data_sz - 5) != RYM_CODE_SOH)
{
return -RYM_ERR_CODE;
}
else
{
ctx->end_called = 1;
}

code = RYM_CODE_SOH;

Expand All @@ -582,32 +641,32 @@ static rt_err_t _rym_do_recv(
rt_err_t err;

ctx->stage = RYM_STAGE_NONE;
ctx->begin_called = 0;
ctx->end_called = 0;
ctx->last_err = RT_EOK;

ctx->buf = rt_malloc(_RYM_STX_PKG_SZ);
if (ctx->buf == RT_NULL)
return -RT_ENOMEM;

err = _rym_do_handshake(ctx, handshake_timeout);
if (err != RT_EOK)
{
rt_free(ctx->buf);
return err;
}
goto __cleanup;
while (1)
{
err = _rym_do_trans(ctx);
if (err != RT_EOK)
goto __cleanup;

err = _rym_do_fin(ctx);
if (err != RT_EOK)
{
rt_free(ctx->buf);
return err;
}
goto __cleanup;

if (ctx->stage == RYM_STAGE_FINISHED)
break;
}
rt_free(ctx->buf);
__cleanup:
_rym_cleanup(ctx, err);
return err;
}

Expand All @@ -618,33 +677,27 @@ static rt_err_t _rym_do_send(
rt_err_t err;

ctx->stage = RYM_STAGE_NONE;
ctx->begin_called = 0;
ctx->end_called = 0;
ctx->last_err = RT_EOK;

ctx->buf = rt_malloc(_RYM_STX_PKG_SZ);
if (ctx->buf == RT_NULL)
return -RT_ENOMEM;

err = _rym_do_send_handshake(ctx, handshake_timeout);
if (err != RT_EOK)
{
rt_free(ctx->buf);
return err;
}
goto __cleanup;

err = _rym_do_send_trans(ctx);
if (err != RT_EOK)
{
rt_free(ctx->buf);
return err;
}
goto __cleanup;

err = _rym_do_send_fin(ctx);
if (err != RT_EOK)
{
rt_free(ctx->buf);
return err;
}

rt_free(ctx->buf);
goto __cleanup;
__cleanup:
_rym_cleanup(ctx, err);
return err;
}

Expand All @@ -666,9 +719,12 @@ rt_err_t rym_recv_on_device(
_rym_the_ctx = ctx;

ctx->on_begin = on_begin;
ctx->on_data = on_data;
ctx->on_end = on_end;
ctx->dev = dev;
ctx->on_data = on_data;
ctx->on_end = on_end;
ctx->begin_called = 0;
ctx->end_called = 0;
ctx->last_err = RT_EOK;
ctx->dev = dev;
rt_sem_init(&ctx->sem, "rymsem", 0, RT_IPC_FLAG_FIFO);

odev_rx_ind = dev->rx_indicate;
Expand Down Expand Up @@ -723,9 +779,12 @@ rt_err_t rym_send_on_device(
_rym_the_ctx = ctx;

ctx->on_begin = on_begin;
ctx->on_data = on_data;
ctx->on_end = on_end;
ctx->dev = dev;
ctx->on_data = on_data;
ctx->on_end = on_end;
ctx->begin_called = 0;
ctx->end_called = 0;
ctx->last_err = RT_EOK;
ctx->dev = dev;
rt_sem_init(&ctx->sem, "rymsem", 0, RT_IPC_FLAG_FIFO);

odev_rx_ind = dev->rx_indicate;
Expand Down
35 changes: 34 additions & 1 deletion components/utilities/ymodem/ymodem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* 2013-04-14 Grissiom initial implementation
* 2019-12-09 Steven Liu add YMODEM send protocol
* 2022-08-04 Meco Man move error codes to rym_code to silence warnings
* 2026-02-01 wdfk-prog update ymodem callbacks and error handling
*/

#ifndef __YMODEM_H__
Expand Down Expand Up @@ -81,13 +82,22 @@ enum rym_stage
};

struct rym_ctx;
/* When receiving files, the buf will be the data received from ymodem protocol
/**
* @brief YMODEM callback signature.
*
* @param ctx The context of the current session.
*
* @note When receiving files, the buf will be the data received from ymodem protocol
* and the len is the data size.
*
* When sending files, the len is the buf size in RYM. The callback need to
* fill the buf with data to send. Returning RYM_CODE_EOT will terminate the
* transfer and the buf will be discarded. Any other return values will cause
* the transfer continue.
*
* @note Keep this typedef unchanged for compatibility with external packages.
* To allow error-aware handling without breaking ABI, add state fields
* (e.g. ctx->last_err) in rym_ctx for callbacks to inspect.
*/
typedef enum rym_code(*rym_callback)(struct rym_ctx *ctx, rt_uint8_t *buf, rt_size_t len);

Expand All @@ -98,14 +108,37 @@ typedef enum rym_code(*rym_callback)(struct rym_ctx *ctx, rt_uint8_t *buf, rt_si
*/
struct rym_ctx
{
/**
* @brief Data callback for each received/sent block.
*/
rym_callback on_data;
/**
* @brief Begin callback for the initial header block.
*/
rym_callback on_begin;
/**
* @brief End callback for session finalization.
* Callers should check ctx->last_err to distinguish success vs failure.
*/
rym_callback on_end;
/* When error happened, user need to check this to get when the error has
* happened. */
enum rym_stage stage;
/* user could get the error content through this */
rt_uint8_t *buf;
/**
* @brief Callback lifecycle state: set when on_begin succeeds.
*/
rt_uint8_t begin_called;
/**
* @brief Callback lifecycle state: set when on_end is invoked.
*/
rt_uint8_t end_called;
/**
* @brief Last transfer error seen by the core state machine.
* on_end can inspect this to distinguish success vs failure.
*/
rt_err_t last_err;

struct rt_semaphore sem;

Expand Down