# Phase 1 PLAN.md 第二轮审查报告

**审查日期:** 2026-04-17
**审查对象:** `/Users/rdzleo/Desktop/CogletESP-camera-version/docs/phase-01-face-tracking/PLAN.md` v1.1
**审查方法:** 对照第一轮 PLAN_CHECK.md 每条问题逐一核验 + 实际代码交叉
**审查人:** GSD Plan Checker（第二轮）

---

## 1. 审查结论

**`PASS_WITH_NOTES`**

第一轮提出的 3 个 BLOCKER + 3 个 HIGH 全部得到正确修复，修复质量整体良好。Revision History 完整、决策点 D-07 在"用户已决策"表中明确登记、依赖图笔误已订正、新增风险 R13/R14/R15 与代码逻辑一致。**可以进入执行阶段**。

仅有 2 处轻微注意点（不阻塞，仅作提醒），见第 3 节。

---

## 2. 原问题修复情况表

| 编号 | 严重级 | 原问题 | 是否修复 | 修复质量 | 备注 |
|------|--------|--------|----------|----------|------|
| BLOCKER #1 | 🔴 | T10 facetrack 缺失 idle 判断 | ✅ 已修复 | **好** | T10 修订（L667-734）+ D-07 决策（L48）+ R15 风险登记（L1078） |
| BLOCKER #2 | 🔴 | grove_active 重复更新 | ✅ 已修复 | **好** | T10 删除重复更新代码 + T09（L659）注释明确"只在此处更新" |
| BLOCKER #3 | 🔴 | T01 用 Capture() 过重 | ✅ 已修复 | **好** | T01 重写为最小 V4L2 DQBUF/QBUF probe，预算 < 200ms |
| HIGH #1 | 🟠 | CMakeLists 非 S3 目标 | ✅ 已修复 | **好** | 三重保护：Kconfig depends + CMake REMOVE_ITEM + 源文件 #if 守卫 |
| HIGH #2 | 🟠 | Capture mutex 饥饿 | ✅ 已修复 | **好** | T04 双超时策略（detection 10ms timeout 跳帧，capture portMAX_DELAY） |
| HIGH #3 | 🟠 | staticflag 硬编码 False 漂移 | ✅ 已修复 | **好** | T08 新增 `last_face_raw` + `FACE_STATIC_THRESHOLD=3` 去重逻辑 |
| 笔误 | — | 依赖图 T05→T07 | ✅ 已修复 | **好** | 改为 T06→T07，并在 v1.1 标头明确说明 |

---

## 3. 修复细节核验

### BLOCKER #1（T10 idle 判断）

- **D-07 决策已登记**（L48）：明确"idle 状态下 RP2040 不驱动眼球舵机追踪。ESP32 侧行为不变（按 D-03 始终发送坐标）；RP2040 侧收到 face:x,y 时，若 animation.current_state == 'idle'，仅更新 grove_active/grove_last_seen/last_face_offset 状态，不调用 set_target()"
- **代码位置正确**（L699-702）：idle 判断放在数据消费 + grove_active 超时之后、舵机驱动之前——这是**严格正确的位置**：
  - 先消费 `last_face_offset`（避免数据堆积）✅
  - 再做 3s 超时回退（即便 idle 也要正确清 `grove_active`）✅
  - 最后 `if idle: return`，跳过 servo 驱动 ✅
- **对齐原代码语义**：原 `facetrack()` L53 是 `if animation.current_state != "idle":`，T10 用 `if idle: return` 实现等价转换，且把 `grove_active` 超时判定上移到 idle return 之前——这反而比原代码更稳健（原代码 idle 时根本不更新 grove_active）。
- **DoD 测试用例完整**（L717-730）：手动测试 1（非 idle 追踪）、测试 2（idle 不追踪）、测试 3（3s 超时）、测试 4（staticflag 联动）四项覆盖。

### BLOCKER #2（grove_active 重复更新）

- **T09 单一更新点**（L649-651）：`animation.grove_active = True; animation.grove_last_seen = time.ticks_ms()` 只在 incoming_commands 循环里设置。
- **T09 显式注释**（L659）："grove_active / grove_last_seen 的更新**只在此处**做，T10 的 facetrack() 不再重复更新"——意图清晰。
- **T10 完全删除原 L456-461 重复代码**：T10 修订后只保留"3 秒无数据回退"兜底（L694-697），不再 set True。
- **职责划分清晰**：
  - T09：incoming_commands 收到 face: → set True + 更新 last_seen
  - T10：facetrack 检查超时 → set False
  - 两处不冲突，未来 `grove_last_seen` 真正反映"最后一次收到 ESP32 数据的时间"。

### BLOCKER #3（T01 最小 probe）

- **改为 V4L2 直接 ioctl**（L75-99）：只调一次 `VIDIOC_DQBUF` + `VIDIOC_QBUF`，不触发 JPEG 编码、不做 PSRAM 大分配、不触发 encoder_thread。
- **执行时间预算合理**：DoD（L116）要求 `elapsed < 200ms`——这是合理的（V4L2 DQBUF 在 10 FPS 下理论 < 100ms 就能拿到下一帧；DVP 唤醒延迟也在此预算内）。
- **probe 调用位置正确**（L101-112）：放在 `Application::Start()` 末尾、`protocol_->Start()` 之后——此时 `Esp32Camera` 已构造完成，`streaming_on_=true`，video_fd_ 已 open。
- **保留诊断 API**（L114）：T04 完成后只删 probe 调用、保留 `ProbeFrameCapture` API 作为诊断工具——比原方案"完全删除"更稳。

### HIGH #1（CMakeLists 非 S3）

- **三重保护明确写入** T05（L370-387 + L388-394 兼容性表）：
  1. **Kconfig 层**：`depends on IDF_TARGET_ESP32S3 || IDF_TARGET_ESP32P4`（T02 L139）
  2. **CMake 层**：`if(NOT CONFIG_IDF_TARGET_ESP32S3) list(REMOVE_ITEM SOURCES "face_tracker.cc") endif()`（T05 L380-382）
  3. **源文件层**：`#if defined(CONFIG_XIAOZHI_ENABLE_FACE_TRACKING) && defined(CONFIG_IDF_TARGET_ESP32S3)` 包裹所有 esp-dl include（T05 L319）
- **兼容性表**（L388-394）覆盖 ESP32-S3 / P4 / 原版 / C3 / C6 五种目标的预期行为。
- **DoD 加入交叉验证**（L397）："ESP32（原版）编译通过（验证非 S3 目标不会因 human_face_detect.hpp 缺失而失败）"。
- **轻微注意**：T05 L392 写"ESP32-P4：face_tracker.cc 编译为空壳"——但 T02 Kconfig L139 明确 `depends on IDF_TARGET_ESP32S3 || IDF_TARGET_ESP32P4`，且 T05 L380 CMake 条件是 `if(NOT CONFIG_IDF_TARGET_ESP32S3)` —— 这意味着 P4 目标会被 CMake 排除掉编译（与 Kconfig 允许冲突）。**详见第 4 节 NOTE-1**。

### HIGH #2（Capture mutex 饥饿）

- **T04 双超时策略明确**（L209，修订说明）："face_track 拿不到锁就跳过这一帧（人脸检测允许丢帧），拍照则可完整持有 mutex"。
- **代码层面正确**：
  - `CaptureForDetection`（L240）：`xSemaphoreTake(capture_mutex_, pdMS_TO_TICKS(10))`，拿不到立即返回 false
  - `Capture`（L275）：`xSemaphoreTake(capture_mutex_, portMAX_DELAY)` 等任意时长
- **使用 FreeRTOS Semaphore 而非 std::mutex**：注释（L231）解释了原因（std::mutex 的 `try_lock_for` 在 ESP32 toolchain 上不可移植）——**正确的工程权衡**。
- **新增 R13 风险登记**（L1076）：与代码逻辑完全一致。
- **T12 步骤 7 显式验证**（L796-799）：listening 状态下触发 MCP take_photo，观察 face_tracker 跳帧而非卡死。
- **轻微注意**：`CaptureForDetection` 与 `ReleaseDetectionFrame` 的 mutex 是**跨调用持有**——`CaptureForDetection` 拿锁后不解锁，由后续的 `ReleaseDetectionFrame` 解锁。这是正确的（保护 mmap_buffers_[buf.index] 的内容直到 caller 用完），但 face_tracker_task 必须保证两者**严格成对**调用。**详见第 4 节 NOTE-2**。

### HIGH #3（staticflag 漂移）

- **T08 新增 static 去重逻辑**（L592-606）：
  - 与 `last_face_raw` 比较 dx, dy
  - 阈值 ≤ `FACE_STATIC_THRESHOLD=3` → 设 `staticflag = True`
  - 否则更新 `last_face_raw` + 设 `staticflag = False`
  - 首次收到坐标视为非静态（合理）
- **阈值 3 像素的合理性**：
  - 在 224×224 归一化坐标下约 2.7%
  - 对照 `coms.py` L60 `deadzone = 20`（更宽容），3 像素只用于**消除 bbox 抖动**，不会影响 deadzone 判定
  - 与原 Grove 的 `if boxes_part != self.last_boxes` 字符串完全相等比较（coms.py L80）相比，3 像素阈值更宽容（容忍 bbox 抖动），是**合理升级**
- **T10 不再硬编码 staticflag = False**（L686 注释明确）："注意：staticflag 已在 T08 parse_face 中更新，此处不再设置"
- **DoD 完整覆盖**（L618-628）：基本解析 4 条 + static 去重 4 条断言。
- **R14 风险登记 + D-D 决策点**（L1077, L989-992）：留出实测调校空间。

### 依赖图笔误

- **L900-902 已改为 T06→T07**：图中 T07 的箭头指向 T06，配上注释 "← T06 依赖 T07（v1.1 修正）"
- **v1.1 修订说明明确**（L20, L873-875）："原版图中 T05→T07 是笔误。实际 T05 只是骨架任务（打印 hello），不调用 uart_send_face；是 T06 才调用 uart_send_face 完成坐标推送"
- **执行顺序表同步更新**（L946）："T06 前置：T02、T03、T04、T05、T07 全部完成后才能开工（v1.1 修正）"

---

## 4. 新发现的问题

### 🔵 NOTE（提示性，不阻塞执行）

#### NOTE-1: ESP32-P4 在 Kconfig 允许但 CMake 排除——存在矛盾

- **位置:** T02 L139（Kconfig `depends on IDF_TARGET_ESP32S3 || IDF_TARGET_ESP32P4`） vs T05 L380（CMake `if(NOT CONFIG_IDF_TARGET_ESP32S3)`）
- **问题:** Kconfig 允许 P4 看到选项并打开，但 CMake 在 P4 上会移除 `face_tracker.cc`，导致 face_tracker 函数变成空壳（`#else` 分支的 3 个空函数）。如果用户在 P4 上启用该选项，编译能过但功能不工作——日志里也不会有任何报错。
- **建议:** 二选一：
  - **方案 A**：CMake 改为 `if(NOT CONFIG_IDF_TARGET_ESP32S3 AND NOT CONFIG_IDF_TARGET_ESP32P4)` 一并支持 P4
  - **方案 B**：Kconfig 改为 `depends on IDF_TARGET_ESP32S3` 只支持 S3，把 P4 从声明列表中去掉
- **不阻塞**：本项目硬件是 S3-N16R8，P4 路径根本不会被走到。但作为面向其他用户的 Kconfig 开关，最好和实际 CMake 行为对齐。

#### NOTE-2: `CaptureForDetection` 与 `ReleaseDetectionFrame` 跨调用持锁——face_tracker 任务必须严格成对

- **位置:** T04 L256-258（注释"不解锁！由 ReleaseDetectionFrame 配对解锁"），T06 L443/458 调用配对
- **观察:** T06 的实现已经正确做到了：在 `CaptureForDetection` 返回 true 后立即用 `auto& results = detector->run(img)`，然后立刻调用 `ReleaseDetectionFrame(f)`。中间没有 `continue`/`return`/异常路径会跳过 `ReleaseDetectionFrame`。
- **潜在隐患:** 如果未来有人在 T06 的 if-else 分支中插入 early return（例如某种检测失败的快速路径），可能漏掉 `ReleaseDetectionFrame` → 整个 capture mutex 永久持有 → MCP 拍照永久卡死。
- **建议:** T06 的 ReleaseDetectionFrame 调用建议用 RAII 风格包装：
  ```cpp
  // 可选改进：建一个 helper struct
  struct DetectionFrameGuard {
      Esp32Camera* cam;
      Esp32Camera::FrameRef* f;
      ~DetectionFrameGuard() { if (cam && f) cam->ReleaseDetectionFrame(*f); }
  };
  ```
  或者至少在 T06 的代码注释中加 **"绝对不要在 CaptureForDetection true 之后到 ReleaseDetectionFrame 之间插入 early return"** 的警示。
- **不阻塞**：当前代码路径正确，仅作未来维护提醒。

---

## 5. 已修复但值得点赞的设计

1. **T10 idle return 之前先做 3 秒超时回退**（L694-702）——比原 `facetrack()` 在 idle 下完全不更新 grove_active 的行为**更稳健**。这意味着用户在 idle 下离开摄像头 3 秒后再唤醒到 listening，`grove_active=False` 已被正确清掉，不会出现"基于过期数据驱动眼球"的视觉异常。
2. **T01 保留 `ProbeFrameCapture` 作为诊断 API**（L114）——比"完全删除"更工程化，未来 issue 排查可独立触发。
3. **T05 三重保护**（Kconfig + CMake + 源文件 #if）——任一机制失效另两层兜底，鲁棒性极高。
4. **T08 阈值 3 像素的工程经验值**——对应 coms.py L60 `deadzone=20`，比例合理；而且在 DoD 里给了完整断言（包括边界 = 阈值的 case）。
5. **R13/R14/R15 与 D-D/D-07 双向链接**——风险登记和决策点形成闭环。

---

## 6. Revision History 完整性核验

- ✅ Revision History 表（L13-16）清晰列出 v1.0 → v1.1 的变更摘要
- ✅ "v1.1 涉及修改的任务"明确列举（L18）：T01、T04、T06、T08、T10
  - 注：T05/T07/T09 也有少量修订（CMake/注释），但属于配套调整，未单独标记
  - **轻微遗漏**：T05 实际有修订（CMake 条件编译策略，HIGH #1 修复），但 v1.1 摘要里没列入。建议下次修订时补全。
- ✅ "v1.1 新增决策"（L19）明确列出 D-07
- ✅ "v1.1 依赖图修正"（L20）明确指出 T06→T07
- ✅ 每个修订任务都有 `[修订于 2026-04-17]` 标记：T01（L63）、T04（L206）、T05（L287）、T08（L549）、T10（L667）—— **T06 没有**[修订于 2026-04-17] 标记，但实际 T06 内容相对 v1.0 有微调（FPS 计算加保底防除零）。建议补上。
- ✅ 任务编号保持 T01-T15 不变
- ✅ 任务清单快速参考（L1148-1166）的 v1.1 修订列已正确标注

---

## 7. 推荐下一步

### ✅ 批准进入执行阶段

第一轮的 3 BLOCKER + 3 HIGH 已全部修复且修复质量良好；新增风险 R13/R14/R15 与代码逻辑一致；Revision History 基本完整；依赖图笔误已订正。

**可以执行 `/gsd-execute-phase 01-face-tracking`**。

### 执行过程中建议留意

1. **NOTE-1（P4 行为不一致）**：执行 T02/T05 时如果方便，顺手统一 Kconfig 和 CMake 的目标列表（建议改 Kconfig 为 `depends on IDF_TARGET_ESP32S3` 只支持 S3，因为本项目硬件就是 S3）。
2. **NOTE-2（CaptureForDetection RAII）**：执行 T04/T06 时考虑加 RAII guard 或至少加警示注释，避免未来维护引入 mutex 永久持有的 bug。
3. **R14（FACE_STATIC_THRESHOLD 调校）**：T12 阶段必须实测验证 3 像素阈值是否合适，准备好按 D-D 调整。
4. **D-07 用户体验观察**：T12 步骤 3 显式测试 idle 状态——这是本次修订的核心，务必拍视频/截图记录眼球闭眼且不动的状态作为 ACCEPTANCE.md 证据。

### 不阻塞但可优化（执行后再议）

- 给 T05 / T06 补 [修订于 2026-04-17] 标记（Revision History 完整性）
- T05 兼容性表对 P4 行为表述对齐 NOTE-1 的修订

---

## 8. 审查总结

- **正面：** 第一轮提出的所有问题都得到准确理解和修复；修复方案不仅"满足要求"还体现了工程权衡（如 FreeRTOS Semaphore vs std::mutex、RAII 担心、保留诊断 API）；新增决策 D-07 的设计深入到了"idle 时眼睑闭合所以追踪无意义"的产品语义层面，不是机械修复。
- **负面：** Revision History 标记不完全（T05/T06 缺标记），少量 Kconfig/CMake 目标列表不一致——但都属于轻微注意，不影响执行。
- **结论：** 修订质量超出预期，**PASS_WITH_NOTES**，进入执行阶段。