3.27_433：添加UART2调试打印、IO监控、指令解析和继电器控制模块。
能够接收UART2指令控制继电器开关，或向UART2发送四路IO输入状态，并使用轮询方式检测IO状态进行及时反馈。
This commit is contained in:
490
code_review_report.md
Normal file
490
code_review_report.md
Normal file
@ -0,0 +1,490 @@
|
||||
# STM32F103 应用层代码深度审查报告
|
||||
|
||||
## 1. 审查概述
|
||||
|
||||
**审查范围**：
|
||||
- `uart2_print.c/.h` - UART2调试打印模块（环形缓冲区+中断发送）
|
||||
- `io_monitor.c/.h` - 四路DI状态监控与去抖模块
|
||||
- `cmd_parser.c/.h` - ASCII指令解析状态机
|
||||
- `relay_control.c/.h` - 继电器控制模块
|
||||
|
||||
**审查方法**：静态代码分析 + 嵌入式系统安全规范检查
|
||||
|
||||
**主要发现**：
|
||||
- 🔴 **高危问题**：4个（中断安全、缓冲区竞态、状态机边界）
|
||||
- 🟡 **中危问题**：5个（参数校验、逻辑缺陷）
|
||||
- 🟢 **低危/建议**：4个（代码健壮性）
|
||||
|
||||
---
|
||||
|
||||
## 2. 详细问题清单
|
||||
|
||||
### 2.1 uart2_print 模块（最严重）
|
||||
|
||||
#### 🔴 **[高危-1] UART2_Print_Send 中 is_sending 检查存在竞态条件**
|
||||
|
||||
**代码位置**：`uart2_print.c` 第73-74行
|
||||
|
||||
```c
|
||||
void UART2_Print_Send(const uint8_t *data, uint16_t len)
|
||||
{
|
||||
// ... 写入缓冲区（已保护）...
|
||||
__enable_irq(); // ← 第71行：保护区结束
|
||||
|
||||
if (written > 0 && !tx_ring.is_sending) { // ← 第73行：非原子检查！
|
||||
UART2_Print_Task(); // ← 第74行
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**风险分析**：
|
||||
- **触发条件**：中断正好在 `__enable_irq()` 之后、`UART2_Print_Task()` 调用之前发生
|
||||
- **触发场景**：
|
||||
1. 线程A调用 `UART2_Print_Send`，数据已写入缓冲区
|
||||
2. `is_sending = false`，进入 if 块
|
||||
3. 中断触发，`HAL_UART_TxCpltCallback` 执行，设置 `is_sending = true`，调用 `UART2_Print_Task` 发送一字节
|
||||
4. `UART2_Print_Task` 再次设置 `is_sending = true`
|
||||
5. 线程A继续执行，调用 `UART2_Print_Task()`（但此时 `is_sending = true`，直接返回）
|
||||
6. **结果**：新写入的字节不会被发送，直到后续再次有数据写入
|
||||
|
||||
**后果**：调试信息丢失，尤其在高频日志输出时部分数据"静默消失"
|
||||
|
||||
---
|
||||
|
||||
#### 🔴 **[高危-2] UART2_Print_Task 完全缺乏中断保护**
|
||||
|
||||
**代码位置**：`uart2_print.c` 第107-122行
|
||||
|
||||
```c
|
||||
void UART2_Print_Task(void)
|
||||
{
|
||||
if (tx_ring.is_sending) { // ← 非原子读取
|
||||
return;
|
||||
}
|
||||
|
||||
if (tx_ring.tail == tx_ring.head) { // ← 非原子读取
|
||||
return;
|
||||
}
|
||||
|
||||
uint8_t byte = tx_ring.buffer[tx_ring.tail]; // ← 读取数据
|
||||
tx_ring.tail = (tx_ring.tail + 1) % UART2_TX_BUFFER_SIZE; // ← 更新索引（无保护）
|
||||
tx_ring.is_sending = true; // ← 非原子写入
|
||||
|
||||
HAL_UART_Transmit_IT(&huart2, &byte, 1);
|
||||
}
|
||||
```
|
||||
|
||||
**风险分析**：
|
||||
- **触发条件**：`UART2_Print_Task` 在主循环中被调用，同时 UART TX 中断发生
|
||||
- **触发场景**：
|
||||
1. 主循环调用 `UART2_Print_Task`，通过所有检查
|
||||
2. 读取 `byte = buffer[tail]`，`tail` 已更新，但 `is_sending` 尚未置位
|
||||
3. UART TX 完成中断触发，`HAL_UART_TxCpltCallback` 执行
|
||||
4. 回调检查 `tail != head`（已变化），调用 `UART2_Print_Task`
|
||||
5. 同一字节被重复发送
|
||||
|
||||
**后果**：数据重复发送，接收方收到重复字符；严重时可能导致协议解析错误
|
||||
|
||||
---
|
||||
|
||||
#### 🔴 **[高危-3] 环形缓冲区容量计算错误 - 差一错误 (Off-by-one)**
|
||||
|
||||
**代码位置**：
|
||||
- `uart2_print.c` 第142-148行 - `Available` 函数
|
||||
- `uart2_print.h` 第26行 - 缓冲区大小定义
|
||||
|
||||
```c
|
||||
uint16_t UART2_Print_Available(void)
|
||||
{
|
||||
uint16_t used;
|
||||
|
||||
if (tx_ring.head >= tx_ring.tail) {
|
||||
used = tx_ring.head - tx_ring.tail;
|
||||
} else {
|
||||
used = UART2_TX_BUFFER_SIZE - tx_ring.tail + tx_ring.head;
|
||||
}
|
||||
|
||||
return UART2_TX_BUFFER_SIZE - used - 1; // ← 这里减1了！
|
||||
}
|
||||
```
|
||||
|
||||
**问题**：当 `head = tail` 时，`used = 0`，返回 `256 - 0 - 1 = 255`。但环形缓冲区的空状态和满状态都是 `head == tail`，无法区分！
|
||||
|
||||
**正确做法**：存储一个 `count` 变量，或使用 `head` 和 `tail` 之间的固定一个空位来区分满/空状态。
|
||||
|
||||
---
|
||||
|
||||
#### 🟡 **[中危-4] UART2_Print_Printf 返回值处理缺陷**
|
||||
|
||||
**代码位置**：`uart2_print.c` 第96-104行
|
||||
|
||||
```c
|
||||
int len = vsnprintf(buffer, sizeof(buffer), fmt, args);
|
||||
va_end(args);
|
||||
|
||||
if (len > 0) { // ← 问题在这里
|
||||
if (len >= (int)sizeof(buffer)) {
|
||||
len = sizeof(buffer) - 1;
|
||||
}
|
||||
UART2_Print_Send((const uint8_t *)buffer, len);
|
||||
}
|
||||
```
|
||||
|
||||
**问题**：`vsnprintf` 返回值 `> 0` 表示需要写入的字符数（不包括 null）。但如果格式化结果为空字符串（返回0），则不会发送。虽然这通常不是问题，但逻辑上应使用 `len >= 0` 或 `len != -1`。
|
||||
|
||||
**风险**：低 - 基本不会触发，但不够健壮
|
||||
|
||||
---
|
||||
|
||||
### 2.2 io_monitor 模块
|
||||
|
||||
#### 🟡 **[中危-5] 去抖初始化可能遗漏第一帧变化检测**
|
||||
|
||||
**代码位置**：`io_monitor.c` 第74-89行
|
||||
|
||||
```c
|
||||
void IO_Monitor_Init(void)
|
||||
{
|
||||
for (int i = 0; i < IO_CHANNEL_COUNT; i++) {
|
||||
io_channel_t *ch = &di_channels[i];
|
||||
|
||||
ch->current_state = HAL_GPIO_ReadPin(ch->port, ch->pin) ? 1 : 0;
|
||||
ch->last_raw_state = ch->current_state;
|
||||
ch->debounce_counter = IO_DEBOUNCE_COUNT; // ← 初始化为3
|
||||
ch->change_count = 0;
|
||||
}
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**风险分析**：
|
||||
- `debounce_counter = IO_DEBOUNCE_COUNT (3)`
|
||||
- 在第一次 `IO_Monitor_Task` 调用时，如果 `raw_state != last_raw_state`
|
||||
- `debounce_counter` 被设为 0
|
||||
- 需要再检测到连续 3 次相同状态才会确认变化
|
||||
- **问题**：初始化时如果IO状态刚好处于变化沿附近，可能导致第一次变化事件被延迟最多 30ms（3×10ms）才上报
|
||||
|
||||
**风险**：低 - 仅影响初始化后第一次变化检测的响应速度
|
||||
|
||||
---
|
||||
|
||||
#### 🟢 **[建议-1] send_di_event 可被中断安全调用但存在嵌套风险**
|
||||
|
||||
**代码位置**：`io_monitor.c` 第58-72行
|
||||
|
||||
`send_di_event` 调用 `UART2_Print_String`，而 `UART2_Print_String` 调用 `UART2_Print_Send`，后者使用 `__disable_irq()`。
|
||||
|
||||
**潜在问题**：如果 `IO_Monitor_Task` 在中断中调用，而此时 UART 发送中断发生，可能导致死锁（虽然STM32F103不太可能发生，因为UART中断优先级通常较高）。
|
||||
|
||||
---
|
||||
|
||||
### 2.3 cmd_parser 模块
|
||||
|
||||
#### 🔴 **[高危-6] 状态机边界条件处理不完善 - 逗号/星号丢失**
|
||||
|
||||
**代码位置**：`cmd_parser.c` 第231-272行
|
||||
|
||||
```c
|
||||
case PARSE_PARAM1:
|
||||
if (byte == ',') {
|
||||
ctx.frame.param1[ctx.field_index] = '\0';
|
||||
ctx.state = PARSE_PARAM2;
|
||||
ctx.field_index = 0;
|
||||
} else if (byte == '*') {
|
||||
// ... 类似处理 ...
|
||||
} else if (is_valid_param_char(byte)) {
|
||||
if (ctx.field_index < PARAM_MAX_LEN - 1) { // ← 检查的是 field_index
|
||||
ctx.frame.param1[ctx.field_index++] = byte;
|
||||
// ...
|
||||
} else {
|
||||
ctx.error_count++;
|
||||
reset_parser();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**问题分析**：
|
||||
- `PARAM_MAX_LEN = 32`，数组大小确实是 32 字节
|
||||
- 但检查条件是 `ctx.field_index < PARAM_MAX_LEN - 1` (即 < 31)
|
||||
- **当 `field_index = 31` 时**，下一个字符如果是逗号或星号：
|
||||
- 不是有效参数字符，进入 else 分支
|
||||
- `error_count++` 并重置解析器
|
||||
- **触发条件**：发送格式为 `$DI,ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789*XX\r\n`（31个字符的参数）
|
||||
|
||||
**风险**：长度接近31的参数帧会被错误拒绝，虽然这种长参数在实际应用中罕见。
|
||||
|
||||
---
|
||||
|
||||
#### 🟡 **[中危-7] 超时检测逻辑存在边界情况**
|
||||
|
||||
**代码位置**：`cmd_parser.c` 第189-197行
|
||||
|
||||
```c
|
||||
void CmdParser_FeedByte(uint8_t byte, uint32_t current_tick)
|
||||
{
|
||||
if (ctx.state != PARSE_IDLE) {
|
||||
if (current_tick - ctx.last_rx_tick > PARSE_TIMEOUT_MS) { // ← 严格大于
|
||||
ctx.error_count++;
|
||||
DEBUG_LOG("Timeout, reset parser");
|
||||
reset_parser();
|
||||
return;
|
||||
}
|
||||
}
|
||||
ctx.last_rx_tick = current_tick; // ← 每个字节都更新
|
||||
```
|
||||
|
||||
**问题**：
|
||||
- 如果正好在 `timeout_ms + 1` 时刻收到字节，减去 `last_rx_tick` 等于 `timeout_ms + 1`，会被判定为超时
|
||||
- 然后 `reset_parser()` 被调用，`last_rx_tick` 不会被更新为当前时刻
|
||||
- 如果下一个字节在 `timeout_ms` 内到来，超时计数可能不准确
|
||||
|
||||
**风险**：低 - 仅在精确的时序边界触发
|
||||
|
||||
---
|
||||
|
||||
#### 🟡 **[中危-8] DI指令参数校验不完善**
|
||||
|
||||
**代码位置**：`cmd_parser.c` 第145-167行
|
||||
|
||||
```c
|
||||
else if (strcmp(frame->cmd, "DI") == 0) {
|
||||
int channel = atoi(frame->param1); // ← atoi 空字符串返回0
|
||||
|
||||
if (channel == 0) {
|
||||
// 返回所有状态
|
||||
}
|
||||
else if (channel >= 1 && channel <= 4) {
|
||||
// 返回指定通道状态
|
||||
}
|
||||
else {
|
||||
send_response_err("PARAM");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**问题**：
|
||||
- `$DI,*` 会被解析为 `channel = 0`（空字符串传给 atoi）
|
||||
- `$DI,0*` 也是 `channel = 0`
|
||||
- 两种格式语义不同，但处理相同
|
||||
|
||||
**风险**：低 - 功能上可能符合预期，但语义不够清晰
|
||||
|
||||
---
|
||||
|
||||
### 2.4 relay_control 模块
|
||||
|
||||
#### 🟡 **[中危-9] 继电器ID检查与硬件配置不匹配**
|
||||
|
||||
**代码位置**：
|
||||
- `relay_control.c` 第43-69行
|
||||
- `relay_control.h` 第24行
|
||||
|
||||
```c
|
||||
#define RELAY_COUNT 1 // ← 硬件只有一个继电器
|
||||
#define RELAY_MIN_INTERVAL 100 // ← 但允许ID 1-4
|
||||
|
||||
void Relay_SetState(uint8_t relay_id, bool state)
|
||||
{
|
||||
if (relay_id < 1 || relay_id > RELAY_COUNT) { // ← 检查 > 1 会失败
|
||||
DEBUG_LOG("Invalid relay ID: %d", relay_id);
|
||||
return;
|
||||
}
|
||||
```
|
||||
|
||||
**问题**：
|
||||
- `RELAY_COUNT = 1`，但允许的 relay_id 范围是 1-4
|
||||
- 如果调用 `Relay_SetState(2, true)`，会被错误拒绝
|
||||
- 与 `cmd_parser.c` 中允许 1-4 的校验逻辑不一致
|
||||
|
||||
**风险**：如果将来扩展到4个继电器但忘记更新 `RELAY_COUNT`，会导致只有第1个继电器工作
|
||||
|
||||
---
|
||||
|
||||
#### 🟡 **[中危-10] current_state 只支持单个继电器**
|
||||
|
||||
**代码位置**：`relay_control.c` 第29行
|
||||
|
||||
```c
|
||||
static bool current_state = false; // ← 只有一个状态变量
|
||||
```
|
||||
|
||||
**问题**：如果将来扩展到多继电器，`current_state` 无法区分各继电器状态。
|
||||
|
||||
---
|
||||
|
||||
## 3. 关键案例详解
|
||||
|
||||
### 📌 案例一（最具代表性）：UART2 环形缓冲区竞态条件
|
||||
|
||||
**修改前**（uart2_print.c 第50-76行）：
|
||||
|
||||
```c
|
||||
void UART2_Print_Send(const uint8_t *data, uint16_t len)
|
||||
{
|
||||
if (len == 0 || data == NULL) {
|
||||
return;
|
||||
}
|
||||
|
||||
uint16_t written = 0;
|
||||
|
||||
__disable_irq();
|
||||
for (uint16_t i = 0; i < len; i++) {
|
||||
uint16_t next = (tx_ring.head + 1) % UART2_TX_BUFFER_SIZE;
|
||||
|
||||
if (next == tx_ring.tail) {
|
||||
tx_ring.overflow_count++;
|
||||
break;
|
||||
}
|
||||
|
||||
tx_ring.buffer[tx_ring.head] = data[i];
|
||||
tx_ring.head = next;
|
||||
written++;
|
||||
}
|
||||
__enable_irq(); // ← 保护区结束
|
||||
|
||||
if (written > 0 && !tx_ring.is_sending) { // ← 竞态窗口！
|
||||
UART2_Print_Task();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**修改后（推荐方案）**：
|
||||
|
||||
```c
|
||||
void UART2_Print_Send(const uint8_t *data, uint16_t len)
|
||||
{
|
||||
if (len == 0 || data == NULL) {
|
||||
return;
|
||||
}
|
||||
|
||||
uint16_t written = 0;
|
||||
bool needs_kickoff = false;
|
||||
|
||||
__disable_irq();
|
||||
for (uint16_t i = 0; i < len; i++) {
|
||||
uint16_t next = (tx_ring.head + 1) % UART2_TX_BUFFER_SIZE;
|
||||
|
||||
if (next == tx_ring.tail) {
|
||||
tx_ring.overflow_count++;
|
||||
break;
|
||||
}
|
||||
|
||||
tx_ring.buffer[tx_ring.head] = data[i];
|
||||
tx_ring.head = next;
|
||||
written++;
|
||||
}
|
||||
|
||||
if (written > 0 && !tx_ring.is_sending) {
|
||||
tx_ring.is_sending = true; // ← 在保护区内设置，避免竞态
|
||||
needs_kickoff = true;
|
||||
}
|
||||
__enable_irq();
|
||||
|
||||
if (needs_kickoff) {
|
||||
HAL_UART_Transmit_IT(&huart2, &tx_ring.buffer[tx_ring.tail], 1);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**消除的隐患**：
|
||||
- 将 `is_sending` 标志的设置纳入临界区
|
||||
- 避免了中断和主循环之间的状态检查竞态
|
||||
- 数据发送的"启动"操作完全在主循环侧，中断回调只负责推进 `tail`
|
||||
|
||||
---
|
||||
|
||||
### 📌 案例二：UART2_Print_Task 中断保护
|
||||
|
||||
**修改前**（uart2_print.c 第107-122行）：
|
||||
|
||||
```c
|
||||
void UART2_Print_Task(void)
|
||||
{
|
||||
if (tx_ring.is_sending) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (tx_ring.tail == tx_ring.head) {
|
||||
return;
|
||||
}
|
||||
|
||||
uint8_t byte = tx_ring.buffer[tx_ring.tail];
|
||||
tx_ring.tail = (tx_ring.tail + 1) % UART2_TX_BUFFER_SIZE;
|
||||
tx_ring.is_sending = true;
|
||||
|
||||
HAL_UART_Transmit_IT(&huart2, &byte, 1);
|
||||
}
|
||||
```
|
||||
|
||||
**修改后**：
|
||||
|
||||
```c
|
||||
void UART2_Print_Task(void)
|
||||
{
|
||||
uint8_t byte;
|
||||
uint16_t current_tail;
|
||||
|
||||
__disable_irq();
|
||||
if (tx_ring.is_sending || tx_ring.tail == tx_ring.head) {
|
||||
__enable_irq();
|
||||
return;
|
||||
}
|
||||
|
||||
current_tail = tx_ring.tail;
|
||||
tx_ring.tail = (tx_ring.tail + 1) % UART2_TX_BUFFER_SIZE;
|
||||
tx_ring.is_sending = true;
|
||||
byte = tx_ring.buffer[current_tail];
|
||||
__enable_irq();
|
||||
|
||||
HAL_UART_Transmit_IT(&huart2, &byte, 1);
|
||||
}
|
||||
```
|
||||
|
||||
**消除的隐患**：
|
||||
- `tail` 的读取和更新在同一个临界区内完成
|
||||
- 避免了中断回调读取到"部分更新"状态的 `tail` 值
|
||||
- `is_sending` 的设置和 `byte` 的读取分离，确保中断回调不会重复触发发送
|
||||
|
||||
---
|
||||
|
||||
## 4. 总结与建议
|
||||
|
||||
### 整体代码质量评估
|
||||
|
||||
| 维度 | 评分 | 说明 |
|
||||
|------|------|------|
|
||||
| 功能完整性 | ⭐⭐⭐⭐ | 基本功能完整，指令解析逻辑清晰 |
|
||||
| 中断安全 | ⭐⭐ | **存在严重竞态条件，必须修复** |
|
||||
| 边界检查 | ⭐⭐⭐ | 大部分边界有检查，但存在遗漏 |
|
||||
| 可维护性 | ⭐⭐⭐⭐ | 代码结构清晰，注释完善 |
|
||||
| 防御性编程 | ⭐⭐⭐ | 有基本防护，但关键路径需加强 |
|
||||
|
||||
### 必须立即修复的问题（高优先级）
|
||||
|
||||
1. **[高危-1] uart2_print.c `UART2_Print_Send` 竞态条件** - 直接影响数据可靠性和调试能力
|
||||
2. **[高危-2] uart2_print.c `UART2_Print_Task` 缺乏中断保护** - 可能导致数据重复
|
||||
3. **[高危-3] uart2_print.c 环形缓冲区容量计算** - 差一错误影响缓冲区利用率
|
||||
4. **[高危-6] cmd_parser.c 状态机边界处理** - 可能拒绝合法长参数帧
|
||||
|
||||
### 建议后续优化的问题
|
||||
|
||||
5. **[中危-4] UART2_Print_Printf 返回值处理** - 改为 `len >= 0`
|
||||
6. **[中危-5] io_monitor 去抖初始化** - 考虑从 1 而非 DEBOUNCE_COUNT 开始
|
||||
7. **[中危-9] relay_control RELAY_COUNT 宏定义** - 与实际校验逻辑对齐
|
||||
8. **[建议-1] 考虑使用 CMSIS_OS2 或 FreeRTOS** - 如果系统复杂度增加
|
||||
|
||||
### 最重要的3-5条改进建议
|
||||
|
||||
1. **为环形缓冲区操作引入完整的临界区保护**：所有涉及 `head`、`tail`、`is_sending` 的读写操作必须放在同一个 `__disable_irq()/__enable_irq()` 保护块内。
|
||||
|
||||
2. **重新设计环形缓冲区满/空判断机制**：使用显式的 `count` 变量或"保留一位"策略来区分 `head == tail` 的两种语义。
|
||||
|
||||
3. **为 `cmd_parser.c` 的 `vsnprintf` 缓冲区添加保护**：校验和计算应使用固定长度，避免因消息格式变化导致截断。
|
||||
|
||||
4. **增加运行时诊断接口**：如 `UART2_Print_GetOverflowCount()` 已有，建议为各模块添加更多诊断计数器，帮助现场定位问题。
|
||||
|
||||
5. **考虑使用 RTOS 的队列/信号量机制**：如果项目复杂度增加，当前这种裸机共享资源方案的可维护性会下降。
|
||||
|
||||
---
|
||||
|
||||
*审查完成。关键问题已在代码注释中标注，建议优先修复高危问题后再进行功能验证测试。*
|
||||
Reference in New Issue
Block a user