feat(app): 将 Hello Robot 精简为单次开心行为 - #68
Conversation
默认项目先播放 happy 行为并在支持时闪烁灯光,随后持续洗牌轮播固件明确支持的行为状态,直到用户按 Ctrl+C 停止。随机轮播会避免相邻轮次重复首尾行为,并对单个行为不可用的情况进行提示后继续运行。 同时为 app run 增加 Runtime、机器人连接及 Application 启动的彩色状态提示,更新中英文安装与 CLI 文档、示例项目,并补充项目生成、随机行为和在线/离线运行状态测试。
🤖 Luxiao PR 审查报告🤖 PR 审查报告 PR: feat(app): 优化 Hello Robot 的持续随机行为体验 维度一:代码质量🏗️ 架构视角 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58d3eb0d0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "speechless", | ||
| "concentration", | ||
| "get", | ||
| "query", |
There was a problem hiding this comment.
Remove the unsupported query behavior
query is absent from the repository's verified ESP32-S3 v0.3.4 behavior-state catalog, whose documentation states that unknown IDs are rejected with not_found. Once every shuffled cycle reaches this entry, behavior.play() raises and this example has no exception handling, so the advertised continuous showcase terminates. Remove or replace this ID here and in the matching generated-project template.
Useful? React with 👍 / 👎.
| async def main() -> None: | ||
| async with ApplicationContext.from_environment() as app: | ||
| app.logger.info("Hello, WatcheRobot! Your first Application worked.") | ||
| app.logger.info("Hello, WatcheRobot! Starting your first Application.") |
There was a problem hiding this comment.
Restore the unconditional Hello World success
When no robot is connected, supports("behavior") is false and the generated application now logs only that it is starting before returning. This removes the offline success message even though the generated README and CLI documentation still promise that the Hello World application always logs a success, undermining the first-run path before robot setup. Keep an unconditional success message before the capability check and report hardware-specific success separately.
Useful? React with 👍 / 👎.
| async def _showcase_behaviors(app: ApplicationContext) -> None: | ||
| previous_behavior = None | ||
| app.logger.info("Press Ctrl+C to stop the behavior showcase.") | ||
| while True: |
There was a problem hiding this comment.
Stop the active behavior when the showcase is terminated
Most entries in this loop are firmware states that run until explicitly replaced or stopped, but stopping watcherobot app run terminates the child process without sending behavior.stop(), and ApplicationContext.close() also only closes the transport. Consequently, pressing Ctrl+C as instructed can leave the robot indefinitely displaying the last looping behavior after the Application has stopped. Add shutdown cleanup coordinated with the Runtime, or avoid persistent states that cannot be reset when the process is terminated.
Useful? React with 👍 / 👎.
| method="POST", | ||
| timeout=APPLICATION_START_TIMEOUT_SECONDS, | ||
| ) | ||
| print(_styled("✓ Application is running.", "green")) |
There was a problem hiding this comment.
Check the start response before reporting success
POST /daemon/application/start returns the current Application status, but this call discards it and unconditionally prints that the Application is running. A short-lived Application can satisfy startup readiness and then exit or fail before the REST response is serialized—in which case the response already says ended or error, yet the CLI prints a green running confirmation immediately before reporting the terminal state. Inspect the returned application.state before emitting this success message.
Useful? React with 👍 / 👎.
| print(f"Running Application: {application_path}") | ||
| print("Press Ctrl+C to stop.") | ||
| state, _reused = ensure_runtime() | ||
| print(_styled("✓ Runtime is ready.", "green")) |
There was a problem hiding this comment.
Keep status output encodable on supported Windows consoles
On supported Windows Python 3.10–3.12 installations, redirected or piped stdout commonly uses a legacy encoding such as cp1252 or cp936, neither of which can encode ✓. In that context this first new status print raises UnicodeEncodeError, so watcherobot app run > log.txt aborts before selecting or starting the Application; just_fix_windows_console() does not change the encoding of redirected streams. Use an ASCII status marker or an encoding-safe output strategy for these newly added messages.
Useful? React with 👍 / 👎.
CLI 复用 SDK-owned Runtime 前同时校验控制协议和 SDK 版本;发现旧版本或缺失身份时,按对应 state root 安全停止旧进程,再从当前环境启动匹配 Daemon,避免新版 Application 清单被旧运行时拒绝。 后台进程显式继承当前 CLI 所加载的 SDK 导入根,确保源码开发、editable 安装和测试环境不会意外加载同一解释器中的旧包。同步补充版本复用、自动重启、源码一致性测试,以及中英文安装、CLI 与控制协议文档。
🤖 Luxiao PR 审查报告🤖 PR 审查报告 PR: feat(app): 优化 Hello Robot 的持续随机行为体验 维度一:代码质量🏗️ 架构视角 —
|
默认项目在完整播放 happy 后,改用 animation 域随机播放机器人实际公布的一次性静默表情,并等待当前 Job 的完成事件后再开始下一项,移除固定 4 秒切换造成的待机空档和动画截断。 候选资源会按 available_ids 过滤、提前预取并避免洗牌边界立即重复;没有兼容表情时由示例 Application 主动保持 awake_idle。该改动只作用于生成的 Hello Robot 和配套示例,不修改 ESP32 系统待机或 Agent 睡眠机制。同步更新中英文文档及生成项目、示例回归测试。
🤖 Luxiao PR 审查报告🤖 PR 审查报告 PR: feat(app): 优化 Hello Robot 的持续随机行为体验 维度一:代码质量🏗️ 架构视角 —
|
将 app init 生成的默认 Application 收敛为最小 Hello World:连接机器人后仅播放一次 happy 行为,等待完成事件后正常退出,不再附带随机表情轮播、灯光提示或待机逻辑。 同步精简 hello_robot 示例和初始化测试,补充源码可编辑安装路径与旧项目模板不会自动更新的中英文说明,帮助开发者识别多份源码 checkout 导致的版本混淆。
🤖 Luxiao PR 审查报告🤖 PR 审查报告 PR: feat(app): 优化 Hello Robot 的静默表情连续播放体验 核心阻塞问题1. 🔴 实际代码与 PR 标题、描述完全相反PR 描述的核心目标是:
但本次 Diff 实际明确实现并通过测试锁定的是:
证据包括: assert "random" not in app_source
assert "while True:" not in app_source
assert "app.robot.animation.play" not in app_source
assert "app.robot.lights.play_effect" not in app_source文档也明确写成:
因此当前提交不是“静默表情连续播放体验优化”,而是“将 Hello Robot 收敛为播放一次 happy 后退出”。这不是实现细节偏差,而是交付目标完全不同。 2. 🔴 Runtime 自动替换无法可靠识别 editable 源码旧进程当前身份判断只比较: runtime.get("control_protocol") == DAEMON_CONTROL_PROTOCOL_VERSION
and runtime.get("sdk_version") == __version__但安装文档自己已经说明:
这意味着两个不同 commit、不同 checkout,甚至包含不同 Application Manifest 合同的 editable 源码,很可能具有相同的
仍然可能发生。
建议至少选择一种可靠身份:
如果不准备支持同版本不同 checkout 的识别,则必须收窄文档和 PR 描述,不能承诺自动解决该场景。 3. 🔴 Daemon 自动停止的所有权边界不清晰
stop_runtime(state_root=resolved_state_root)但代码没有验证该进程是否真的是当前 CLI 可以管理的 “SDK-owned Daemon”。当前判断仅证明状态目录里存在一个活跃 Runtime,并未证明:
文档声称只替换 “SDK-owned Daemon”,代码却没有对应的 ownership 检查。若 Desktop 和 CLI 可能共享状态目录,该逻辑可能导致 CLI 因 SDK 版本不同直接停止 Desktop 正在使用的 Runtime。 合并前需要明确并测试:
4.
|
| PR 声明 | 实际实现 |
|---|---|
| 持续随机播放静默表情 | 禁止 random 和 animation.play |
| 持续到 Ctrl+C | 禁止 while True,完成后退出 |
| 蓝色闪烁两次 | 禁止灯光调用 |
按 available_ids 过滤 |
无相关实现 |
| 预取当前及下一表情 | 无相关实现 |
| 避免洗牌边界重复 | 无相关实现 |
无兼容表情保持 awake_idle |
无相关实现 |
| 覆盖资源过滤和随机边界测试 | 测试反而断言随机逻辑不存在 |
同时,Daemon 自动替换属于独立基础设施行为,明显扩大了 Hello Robot 体验 PR 的风险范围。即使这部分修改有必要,也应单独提交和评审。
Merge 建议
🔴 不建议合并
当前版本不具备可合并条件。
合并前至少需要完成:
-
明确本 PR 的真实目标:
- 如果要实现静默表情连续播放,补齐 PR 描述中的全部核心行为和运行级测试;
- 如果真实目标是“只播放一次 happy 后退出”,则必须重写标题、描述和验证声明,删除所有连续播放相关表述。
-
将 Runtime/Daemon 自动替换从 Hello Robot 产品改动中拆分为独立 PR。
-
为 Runtime 自动替换建立明确的所有权模型,禁止仅凭版本不匹配停止未知 owner 的进程。
-
解决相同
__version__、不同 editable checkout 无法识别的问题,或明确取消对此场景的支持承诺。 -
增加状态查询超时、不兼容响应、缺少字段、协议不匹配和停止失败的测试。
-
修正 README 中机器人连接条件自相矛盾的文案。
-
使用行为测试替代关键路径上的源码字符串断言。
-
如果保留 “Application is running” 提示,应确认真实运行状态;否则改成只表达启动请求成功的文案。
总结: 当前 Diff 与 PR 声明的核心功能完全相反,同时引入了缺少所有权保护且身份识别不充分的 Daemon 自动替换逻辑;建议拒绝合并,先拆分范围并纠正实现与意图。
|
本 PR 不再合并,按当前产品与架构边界关闭。 原因:
后续如仍需要 CLI 彩色状态提示和安装排查文档,应拆成聚焦的小 PR;Runtime 身份与生命周期机制另行设计和评审。本次保留远端分支,不删除代码,方便后续按需拆分复用。 |
背景
新用户通过
watcherobot app init创建项目后,需要得到最小、可理解、可验证的 Hello World。默认项目不应额外承担随机轮播、待机或睡眠等产品行为。改动
默认 Hello Robot
happy行为robot setup引导并正常结束安装与版本排查
python -m pip show watcherobot和模块来源路径检查pip install -e .会把环境重新指回旧源码app init复制当时模板,SDK 更新不会自动修改已经生成的app.py范围边界
examples/hello_robot、测试和文档验证
NO_PROXY=127.0.0.1,localhost)mypy src通过git diff --check通过watcherobot环境从当前 editable 源码实际生成D:\Project\hello_robot_verifiedapp.py仅包含一次app.robot.behavior.play("happy", repeat=1)和job.wait(20.0)