Skip to content

fix(drv_can): resolve issues with reopening after close and TX mailbox blockage #10898

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
wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −17
Open

fix(drv_can): resolve issues with reopening after close and TX mailbox blockage #10898

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 18 additions & 17 deletions bsp/stm32/libraries/HAL_Drivers/drivers/drv_can.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,10 +472,11 @@ static rt_err_t _can_control(struct rt_can_device *can, int cmd, void *arg)
argval = (rt_uint32_t) arg;
if (argval == 0)
{
HAL_CAN_Stop(&drv_can->CanHandle);
HAL_CAN_DeInit(&drv_can->CanHandle);
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Best Practices / 最佳实践: Missing error handling for HAL_CAN_DeInit / 缺少 HAL_CAN_DeInit 的错误处理

English: The return value of HAL_CAN_DeInit is not checked. While less critical than initialization errors, deinitialization failures could indicate resource cleanup issues. Consider checking and logging or returning the error status for better error visibility and debugging.

中文：未检查 HAL_CAN_DeInit 的返回值。虽然不如初始化错误那么关键，但反初始化失败可能表明资源清理问题。考虑检查并记录或返回错误状态，以获得更好的错误可见性和调试能力。

Suggested change
HAL_CAN_DeInit(&drv_can->CanHandle);
if (HAL_CAN_DeInit(&drv_can->CanHandle) != HAL_OK)
{
LOG_E("CAN deinitialization failed");
return -RT_ERROR;
}

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Maintainability / 可维护性: Incomplete CAN stop/start implementation / CAN 停止/启动实现不完整

English: Using HAL_CAN_DeInit when stopping the CAN (argval == 0) without a corresponding HAL_CAN_Init when starting (argval != 0) creates an asymmetric initialization pattern. Line 479 calls _can_config which internally calls HAL_CAN_Init, but this is not explicit. Consider using HAL_CAN_Stop for stopping and ensuring proper initialization/deinitialization pairs for clarity.

中文：当停止 CAN 时（argval == 0）使用 HAL_CAN_DeInit，但在启动时（argval != 0）没有对应的显式 HAL_CAN_Init 调用，这创建了不对称的初始化模式。虽然第 479 行调用的 _can_config 内部会调用 HAL_CAN_Init，但这不够明确。建议停止时使用 HAL_CAN_Stop，并确保初始化/反初始化成对出现以提高代码清晰度。

Suggested change
HAL_CAN_DeInit(&drv_can->CanHandle);
HAL_CAN_Stop(&drv_can->CanHandle);

Copilot uses AI. Check for mistakes.
}
else
{
_can_config(&drv_can->device, &drv_can->device.config);
HAL_CAN_Start(&drv_can->CanHandle);
Comment on lines +479 to 480
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Bug / 错误: Missing error handling for CAN configuration and start / 缺少 CAN 配置和启动的错误处理

English: Neither the return value of _can_config nor HAL_CAN_Start is checked. If either operation fails, the function incorrectly returns RT_EOK (success) on line 486. Both errors should be checked and propagated to the caller. This is especially important for CAN configuration which can fail for various reasons (e.g., invalid baud rate parameters). Other cases in this function properly check _can_config return values (see lines 423, 443, 456).

中文：既未检查 _can_config 的返回值，也未检查 HAL_CAN_Start 的返回值。如果任一操作失败，函数会在第 486 行错误地返回 RT_EOK（成功）。两个错误都应该被检查并传播给调用者。这对于 CAN 配置尤其重要，因为它可能因各种原因失败（例如，无效的波特率参数）。此函数中的其他情况正确检查了 _can_config 返回值（参见第 423、443、456 行）。

Example / 示例:

rt_err_t result = _can_config(&drv_can->device, &drv_can->device.config);
if (result != RT_EOK)
{
    return result;
}
if (HAL_CAN_Start(&drv_can->CanHandle) != HAL_OK)
{
    return -RT_ERROR;
}
Suggested change
_can_config(&drv_can->device, &drv_can->device.config);
HAL_CAN_Start(&drv_can->CanHandle);
rt_err_t result = _can_config(&drv_can->device, &drv_can->device.config);
if (result != RT_EOK)
{
return result;
}
if (HAL_CAN_Start(&drv_can->CanHandle) != HAL_OK)
{
return -RT_ERROR;
}

Copilot uses AI. Check for mistakes.
}

Expand Down Expand Up @@ -515,32 +516,32 @@ static int _can_sendmsg(struct rt_can_device *can, const void *buf, rt_uint32_t
(state == HAL_CAN_STATE_LISTENING))
{
/*check select mailbox is empty */
uint32_t mailbox_mask;
uint32_t tme_flag;

switch (1 << box_num)
{
case CAN_TX_MAILBOX0:
if (HAL_IS_BIT_SET(hcan->Instance->TSR, CAN_TSR_TME0) != SET)
{
/* Return function status */
return -RT_ERROR;
}
mailbox_mask = CAN_TX_MAILBOX0;
tme_flag = CAN_TSR_TME0;
break;
case CAN_TX_MAILBOX1:
if (HAL_IS_BIT_SET(hcan->Instance->TSR, CAN_TSR_TME1) != SET)
{
/* Return function status */
return -RT_ERROR;
}
mailbox_mask = CAN_TX_MAILBOX1;
tme_flag = CAN_TSR_TME1;
break;
case CAN_TX_MAILBOX2:
if (HAL_IS_BIT_SET(hcan->Instance->TSR, CAN_TSR_TME2) != SET)
{
/* Return function status */
return -RT_ERROR;
}
mailbox_mask = CAN_TX_MAILBOX2;
tme_flag = CAN_TSR_TME2;
break;
default:
RT_ASSERT(0);
break;
return -RT_ERROR;
}

if (HAL_IS_BIT_SET(hcan->Instance->TSR, tme_flag) != SET)
{
HAL_CAN_AbortTxRequest(hcan, mailbox_mask);
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Best Practices / 最佳实践: Changed mailbox handling behavior without clear justification / 更改邮箱处理行为但缺乏明确理由

English: The new implementation automatically aborts a pending transmission request using HAL_CAN_AbortTxRequest when the mailbox is not empty. This changes the behavior from simply returning an error to actively aborting the previous transmission. This could lead to message loss if a previous transmission was in progress. The original behavior of returning an error and letting the caller handle the busy mailbox was more predictable. If aborting is necessary, this should be documented and the implications should be carefully considered, especially for real-time CAN applications where message timing is critical.

中文：新实现在邮箱非空时使用 HAL_CAN_AbortTxRequest 自动中止待处理的传输请求。这将行为从简单返回错误改为主动中止先前的传输。如果先前的传输正在进行中，这可能导致消息丢失。原来的行为是返回错误并让调用者处理繁忙的邮箱，这更加可预测。如果必须中止，应该记录此行为并仔细考虑其影响，特别是对于消息时序至关重要的实时 CAN 应用。

Suggested change
HAL_CAN_AbortTxRequest(hcan, mailbox_mask);
/* Mailbox is busy, return error and let caller handle it */

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Documentation / 文档: Missing documentation for new mailbox abort behavior / 缺少新邮箱中止行为的文档说明

English: The function documentation (lines 489-503) describes that this function "is called by _can_int_tx after a hardware mailbox has already been acquired." However, the new implementation at line 543 now calls HAL_CAN_AbortTxRequest when the mailbox is busy, which changes the function's behavior significantly. The documentation should be updated to reflect this new abort-on-busy behavior and explain when and why transmission requests are aborted.

中文：函数文档（第 489-503 行）描述此函数"在硬件邮箱已被获取后由 _can_int_tx 调用"。然而，第 543 行的新实现在邮箱繁忙时调用 HAL_CAN_AbortTxRequest，这显著改变了函数的行为。文档应更新以反映这种新的繁忙时中止行为，并解释何时以及为何中止传输请求。

Copilot uses AI. Check for mistakes.
return -RT_ERROR;
}

if (RT_CAN_STDID == pmsg->ide)
Expand Down