fix(drv_can): resolve issues with reopening after close and TX mailbox blockage #10898
Conversation
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
📌 Code Review Assignment🏷️ Tag: bsp_stm32Reviewers: @Liang1795 @hamburger-os @wdfk-prog Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-11-05 16:31 CST)
📝 Review Instructions
|
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the STM32 CAN driver to improve the CAN start/stop control flow and refactor the mailbox handling logic in the message send function.
Key Changes:
- Changed CAN stop behavior from
HAL_CAN_StoptoHAL_CAN_DeInitand added reconfiguration on CAN start - Refactored mailbox availability checking to use variables instead of duplicated switch cases
- Added automatic transmission abort when mailbox is busy instead of immediately returning error
|
|
||
| if (HAL_IS_BIT_SET(hcan->Instance->TSR, tme_flag) != SET) | ||
| { | ||
| HAL_CAN_AbortTxRequest(hcan, mailbox_mask); |
There was a problem hiding this comment.
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 应用。
| HAL_CAN_AbortTxRequest(hcan, mailbox_mask); | |
| /* Mailbox is busy, return error and let caller handle it */ |
|
|
||
| if (HAL_IS_BIT_SET(hcan->Instance->TSR, tme_flag) != SET) | ||
| { | ||
| HAL_CAN_AbortTxRequest(hcan, mailbox_mask); |
There was a problem hiding this comment.
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，这显著改变了函数的行为。文档应更新以反映这种新的繁忙时中止行为，并解释何时以及为何中止传输请求。
| _can_config(&drv_can->device, &drv_can->device.config); | ||
| HAL_CAN_Start(&drv_can->CanHandle); |
There was a problem hiding this comment.
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;
}| _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; | |
| } |
| if (argval == 0) | ||
| { | ||
| HAL_CAN_Stop(&drv_can->CanHandle); | ||
| HAL_CAN_DeInit(&drv_can->CanHandle); |
There was a problem hiding this comment.
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 的返回值。虽然不如初始化错误那么关键，但反初始化失败可能表明资源清理问题。考虑检查并记录或返回错误状态，以获得更好的错误可见性和调试能力。
| HAL_CAN_DeInit(&drv_can->CanHandle); | |
| if (HAL_CAN_DeInit(&drv_can->CanHandle) != HAL_OK) | |
| { | |
| LOG_E("CAN deinitialization failed"); | |
| return -RT_ERROR; | |
| } |
| if (argval == 0) | ||
| { | ||
| HAL_CAN_Stop(&drv_can->CanHandle); | ||
| HAL_CAN_DeInit(&drv_can->CanHandle); |
There was a problem hiding this comment.
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，并确保初始化/反初始化成对出现以提高代码清晰度。
| HAL_CAN_DeInit(&drv_can->CanHandle); | |
| HAL_CAN_Stop(&drv_can->CanHandle); |
1 participant
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
当前的 stm32 CAN 驱动 (
drv_can.c) 存在两个潜在问题：rt_device_control接口，通过RT_CAN_CMD_START命令先关闭 CAN 设备再重新打开时，CAN 设备可能无法正常重新启动和工作。你的解决方案是什么 (what is your solution)
HAL_CAN_Stop()修改为HAL_CAN_DeInit()。DeInit会彻底复位 CAN 外设状态。作为对应，在重新打开设备时，在调用HAL_CAN_Start()之前，先调用_can_config()函数，使用设备结构体中保存的配置来重新初始化 CAN 外设。_can_sendmsg中，当检查到指定的发送邮箱非空（即TME位为0）时，不再直接返回错误，而是先调用HAL_CAN_AbortTxRequest()函数来主动取消并清除该邮箱中挂起的发送请求。这样可以释放被占用的邮箱，使得新的发送请求可以被正常处理，提高了驱动的健壮性。请提供验证的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