jim800121chen
d7b5a2398a
A 階段第一個 milestone、純 bridge.py 層 + ctypes 直接呼叫 KneronPLUS C symbol。
Source:
- server/scripts/kneron_bridge.py: 1207 → 2058 行（+851）
- server/scripts/test_kneron_bridge_firmware.py: 新檔 840 行、36 unit tests 全綠 0.076s
Firmware bundled:
- server/scripts/firmware/KL520/fw_loader.bin（90112 bytes、MD5 aef7cca17bc023abbd6152c46c18e774、與 warrenchen 一致）
- server/scripts/firmware/{KL520,KL720}/VERSION（v2.2.0）
實作對齊 TDD §6.1 規格（98% 對齊度）：
- handler input/output schema 100%
- stage enum: preparing/loading/flashing/verifying/done/error（採 Design 命名）
- reason enum 7/8（disconnect_during_op 留 M9-5 實機測試）
- ctypes binding 1:1 對齊 warrenchen legacy_plus121_runner.py
- 4 個情境 stage 序列驗證通過（KL520 KDP1+loader / KL520 KDP1 缺 loader / KL720 legacy / 已 KDP2）
- timeout 60s/200s、USB stable 5-8s wait、SIGTERM 拒絕邏輯
- progress event schema 完整（percent/stage/message/elapsed_ms/eta_ms/extra）
Reviewer 兩輪審查：
- 第 1 輪：0 Critical / 3 Major / 4 Minor / 4 Suggestion
- 第 2 輪：通過 with 1 Minor + 1 Suggestion（m5 test 死碼 / s5 test 註解、留 M9-2 順手清）
- M3 firmware 字串覆蓋從 substring → 顯式 enumeration + KDP3+ forward-compat（防未來 brick 風險）
- M2 控制流重構（needs_loader/should_run_loader_stage/loader_required_but_missing 三個顯式 bool）
- m3 single-owner disconnect 原則完整落地
既有 6 個 handler（scan/connect/disconnect/reset/load_model/inference）零改動、無 spillover risk。
下一步：M9-2 Go driver UpgradeFirmware + firmware/service.go
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
146 lines
8.0 KiB
Markdown
146 lines
8.0 KiB
Markdown
# M9-1 Reviewer Round 2 — bridge.py firmware_upgrade（第 2 輪修改驗證）
|
||
|
||
> 審查日期：2026-05-25
|
||
> 範圍：backend 第 2 輪修改驗證、不重審第 1 輪細節
|
||
> 第 1 輪報告：`m9-1-bridge-firmware-upgrade-review.md`
|
||
|
||
## TL;DR
|
||
|
||
**通過（with 1 Minor + 1 Suggestion）。建議解除 M9-2 阻擋、可啟動。**
|
||
|
||
backend 第 2 輪修改紀律高、所有 Major / Minor 全部正確落地、4 個情境 stage 序列邏輯清晰可讀、ctypes 簽名測試補上、firmware 字串覆蓋從 substring → 顯式 enumeration + forward-compat。第 2 輪僅發現 1 個 Minor regression（test 檔留下 `_firmware_upgrade_start_ts` 死碼、不影響 prod 但 cleanup 不徹底）+ 1 個 Suggestion。**第 1 輪 8 項 issue 修了 8 項、新發現 0 Critical / 0 Major / 1 Minor / 1 Suggestion**。s1 / s2 follow-up 評估合理、不影響 M9-2 啟動。
|
||
|
||
---
|
||
|
||
## 1. 第 1 輪 issue 修改驗證（逐項）
|
||
|
||
| # | 第 1 輪 Issue | 第 2 輪修改位置 | 驗證結果 |
|
||
|---|--------------|--------------|---------|
|
||
| **M1** | `_FwError` / `_FwTimeoutError` / `_fw_handle_failure` 宣告位於 handler 之後、readability + 防禦問題 | `kneron_bridge.py:1535 / 1545 / 1553` 全部移到 handler（1587）之前 | ✅ 完全到位 |
|
||
| **M2** | `needs_loader` 控制流隱式 | 1717-1722：三個顯式 bool（`needs_loader` / `should_run_loader_stage` / `loader_required_but_missing`）+ 註解四情境 | ✅ 完全到位、M9-2 Go driver 可對照註解 |
|
||
| **M3** | substring match 對 KDP3 forward-compat 脆弱 | `_fw_classify_legacy` 重寫（1463-1508）：`legacy_exact` set + `startswith("KDP1.")` + KDP2-9 prefix forward-compat | ✅ 完全到位 |
|
||
| **m1** | `_FW_ALLOWED_CHIPS` 雙重防護 | 1180 constant + 1201 內部過 + 1204 字元防護 | ✅ 真的雙重防護、非冗餘 |
|
||
| **m2** | libkplus fallback non-deterministic | 1324 `sorted()` + 1330 WARNING log | ✅ 完全到位 |
|
||
| **m3** | double-disconnect risk | `_fw_handle_failure` 不再 disconnect、success 路徑兩處設 `dg=None`、finally 用 `if dg is not None` | ✅ Single owner 原則完整 |
|
||
| **m4** | `_firmware_upgrade_start_ts` 全域變數 | prod code 全砍、SIGTERM closure capture | ⚠️ prod 完全到位、但 test 檔殘留死碼（標 m5） |
|
||
| **s4 (1-4)** | 4 個 test case | 4 個新 test 全補（line 405-487 + 500-568） | ✅ 完全到位 |
|
||
|
||
第 1 輪 8 項 issue 修了 8 項、其中 7 項完全到位、1 項（m4）prod 完全到位但 test 殘留（標 Minor m5）。
|
||
|
||
---
|
||
|
||
## 2. 第 2 輪新發現（regression risk）
|
||
|
||
### Critical / Major
|
||
**無**。
|
||
|
||
### 🟡 Minor
|
||
|
||
| # | 軸 | 檔案:行 | 問題 | 建議修法 |
|
||
|---|---|---------|------|---------|
|
||
| **m5** | Correctness / Test hygiene | `test_kneron_bridge_firmware.py:616, 632` | m4 在 bridge.py 已砍 `_firmware_upgrade_start_ts`、但 test 仍 `bridge._firmware_upgrade_start_ts = 0.0`、`bridge._firmware_upgrade_start_ts = start_ts`。Python 動態 setattr 不會拋錯、但留下死碼掩蓋 m4 修改徹底性 | 刪除 line 616 + 632 兩行賦值、保留 615 `_firmware_upgrade_in_progress = False` |
|
||
|
||
### 💡 Suggestion
|
||
|
||
| # | 軸 | 檔案:行 | 建議 |
|
||
|---|---|---------|------|
|
||
| s5 | Test | `test_kneron_bridge_firmware.py:680` | `test_sigterm_handler_unregistered_after_upgrade` 第二次呼叫 register 會覆蓋第一次的 `_fw_original_sigterm_handler`、測試邏輯能過但意圖不清。建議補註解「測試 register 的 idempotence」或拆兩個 test |
|
||
|
||
---
|
||
|
||
## 3. 4 個情境 stage 序列驗證（M2 重構後）
|
||
|
||
| 情境 | 預期 stage 序列 | 實作確認 | 結果 |
|
||
|------|---------------|---------|------|
|
||
| 1. KL520 KDP1 legacy + loader.bin | preparing → loading → flashing → verifying → done | 1739-1783 loader stage + 1799-1812 `kp_load_firmware_from_file`；test `test_kl520_kdp1_legacy_full_5_stages`（line 214-247） | ✅ |
|
||
| 2. KL520 KDP1 legacy 缺 loader.bin | preparing → error(loader_write_failed) | 1730-1736 raise `_FwError("loading", "loader_write_failed", ...)`；test `test_loader_write_failed`（line 346-358） | ✅ |
|
||
| 3. KL720 KDP1 legacy 無 loader.bin（warrenchen 模式）| preparing → flashing → verifying → done | 1784-1788 `elif needs_loader` skip loading + 1813-1828 `kp_update_kdp_firmware_from_files(scpu, ncpu, True)`；test `test_kl720_kdp_legacy`（line 271-288） | ✅ |
|
||
| 4. 已 KDP2（KL520 / KL720）| preparing → flashing → verifying → done | `_fw_classify_legacy` KDP2 prefix 命中 return False、走 warrenchen 模式；test `test_kl520_already_kdp2_short_circuit`（line 249-269） | ✅ |
|
||
|
||
**四情境 stage 序列邏輯與 test 驗證皆完全對齊、M2 重構成功。**
|
||
|
||
---
|
||
|
||
## 4. s1 / s2 follow-up 評估
|
||
|
||
| follow-up | 留 follow-up 理由 | 對 M9-2 影響 |
|
||
|-----------|----------------|------------|
|
||
| **s1**（handler ~300 行抽 helper）| 抽 helper 需傳大量 shared state、closure 設計細節多、M2 已讓 main flow 可讀性大幅提升、ROI 不高 | ❌ 無影響、M9-2 看 bridge 介面而非內部結構 |
|
||
| **s2**（poll loop 用 exponential backoff）| 實測 5s 已穩、上界 8s、最多 6 次 poll、CPU 開銷可忽略、純 micro-optimization | ❌ 無影響、Go driver 不參與 polling |
|
||
|
||
兩個 follow-up 都不阻擋 M9-2。
|
||
|
||
---
|
||
|
||
## 5. TDD §6.1 對齊度
|
||
|
||
| 項目 | round 1 | round 2 |
|
||
|------|---------|---------|
|
||
| Handler input/output schema | 100% | 100% |
|
||
| stage enum | 100% | 100% |
|
||
| reason enum 8 種 | 7/8 | 7/8（`validate_failed` downgrade-only、A 階段不需） |
|
||
| progress event schema | 100% | 100% |
|
||
| Stage 觸發點 | ✅ | ✅ M2 重構後流程更清晰 |
|
||
| Timeout 60s/200s | ✅ | ✅ |
|
||
| USB stable 5-8s | ✅ | ✅ |
|
||
| Graceful shutdown 拒絕 | ✅ bridge 端 | ✅ bridge 端 |
|
||
| ctypes 走法 | ✅ | ✅ + binding 簽名 test |
|
||
| MAGIC 值 | ✅ | ✅ |
|
||
| 防 firmware 字串脆弱 | ⚠️ substring | ✅ 顯式 enumeration + forward-compat |
|
||
|
||
對齊度 **98% → 98%**（preparing stage 細分 scan/connect sub-message 留 M9-5）；但 M2 + M3 對「未來 KDP3+ device 不會誤觸 loader」這個未來相容性問題顯著改善。
|
||
|
||
---
|
||
|
||
## 6. 既有 6 個 handler 零改動驗證
|
||
|
||
| Handler | round 1 行為 | round 2 行為 | 結果 |
|
||
|---------|------------|------------|------|
|
||
| `handle_scan` / `handle_connect` / `handle_disconnect` / `handle_reset` / `handle_load_model` / `handle_inference` | 693-1202 | 693-1202 | ✅ 6/6 零改動、無 spillover |
|
||
|
||
第 2 輪修改完全限制在 firmware_upgrade 區段（1205-1983）+ `_resolve_firmware_paths_full`（1183-1240）+ 1180 `_FW_ALLOWED_CHIPS` constant。
|
||
|
||
---
|
||
|
||
## 7. 9 個新增 tests 品質評估
|
||
|
||
| Test | 對應 | 品質 |
|
||
|------|------|------|
|
||
| `test_kl520_legacy_empty_firmware_string` | M3/s3 | ✅ 邊界完整 |
|
||
| `test_kl520_legacy_usb_boot_strings` | M3/s3 | ✅ subTest 7 變體 |
|
||
| `test_kl520_legacy_kdp1_variants` | M3/s3 | ✅ KDP1.x / 大小寫 |
|
||
| `test_kdp3_kdp4_not_legacy` | s3 | ✅ **最關鍵**、KDP3-9 forward-compat |
|
||
| `test_unknown_firmware_default_not_legacy` | M3 | ✅ 保守 default |
|
||
| `test_loading_stage_disconnect_during_op` | s4(1) | ✅ 覆蓋 1764-1775 |
|
||
| `test_loading_stage_reconnect_failed` | s4(2) | ✅ `call_count` 攔截 |
|
||
| `test_failure_event_full_extra_fields` | s4(3) | ✅ 4 必填欄位 |
|
||
| `test_libkplus_binding_signatures` | s4(4) | ✅ MockCDLL 設計巧妙 |
|
||
|
||
9 個新 test 都覆蓋盲點、命名清楚、docstring 帶 line ref、品質高。
|
||
|
||
---
|
||
|
||
## 8. 是否阻擋 M9-2
|
||
|
||
**否、解除阻擋、可啟動 M9-2**。
|
||
|
||
理由：
|
||
1. 第 1 輪 3 Major 全部完全到位、其中 M2 控制流重構對 M9-2 Go driver 開發直接有幫助
|
||
2. M3 firmware 字串覆蓋解決未來 KDP3+ device brick 風險、長期穩定性關鍵改善
|
||
3. m5 是 test hygiene 問題、不阻擋功能
|
||
4. s1 / s2 follow-up 對 M9-2 介面層完全無影響
|
||
|
||
---
|
||
|
||
## 9. 是否需 backend 第 3 輪修改
|
||
|
||
**否**。僅 1 Minor + 1 Suggestion、可在 M9-2 期間順手清掉或併入 M9-1 PR 收尾 cleanup commit。
|
||
|
||
---
|
||
|
||
## 10. 結論
|
||
|
||
- **通過 with Minor**：m5 + s5 不阻擋 M9-2
|
||
- **M9-2 啟動建議**：立即派 backend
|
||
- **m5 + s5 處理**：M9-2 期間順手清、不必為此單獨派 backend round 3
|