Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Third sournd review commnets #1

Open
unicornx opened this issue Dec 5, 2024 · 2 comments
Open

Third sournd review commnets #1

unicornx opened this issue Dec 5, 2024 · 2 comments

Comments

@unicornx
Copy link

unicornx commented Dec 5, 2024

COMMENT-1

shell 脚本格式不统一,请将缩进全部统一为 TAB,目前看上去主要是 script/env.sh 需要改

COMMENT-2

shell 脚本中的打印输出请使用英文,RTT 和 DUO 目前也考虑海外用户。

COMMENT-3

README.md 的 markdown 格式建议改进,特别是中英文字符之间缺少空格隔开,以及文件路径或者变量名要用 "`" 括起来。具体要求参考 https://github.com/plctlab/plct-rt-thread/blob/notes/0.notes/20241115-notes-example.md

COMMENT-4

README.md 中前面加上章节目录,章节标题加上序号。具体例子看 https://github.com/plctlab/plct-rt-thread/blob/notes/0.notes/20241115-notes-example.md。如果你用 VSCode, 可以使用 "Markdown All in One" 插件自动加入目录和章节序号。

COMMENT-5

配置环境中的环境变量 DPT_PATH 我觉得应该不需要配置,因为我们可以要求用户进入 duo-pkgtool 目录下执行所有命令, 如果要支持在任何位置都可以执行 duo-pkgtool,这个需求太强了,没有必要,而且对于这种场景,linux 里的常规做法是加入 PATH, 也不需要另外单独配置 DPT_PATH

只要我们要求用户进入 duo-pkgtool 目录下执行所有命令,则只是要获取当前工作路径,假设为 CWD,就可以找到 duo-pkgtool 下的所有文件了,所有操作也是相对于 CWD 进行。

COMMENT-6

配置环境中的环境变量不满足要求,具体看 plctlab/plct-rt-thread#1 中的说明。
plctlab/plct-rt-thread#1 中列的至少三个环境变量也是考虑了未来在 RTT 环境下执行 duo-pkgtool 的情况(保持现有习惯前提下替换现有的 cvitek_bootloader),你也可以提前考虑一下,看看还有其他问题,欢迎随时提出。

COMMENT-7

DPT 需要环境变量我觉得不需要配置到 .bashrc 中,只要执行时在当前 shell 中 export 就好了,这样对用户系统影响最小,而且考虑到以后这个工具在 RTT 仓库下替换现有的 cvitek_bootloader 的需求,也应该尽量避免引入额外步骤去配置 .bashrc 文件。

另外考注意虑到两种应用场景:

  • 直接手动运行 duo-pkgtool, 可以采用 prompt 提示交互让用户输入环境变量,类似 menuconfig,但我们变量少,就不用做 menuconfig 那么复杂了。
  • RTT 下执行 scons 后台调用 duo-pkgtool 的场景,此时对这些 DPT 环境变量,我理解是可以直接自动获得的,就不用用户 prompt 输入了,这样在用户体验上也是和现在保持一致。

COMMENT-8

打包的步骤没有考虑 board type 的问题,具体看 plctlab/plct-rt-thread#1 中的说明。

COMMENT-9

README.md 中的 “配置环境” 和 “加载环境” 我觉得可以合并为一步,就是 plctlab/plct-rt-thread#1 中的第三步,可能叫 “设置环境” 更好些。本质就是获取那些 DPT 环境变量并 export,同时加上你代码中 import 一些函数。

COMMENT-10

“设置环境” 中还应该加入一些对我们使用依赖的外部安装软件的检查,譬如 mkimage 等。而且这些需要额外 apt 安装的步骤在 README 中请加上。

COMMENT-11

“设置环境” 这步做好后,在 print_usage 中应该打印出我们 export 的关键的 DPT 环境变量的值,以便用户查看,因为这些值对于我们后继的工作很关键。

COMMENT-12

函数 print_usagehelp 容易引起误解。习惯上 print_usage 就是 help。我理解你这里的 print_usage 应叫 print_env

COMMENT-13

prebuilt/milkv-duo256m/fsbl/fiptool.py 这个文件是公用的吧,如果是公用的,我建议放到 prebuild 下去。 还有一种选择是放到 script 下,但是我考虑到这个文件可能和 prebuild 的image一样会需要随着 duo-sdk 的版本升级而升级,可能需要作为 plctlab/plct-rt-thread#6 工作的一部分,放在 prebuild 下体现了这个思想,当然从文件类型来看,是个脚本,似乎放在 script 下更好。大家可以讨论一下?

但 anyway,这个文件我记得对 duo 是 common 的,请 double-check。

@koikky
Copy link
Owner

koikky commented Dec 9, 2024

老师,完成了一个版本。还是这个仓库,之前的放在了分支v1.1里,新的在main分支里。

@unicornx
Copy link
Author

COMMENT-7

DPT 需要环境变量我觉得不需要配置到 .bashrc 中,只要执行时在当前 shell 中 export 就好了,这样对用户系统影响最小,而且考虑到以后这个工具在 RTT 仓库下替换现有的 cvitek_bootloader 的需求,也应该尽量避免引入额外步骤去配置 .bashrc 文件。

另外考注意虑到两种应用场景:

  • 直接手动运行 duo-pkgtool, 可以采用 prompt 提示交互让用户输入环境变量,类似 menuconfig,但我们变量少,就不用做 menuconfig 那么复杂了。
  • RTT 下执行 scons 后台调用 duo-pkgtool 的场景,此时对这些 DPT 环境变量,我理解是可以直接自动获得的,就不用用户 prompt 输入了,这样在用户体验上也是和现在保持一致。

这里提的 prompt 方式我感觉会有问题,估计是这个误导你了,你看看要不要改一下?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants