Skip to content

Conversation

@HelloByeAll
Copy link
Contributor

@HelloByeAll HelloByeAll commented Apr 18, 2023

拉取/合并请求描述:(PR description)

[

为什么提交这份PR (why to submit this PR)

新增:MSH命令 options补全
]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification

@CLAassistant
Copy link

CLAassistant commented Apr 18, 2023

CLA assistant check
All committers have signed the CLA.

{
switch (MSH_OPT_ID_GET(cmd_list))
{
case RT_Object_Class_Thread: list_thread(); break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感觉这么改,有点复杂,还是感觉如何支持二级命令补全。比较方便

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个只是命令函数中的使用示例,补全和这部分代码没关系

@Guozhanxin
Copy link
Member

目前实现的方案是什么,可以在描述里补充一下。

@HelloByeAll
Copy link
Contributor Author

CMD_OPTIONS_NODE_START(eth_cmd)
CMD_OPTIONS_NODE(1, write, tx data)
CMD_OPTIONS_NODE(2, irq, irq list)
CMD_OPTIONS_NODE(3, speed-test, eth physical speed test)
CMD_OPTIONS_NODE_END
MSH_CMD_EXPORT_ALIAS(eth_cmd, eth, eth cmd, optenable)
#define MSH_FUNCTION_EXPORT_CMD(name, cmd, desc, opt)                           \
                __TI_FINSH_EXPORT_FUNCTION(__fsym_##cmd);                       \
                const char __fsym_##cmd##_name[] = #cmd;                        \
                const char __fsym_##cmd##_desc[] = #desc;                       \
                rt_used RT_NOBLOCKED const struct finsh_syscall __fsym_##cmd =  \
                {                           \
                    __fsym_##cmd##_name,    \
                    __fsym_##cmd##_desc,    \
                    opt,                    \
                    (syscall_func)&name     \
                };

typedef struct msh_cmd_opt
{
    rt_uint32_t         id;
    const char          *name;
    const char          *des;
} msh_cmd_opt_t;

#define CMD_OPTIONS_STATEMENT(command) static struct msh_cmd_opt command##_msh_options[];
#define CMD_OPTIONS_NODE_START(command) static struct msh_cmd_opt command##_msh_options[] = {
#define CMD_OPTIONS_NODE(_id, _name, _des) {.id = _id, .name = #_name, .des = #_des},
#define CMD_OPTIONS_NODE_END    {0},};
void msh_opt_auto_complete(char *prefix)
{
    int argc;
    char *opt_str = RT_NULL;
    msh_cmd_opt_t *opt = RT_NULL;

    if ((argc = msh_get_argc(prefix, &opt_str)))
    {
        opt = msh_get_cmd_opt(prefix);
    }
    else if (!msh_get_cmd(prefix, strlen(prefix)) && (' ' == prefix[strlen(prefix) - 1]))
    {
        opt = msh_get_cmd_opt(prefix);
    }

    if (opt && opt->id)
    {
        switch (argc)
        {
        case 0:
            msh_opt_help(opt);
            break;

        case 1:
            msh_opt_complete(opt_str, opt);
            break;

        default:
            break;
        }
    }
}

将 MSH_CMD_EXPORT_ALIAS 改为可变长度参实现,输入第四个参数时开启选项补全, 将opt的地址放到finsh_syscall中, 输入tab时分析是否需要补全, 通过get到opt的句柄进行二级选项的补全

@HelloByeAll HelloByeAll force-pushed the msh_options_completion branch from 9a257fa to ae42f73 Compare April 18, 2023 09:39
@HelloByeAll HelloByeAll force-pushed the msh_options_completion branch from ae42f73 to 74e14f3 Compare April 18, 2023 09:43
@mysterywolf
Copy link
Member

ROM会额外开销多少?

@HelloByeAll
Copy link
Contributor Author

ROM会额外开销多少?

aarch64 qemu上 代码量增加2064byte 每个命令增加一个指针, 41个命令 text段增加了328bytes, 随后的大小来自于选项的字符串

case RT_Object_Class_Device: list_device(); break;
#endif /* RT_USING_DEVICE */
#ifdef RT_USING_DFS
case 0x100:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

直接十六进制数字,什么情况哈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以考虑另外定义一组宏定义,例如

#define CMD_LIST_OPT_INDEX_THREAD   0
// .....

#endif /* end of FINSH_USING_SYMTAB */

#define GET_MACRO(_1, _2, _3, _FUN, ...) _FUN
#define GET_EXPORT_MACRO(_1, _2, _3, _4, _FUN, ...) _FUN
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个后续如果有宏名称冲突怎么办?

rt_kprintf("\n");
msh_auto_complete(prefix);

msh_opt_auto_complete(prefix);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是否需要和宏FINSH_OPTION_COMPLETION_ENABLED相关?

@BernardXiong
Copy link
Member

确实是一种模式,不过要把代码整理好,写好,还得多考虑下,加油

@HelloByeAll
Copy link
Contributor Author

静态代码检查貌似会失败在宏的可变长度参哪里, 不支持这个检测吗?

@supperthomas supperthomas changed the title 新增:MSH命令 options补全 [components][finsh] 新增:MSH命令 options补全 May 4, 2023
# CONFIG_PKG_USING_MICRO_ROS is not set
# CONFIG_PKG_USING_MCP23008 is not set
# CONFIG_PKG_USING_BLUETRUM_SDK is not set
# CONFIG_PKG_USING_MISAKA_AT24CXX is not set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

這個文件自動生成的,可以刪除。不需要修改。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

修改默认值的时候,最好还是确保所有 bsp 都改了吧。不过讲真,个人认为最好直接不要在 git 仓库存这种可以自动生成的文件。linux 就是这样做的。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这文件是存储的用户配置,比如一些选项打开或者没打开,必须要存的,linux 也有存的

Copy link
Contributor

@a1012112796 a1012112796 May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这文件是存储的用户配置,比如一些选项打开或者没打开,必须要存的

首先这些文件是可以自动生成的 (kconfig 文件有默认配置值)。其次,维护负担问题,例如修改某个组件的kconfig ,那几乎所有 bsp 的 .config 文件都需要修改 (如果不改, 保持历史状态,存的意义何在?). 最直观的感受是这些 .config 文件都很相似。。。

linux 也有存的

linux 没存,.config 在linux默认是ignore的,最多只有一个 xxx_defaultconfig 文件, 用来存储某个 soc 的部分特殊配置. 然后 ACH=XXX ... make xxx_defaultconfig 自动生成完整的 .config 文件。
image

/* #define MSH_CMD_EXPORT_ALIAS(command, alias, desc) or
#define MSH_CMD_EXPORT_ALIAS(command, alias, desc, opt) */
#ifdef FINSH_OPTION_COMPLETION_ENABLED
#define MSH_CMD_EXPORT_ALIAS(...) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好高端的写法 👍

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

Successfully merging this pull request may close these issues.

8 participants