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

4th sournd review commnets #3

Open
unicornx opened this issue Dec 10, 2024 · 4 comments
Open

4th sournd review commnets #3

unicornx opened this issue Dec 10, 2024 · 4 comments

Comments

@unicornx
Copy link

unicornx commented Dec 10, 2024

comment 1

在 duo-pkgtool 目录下导入 env 会失败:

$ cd duo/duo-pkgtool/
$ . ./script/env.sh 
readlink: invalid option -- 'b'
Try 'readlink --help' for more information.
dirname: missing operand
Try 'dirname --help' for more information.
# Hello, here is duo-pkgtool, now configure information for you. 

touch: cannot touch '/.config': Permission denied
Failed to create file '.config' in directory ''.

上面出错的原因貌似是 env.sh 中 DPT_SCRIPT_PATH=$(dirname $(readlink -f "$0")) 有问题,无法得到 duo-pkgtool 的 CWD

我建议的解决方法是将 env.sh 加上可执行权限,然后直接执行该脚本就好了。

另外我希望用户是进入 duo-pkgtool 下执行命令,但看你的 README 写的很奇怪,感觉是要在 duo-pkgtool 同级目录下执行?

  • 每次使用前,需要加载 duo-pkgtool/script/目录下的 env.sh。示例如下:
    $ source duo-pkgtool/script/env.sh 

comment 2

我上次提的 comment 中说用 prompt,但现在觉得 prompt 的设计没有考虑到以后在 RTT 下替代 cvitek_bootloader 的计划。为了兼容目前 RTT 的使用习惯,打包过程中是不会引入交互的,所以 env.sh 需要支持命令行参数选项模式。而且 env 和 mkpkg 都需要考虑 RTT 下替代 cvitek_bootloader 的计划。

prompt 可能只对另一种用户场景有意义,就是制作版本测试和发行版本的人。

所以综合下来我觉得采用还是用 命令行 选项更好,prompt 或许可以以后作为一种高级特性再考虑,第一期暂时先不需要了吧。如果你觉得需要保留,可以实现成一个新的命令,env 这个命令还是先做成纯命令行+参数选项方式的。

comment 3

基于 prompt 的方式,我发现使用 env.h 过程中发现有一种使用场景会有问题:假设执行 env.h 过程中如果配置了 board type 后发现配置错了, ctrl-c 退出,再次执行 env.sh, 就不会允许再配置 board type 了,而是直接进入下一个配置。这个感觉有点不方便。

而且我觉得目前的配置操作实现有点复杂了,env.shchange_config 我觉得可以合并为一个,每次都是从头开始配置一遍就好了。本身配置项不多,多个命令没有必要,大部分软件也就一个 configure 或者 menuconfig 就好了。具体实现时,如果用户以前配置过,譬如环境变量中存在对应的值,可以在 prompt 过程中显示当前值,用户如果不修改直接回车即可。如果没有配置过,则提示默认值,用户直接回车即可,否则输入自己的值。以上做法尽可能降低用户的交互难度,方便使用。

comment 4

有个问题,为啥要生成 .config 配置文件?有持久性的需要吗?我们的用户使用场景存在反复退出当前 shell 又进入的情况吗?如果不存在用户反复退出当前 shell 会话的情况,我觉得导出环境变量在内存种就足够了。我们只需要基于当前 shell 的内存环境变量就可以,这样也简化实现。

comment 5

配置完成后请将导出的环境变量值打印出来,另外输出路径可以是加上了 board type 的完整路径(因为此时已经知道了 board type)

comment 6

rt-thread 的路径我觉得可以只要输入到 rt-thread 就好了。bsp/cvitek 甚至都可以省略,对用户要求越简单越好。

comment 7

输出的默认路径写错了

echo -n "Please input the output directory(You can just input enter, the output directory default is '.../duo-pkgtool/output'): "

为啥是 ”..."? 而且同样我们是以 duo-pkgtool 所在路径为 base 描述相对地址,所以应该是 ./output

comment 8

env.sh

local BOARD_NAME=("duo" "duo256m" "duos")

这个做成全局变量可好,也许可以复用,譬如我看你提示中会写死,譬如:

printf "# The models supported are duo, duo256m and duos.(The default model is duo256m)\n\n"

那如果以后我们再扩展一个 duo 的 model,怎么办?

comment 9

脚本中打印的格式,例如 “\033[91m” 这些还能做成变量宏形式?

comment 10

mkpkg 命令的实现,参考设计 plctlab/plct-rt-thread#1

第四步:执行构建命令。建议打包命令简化设计成一个,大概如下:
mkpkg [board_type] [-a]

board_type: 可选选项,和配置命令的那个参数类似,默认不给就是 duo256m,增加这个参数的目的主要是考虑用户可能会切换打包的 board type,增加这个选项后,可以在打包时覆盖配置时设置的 board type。一个可能的使用场景是测试团队或者发行版团队可能会用我们这个工具批量对duo 的不同产品批处理打包,这样只要一次配置,多次 mkpkg 就好了。
-a:可选项,如果指定了就同时生成 fip.bin 和 boot.sd。默认不要 -a,即只做 boot.sd。因为考虑用户使用场景,这个打包其实更多是为 RT-smart 服务,所以频繁更换的会是 boot.sd

你的实现不满足 board_type 的切换需求。

另外,原设计中 ‘-a' 的考虑是否是不够的?按照你的想法,是不是需要支持三种打包

  • 只打包小核
  • 只打包大核
  • 同时打包大核和小核

这个问题如果确定下来,原设计需要修改。

@unicornx
Copy link
Author

我建议把所有使用场景再梳理一下,避免下次改动还会出现实现不满足需求的情况。

其实就是把所有我们需要支持的 usecase 列一下

@koikky
Copy link
Owner

koikky commented Dec 11, 2024

好的,我再看一下

@unicornx
Copy link
Author

unicornx commented Dec 11, 2024

主要用户分两类:
第一类:测试/发版本的人员
第二类:RTT 开发人员

推荐使用方法

cd duo_pkgtool
source ./func.sh
DPT_KERNEL_PATH=a [DPT_BOARD_TYPE=b] [DPT_OUTPUT_PATH=c] ./mkpkg [-a/-l/-b]

默认值:
DPT_BOARD_TYPE=duo256m
DPT_OUTPUT_PATH=./output
打包类型,默认 -a, 主要考虑第一类用户手动输入最常用习惯。

@koikky
Copy link
Owner

koikky commented Dec 17, 2024

老师,按照之前的方案,又重新做了一个版本。还是这个仓库,上次的放在了分支v1.2里,新的在main分支里。

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