Files
ASER/CODE_REVIEW.md
2026-03-31 23:30:33 +08:00

398 lines
18 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# ARES 项目代码审查报告
> **审查日期**: 2025 年
> **审查范围**: `/App` 目录下全部业务代码(排除 `VL53L0X_API/core` ST 官方库)
> **审查基准**: HANDOFF.md 中的设计描述与已修复 BUG 列表
> **审查重点**: 残留 BUG、潜在运行时风险、代码质量问题
---
## 审查总结
| 严重等级 | 数量 | 说明 |
|---------|------|------|
| 🔴 严重 (可能导致功能错误/崩溃) | 4 | 运行时会导致错误行为 |
| 🟠 中等 (潜在风险/隐患) | 6 | 特定条件下可能触发问题 |
| 🟡 低 (代码质量/可维护性) | 7 | 不影响功能但应改善 |
**总体评价**: 项目架构设计清晰HANDOFF.md 中列出的 9 个严重 BUG 和 7 个质量问题均已正确修复。但仍存在若干残留问题,以下逐一分析。
---
## 🔴 严重问题
### S-1: VL53L0X 卡尔曼滤波 Q/R 参数硬编码,未使用 `robot_params.h` 配置
**文件**: `VL53L0X_API/platform/vl53_board.c:84`
```c
/* 初始化卡尔曼滤波器:默认 Q=1.0, R=9.5 */ // ← 注释过期
vl53_kalman_init(&board->kf[i], 10.0f, 14.1f); // ← 硬编码值
```
`robot_params.h` 中定义了 `PARAM_VL53_KALMAN_Q = 10.0f``PARAM_VL53_KALMAN_R = 14.1f`,但 `vl53_board.c` 没有 `#include "robot_params.h"`,而是直接写死了 `10.0f``14.1f`。虽然当前值恰好与参数一致,但**修改 `robot_params.h` 中的 `PARAM_VL53_KALMAN_Q/R` 不会生效**,与 BUG-2 (EKF 参数硬编码) 属于同类问题。
**修复建议**:
```c
#include "robot_params.h"
// ...
vl53_kalman_init(&board->kf[i], PARAM_VL53_KALMAN_Q, PARAM_VL53_KALMAN_R);
```
---
### S-2: `LASER_SIMPLE_GetSnapshot()` 返回指针无线程安全保护,存在数据撕裂风险
**文件**: `laser/laser_manager.c:277-279`
```c
const laser_simple_snapshot_t *LASER_SIMPLE_GetSnapshot(void) {
return &g_snapshot; // ← 返回原始指针,无拷贝、无临界区保护
}
```
消费者 `AppTasks_RunLaserTestTask_Impl()` 调用此函数获取指针后直接传给 `Blackboard_UpdateLaser(snap)`。由于 `LaserTask`内部10ms任务`taskENTER_CRITICAL()` 中**逐通道写入** `g_snapshot.ch[i]`,而消费者拿到的是裸指针,读取可能发生在写入的间隙——比如前两个通道已更新、后两个通道还是旧值——构成**部分撕裂**。
虽然 `Blackboard_UpdateLaser` 内部有临界区保护,但问题出在**读 g_snapshot 的时候没有保护**。当前架构中 `laserTestTask` 以 50ms 周期读取、`LaserTask` 以 10ms 周期写入,两者优先级相同,可能发生抢占。
**影响等级**: 中高。实际撕裂概率取决于临界区外的读取窗口大小。由于 `laser_simple_snapshot_t` 只包含 4 个 `laser_simple_data_t`,单次 `memcpy` 耗时极短,实际风险较低。但从防御性编程角度应修复。
**修复建议**:
```c
void LASER_SIMPLE_GetSnapshotCopy(laser_simple_snapshot_t *out) {
taskENTER_CRITICAL();
*out = g_snapshot;
taskEXIT_CRITICAL();
}
```
---
### S-3: `snc_parse_odom_delta()` 在 ISR 中写 `odom_accum` 无临界区保护
**文件**: `Can/snc_can_app.c:148-173` (在 `SNC_CAN_RxFifo0Callback` 即 CAN ISR 中被调用)
```c
static void snc_parse_odom_delta(const uint8_t *d)
{
// ... 直接写 g_snc_can_app.odom_accum 的各个字段 ...
SNC_OdomDeltaAccum_t *acc = &g_snc_can_app.odom_accum;
acc->fl_accum += (int32_t)snc_read_i16_le(d[0], d[1]);
// ...
if (acc->frame_count < 255U) {
acc->frame_count++;
}
}
```
而消费侧 `SNC_CAN_ConsumeOdomDelta()` 使用 `taskENTER_CRITICAL()` 保护读取和清零。问题是:**`taskENTER_CRITICAL()` 在 Cortex-M 上通过 BASEPRI 屏蔽中断,只屏蔽优先级不高于 `configMAX_SYSCALL_INTERRUPT_PRIORITY` 的中断**。如果 CAN FIFO0 中断的优先级高于此阈值(即数值更小),那么 `snc_parse_odom_delta()` 可能在消费者持有临界区期间执行,导致**竞态条件**消费者清零后ISR 立刻写入新数据,然后消费者返回的 `snapshot``frame_count=0``accum` 值已非零。
**影响等级**: 取决于 CAN 中断优先级配置。如果 CAN 中断优先级在 FreeRTOS 管理范围内(优先级数值 ≥ `configMAX_SYSCALL_INTERRUPT_PRIORITY`),则 `taskENTER_CRITICAL()` 可以正确屏蔽它,问题不存在。**需要检查 CubeMX 中 FDCAN1 中断优先级配置**。
**修复建议**: 确认 FDCAN1 中断优先级 ≥ `configMAX_SYSCALL_INTERRUPT_PRIORITY`。或在 `snc_parse_odom_delta()` 中也使用 `taskENTER_CRITICAL()`(但 ISR 中应使用 `taskENTER_CRITICAL_FROM_ISR()`)。
---
### S-4: `corridor_msgs.h` 使用 `#pragma pack(push, 1)` 导致 EKF 矩阵和浮点运算性能损失及潜在对齐异常
**文件**: `preproc/corridor_msgs.h:8,61`
```c
#pragma pack(push, 1)
// ...
typedef struct {
float P[3][3]; // 36 字节的浮点矩阵,被强制 1 字节对齐
float innovation[3]; // 12 字节
float mahalanobis_d2;
// ...
} CorridorState_t;
// ...
#pragma pack(pop)
```
`CorridorState_t` 包含大量 `float``float[3][3]` 数组,被 `#pragma pack(push, 1)` 强制 1 字节对齐。在 Cortex-M7 上:
1. **性能损失**: 未对齐的 float 访问会触发硬件 unaligned access 处理,速度比对齐访问慢数倍。这个结构体在 navTask 的 20ms 循环中被高频读写,影响实时性能。
2. **FPU 指令风险**: 某些 FPU 指令(如 `VLDM`/`VSTM`**要求 4 字节对齐**,不对齐会触发 UsageFault。虽然 GCC 通常会生成安全的加载指令,但编译器优化可能引入向量化或批量加载指令。
`CorridorObs_t``RawCmd_t` 也受影响,但内部结构更简单,风险较低。
**修复建议**: `#pragma pack(push, 1)` 仅用于需要匹配物理总线帧格式的结构体(如 `chassis_can_msg.h` 中的 CAN 帧,那是正确的用法)。`corridor_msgs.h` 中的结构体是内存中的数据传递,**不需要 pack(1)**,应删除。
---
## 🟠 中等问题
### M-1: IMU 帧解析状态机缺少帧类型 0x55 之后的 header byte 校验
**文件**: `IMU/hwt101.c:64-68`
```c
if (s_frame_idx == 0 && byte != 0x55) continue;
s_frame[s_frame_idx++] = byte;
if (s_frame_idx >= 11) {
```
当前状态机只检查首字节 `0x55`,然后无条件接收后续 10 字节。如果数据流中出现非帧头的 `0x55`(比如校验和恰好是 `0x55`),会导致帧错位。虽然最后有校验和检查可以过滤错误帧,但会**浪费 10 个字节的解析窗口**,在高频数据流中可能导致短暂的数据更新延迟。
**修复建议**: 在 `s_frame_idx == 1` 时检查第二字节是否为有效帧类型 (`0x51`/`0x52`/`0x53`),不匹配则 `s_frame_idx = 0` 重新寻帧。
---
### M-2: `nav_script.c` 转向方向始终为正,不支持反向 (顺时针) 180° 转弯
**文件**: `nav/nav_script.c:202,229`
```c
float target_delta = s_cfg.turn_target_angle; // 默认 PI (180度)
// ...
float turn_dir = (target_delta > 0.0f) ? 1.0f : -1.0f;
```
`turn_target_angle``app_tasks.c` 中始终被初始化为 `3.14159265f`(正值),所以 `turn_dir` 永远为 `1.0f`(逆时针)。如果因场地布局需要顺时针转弯,当前代码无法支持。更关键的是,**两次 180° 转弯方向相同**——第一次到端逆时针转,第二次到端又逆时针转,结果机器人回到出发方向。这可能是设计意图(转完 180° 后继续正向走),但如果走廊空间不对称,固定转向方向可能导致转弯时撞墙。
**修复建议**: 考虑根据当前方向或走廊几何自动选择转向方向,或至少在 `NavScriptConfig_t` 中加一个 `turn_direction` 参数。
---
### M-3: `Odom_GetSpeed()` 在临界区外读取 `s_odom.vx` 和 `s_odom.wz`
**文件**: `Contract/robot_odom.c:122-124`
```c
void Odom_GetSpeed(float *out_vx, float *out_wz, OdomStatus_t *out_status)
{
if (out_vx != NULL) *out_vx = s_odom.vx; // ← 临界区外
if (out_wz != NULL) *out_wz = s_odom.wz; // ← 临界区外
if (out_status != NULL) {
lock_odom(); // ← 只有 status 在临界区内
// ...
unlock_odom();
}
}
```
`s_odom.vx``s_odom.wz``float` 类型,在 Cortex-M7 上 32 位对齐的 float 读写是原子的,所以单独读取不会撕裂。但 `vx``wz` 之间不是原子的——可能读到旧的 `vx` 和新的 `wz`。不过此函数当前未被任何代码调用(速度通过 `Blackboard_UpdateOdom` 传递),所以实际影响为零。
**修复建议**: 将 `vx/wz` 的读取也放入 `lock_odom()` 中,或标注此函数为 `@deprecated`
---
### M-4: `SNC_CAN_SendHeartbeat()` 使用零长度数组
**文件**: `Can/snc_can_app.c:194`
```c
HAL_StatusTypeDef SNC_CAN_SendHeartbeat(void)
{
uint8_t data[0]; // ← 零长度数组C11 标准未定义行为
HAL_StatusTypeDef ret = snc_fdcan_add_tx_std(SNC_CAN_ID_HEARTBEAT, data, 0U);
```
零长度数组 `uint8_t data[0]` 在 C11 标准中是未定义行为C99 的 flexible array member 语法是 `[]` 且只能在结构体末尾)。虽然 GCC 作为扩展支持它,且 `snc_fdcan_add_tx_std` 传入 `dlc_bytes=0` 不会实际读取 `data`,但这仍属于未定义行为。
**修复建议**: 改为 `uint8_t data[1] = {0};` 或直接传 `NULL`(需确认 HAL 是否接受 NULL data
---
### M-5: 安全状态机 (SegFsm) 在脚本覆盖模式下仍然生效,可能干扰转弯等特殊动作
**文件**: `app_tasks.c` navTask 流水线 Step 5-6
```c
/* Step 5: 控制律 */
if (script_out.use_override) {
raw_cmd.v = script_out.override_v; // 脚本指定速度
raw_cmd.w = script_out.override_w;
} else if (script_out.request_corridor) {
CorridorCtrl_Compute(..., &raw_cmd);
}
/* Step 6: 段状态机 -> 安全仲裁 */
SegFsm_Update(&raw_cmd, &obs, &corridor_state, &fsm_out);
```
当脚本处于 `TURN_AT_END` 阶段时,`use_override = true`,输出 `v=0, w=turn_omega`(原地转向)。但此时 EKF 因侧墙观测丢失,`conf` 可能降到 `< 0.1`,触发 `SegFsm` 进入 `E-STOP`**强制将转向角速度归零**,导致机器人无法完成转弯。
虽然 `E-STOP` 有自动恢复机制(`conf >= 0.5` 时恢复),但在转弯过程中侧墙持续不可见,`conf` 不会恢复,造成**死锁**:转不了弯 → 永远看不到墙 → 永远 E-STOP。
**影响等级**: 实车中度风险。取决于转弯时 EKF 的 IMU 航向观测是否能维持 `conf >= 0.1`。如果 IMU 航向观测能阻止 `conf` 跌破阈值,则不会触发。但这是一个脆弱的依赖。
**修复建议**: 在 `SegFsm_Update()` 中增加一个 bypass 标志,当脚本处于 `use_override` 模式时跳过 E-STOP 判定;或仅在 `request_corridor` 为 true 时检查置信度。
---
### M-6: `process_complementary_laser()` 中 ATK 距离值无上限校验
**文件**: `preproc/corridor_preproc.c:30-73`
```c
static bool process_complementary_laser(const SensorItem_t *stp, const SensorItem_t *atk, float *out_m)
{
// ...
float atk_m = atk->value / 1000.0f;
// ATK 没有像 VL53 那样的量程校验 (0.02~2.0m)
// 如果 ATK 吐出异常大值 (如 65535mm),会被当作有效数据
```
`process_side_laser()` 对 VL53L0X 数据做了 `[0.02m, 2.0m]` 范围校验,但 `process_complementary_laser()` 对 ATK 和 STP 数据**没有上限校验**。如果 ATK 传感器吐出异常大值(如掉线前最后一帧的乱码),这个值会被当作有效距离传递给安全状态机,可能导致**该停车时不停车**。
**修复建议**: 添加 ATK/STP 的量程校验,如 `atk_m > 0.0f && atk_m <= 8.0f` (ATK 标称量程 4m留双倍余量)。
---
## 🟡 低优先级 / 代码质量问题
### L-1: `PARAM_IMU_YAW_OFFSET` 声明但未使用
**文件**: `robot_params.h:100`
```c
#define PARAM_IMU_YAW_OFFSET 0.0f // 声明了但代码中未使用
```
HANDOFF.md CAL-4 已标注此问题。如果 IMU 安装有固定偏角,此参数应在 `HWT101_Process()``corridor_filter.c` 中使用。当前为死代码。
---
### L-2: `corridor_filter.c` 中 `s_imu_yaw_ref_set` 在 Reset/Init 时未重置
**文件**: `est/corridor_filter.c:23-24`
```c
static float s_imu_yaw_ref_rad = 0.0f;
static bool s_imu_yaw_ref_set = false;
```
`CorridorFilter_Init()` 不会重置 `s_imu_yaw_ref_set`。如果比赛中途调用 `NavScript_Reset()``CorridorFilter_Init()` 重新初始化,旧的 `s_imu_yaw_ref_rad` 会保留,导致 IMU 航向观测使用过时的参考值。
**修复建议**: 在 `CorridorFilter_Init()` 末尾添加:
```c
s_imu_yaw_ref_rad = 0.0f;
s_imu_yaw_ref_set = false;
```
---
### L-3: `retarget.c` 中 `_write()` 的忙等循环可能阻塞调用任务
**文件**: `retarget.c:44-64`
```c
int _write(int file, char *ptr, int len)
{
while (1) {
result = CDC_Transmit_FS((uint8_t *)ptr, (uint16_t)len);
if (result == USBD_OK) return len;
if ((HAL_GetTick() - start_tick) > 20U) return 0;
if (osKernelGetState() == osKernelRunning) osDelay(1);
}
}
```
如果 USB CDC 端口忙碌Host 未连接或 buffer 满),`printf` 会阻塞当前任务最多 20ms。对于 `navTask`20ms 周期)和 `canTxTask`20ms 周期),一次 `printf` 阻塞就可能导致**整个控制周期跳过**。当前 `App_PrintStatus()` 已被注释掉,但如果未来取消注释或在其他任务中添加 `printf`,可能造成问题。
**修复建议**: 在实时任务中避免使用 `printf`,或将 `_write` 改为非阻塞(丢弃模式)。
---
### L-4: 多个模块重复定义 `clampf()` 静态内联函数
**文件**:
- `est/corridor_ekf.c:42`
- `nav/corridor_ctrl.c:12`
- `nav/nav_script.c:34`
- `nav/segment_fsm.c:12`
四个文件各自定义了相同的 `static inline float clampf()`。虽然由于 `static` 链接属性不会导致链接错误,但违反 DRY 原则。
**修复建议**: 提取到公共头文件 `robot_params.h` 或新建 `utils.h`
---
### L-5: `CorridorObs_t.valid_mask` 类型为 `uint8_t` 但掩码使用 bit 0-5 共 6 位
**文件**: `preproc/corridor_msgs.h:31``corridor_preproc.h:10-15`
当前 6 个掩码位刚好在 `uint8_t` 范围内(最大 bit 5 = 0x20但余量很小。如果未来添加更多传感器如第二组 VL53很容易溢出。不紧急但值得注意。
---
### L-6: `vl53_board.c:84` 注释与实际值不一致
```c
/* 初始化卡尔曼滤波器:默认 Q=1.0, R=9.5 */ // ← 注释说 Q=1.0, R=9.5
vl53_kalman_init(&board->kf[i], 10.0f, 14.1f); // ← 实际值 Q=10.0, R=14.1
```
注释是旧版残留,与当前代码不符。
---
### L-7: `nav_script.c:285` 使用 `exit_start_s == 0.0f` 判断是否已触发,但 `s` 初始值就是 0
```c
if (s_internal.exit_start_s == 0.0f) {
s_internal.exit_start_s = state->s;
}
```
如果恰好在 `state->s ≈ 0.0` 时进入 EXIT 阶段(理论上不会,因为已经往返走过垄沟),`exit_start_s` 会被设为 0然后下次循环 `== 0.0f` 再次为 true重复赋值但不会出错`state->s` 每次都差不多)。实际上因为 `memset(&s_internal, 0, ...)` 已将其初始化为 0.0,如果 EXIT 阶段被跳过再重入,可能出现意外行为。
**修复建议**: 使用 `bool exit_triggered` 标志代替浮点数零值判断。
---
## 架构设计审查
### 优点
1. **分层清晰**: 传感器驱动 → 黑板 → 预处理 → EKF → 控制 → 安全 → 指令槽,每层职责明确。
2. **线程安全设计合理**: 黑板的 `taskENTER_CRITICAL` + snapshot 模式有效防止了数据撕裂。
3. **传感器互补融合策略** (`process_complementary_laser`): STP+ATK 的互补逻辑考虑了盲区、单体故障、保守防撞等多种场景,设计周到。
4. **EKF 实现质量高**: 鲁棒 χ² 检验、分级观测更新1/2/3 DOF、协方差保护等机制完备。BUG-7卡尔曼增益矩阵乘法的修复正确。
5. **参数集中管理**: `robot_params.h` 作为唯一调参入口,大部分参数已正确引用(除 S-1 遗漏)。
6. **里程计累加器设计** (BUG-8 修复): ISR 累加 + 原子取走清零的设计有效解决了漏积分/重复积分问题。
### 可改进方向
1. **缺少 Watchdog**: 系统没有硬件看门狗 (IWDG/WWDG)。如果任何任务死锁或 HardFaultMCU 将永久挂起而不会自动重启。建议在空闲任务中喂看门狗。
2. **缺少运行时错误日志**: 当前的错误处理主要是"静默忽略"或"返回零值"。建议增加一个轻量级错误计数器或环形日志 buffer方便赛后分析。
3. **EKF 和安全状态机没有联动**: 如 M-5 所述,脚本覆盖模式下安全状态机可能干扰正常流程。建议在 `SegFsmOutput_t` 中增加 bypass 机制。
4. **`laserTestTask``osDelay(50)` 不精确**: 使用 `osDelay` 而非 `osDelayUntil`,任务执行时间会累加到周期中,导致实际周期 > 50ms。对于激光数据更新频率有影响。
---
## 已修复 BUG 验证
| HANDOFF 编号 | 修复内容 | 验证结果 |
|-------------|---------|---------|
| BUG-1 | IMU wz deg→rad 转换 | ✅ `app_tasks.c:305` 使用 `PARAM_DEG2RAD()` |
| BUG-2 | EKF Q/R/P0 从 params 读取 | ✅ `corridor_filter.c:50-66` 使用 `PARAM_EKF_*` |
| BUG-3 | `SegFsm_Start()` 调用 | ✅ `app_tasks.c:382` |
| BUG-4 | BACKWARD 段使用 d_front | ✅ `nav_script.c:254` |
| BUG-5 | 超时使用配置参数 | ✅ `nav_script.c:157` |
| BUG-6 | EXIT 速度独立参数 | ✅ `nav_script.c:273` 使用 `s_cfg.exit_v` |
| BUG-7 | 完整矩阵乘法 K=PHT*S_inv | ✅ `corridor_ekf.c:576-607` 两步乘法 |
| BUG-8 | 里程计累加器模式 | ✅ `snc_can_app.c` ISR 累加 + `ConsumeOdomDelta` 原子取走 |
| BUG-9 | IMU yaw unwrap + 连续角度 | ✅ `hwt101.c` unwrap 逻辑 + `nav_script.c` 使用 IMU yaw |
| Q-1~Q-7 | 各项代码质量修复 | ✅ 全部验证通过 |
---
## 修复优先级建议
| 优先级 | 编号 | 预估工时 |
|--------|------|---------|
| 立即修复 | S-4 (pack 对齐) | 5 分钟 |
| 立即修复 | S-1 (VL53 KF 参数) | 5 分钟 |
| 赛前修复 | M-5 (安全 FSM bypass) | 30 分钟 |
| 赛前修复 | M-6 (ATK 量程校验) | 10 分钟 |
| 赛前修复 | S-2 (快照线程安全) | 15 分钟 |
| 赛前修复 | L-2 (IMU ref 重置) | 5 分钟 |
| 确认配置 | S-3 (CAN 中断优先级) | 10 分钟 |
| 建议改进 | 其余所有 | 按需 |
---
> **审查结论**: 项目整体质量良好,架构设计规范,历史 BUG 修复彻底。上述 S-4 和 S-1 建议在下次烧录前立即修复M-5 (安全状态机与脚本冲突) 是实车测试中最可能暴露的问题,建议优先验证。