Skip to content

[componnets][pin]add pin operate command in MSH#5892

Merged
Guozhanxin merged 17 commits into
RT-Thread:masterfrom
xfwangqiang:origin_master
May 30, 2022
Merged

[componnets][pin]add pin operate command in MSH#5892
Guozhanxin merged 17 commits into
RT-Thread:masterfrom
xfwangqiang:origin_master

Conversation

@xfwangqiang

@xfwangqiang xfwangqiang commented Apr 29, 2022

Copy link
Copy Markdown
Contributor

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

[

  1. 因为FINSH功能被删除,关于pin的操作函数无法使用,增加了MSH下的PIN操作函数。
  2. 在PIN操作函数中,为了解析十进制格式和十六进制格式,引入了一些字符串相关的函数,放在msh_parse.c中。
  3. 在AT32的BSP目录中,增加了at32_pin_get函数作为pin num功能的实现。

以上修改,已经在at32f403a-start的BSP和开发板上验证通过。
]

以下的内容不应该在提交PR时的message修改,修改下述message,PR会被直接关闭。请在提交PR后,浏览器查看PR并对以下检查项逐项check,没问题后逐条在页面上打钩。
The following content must not be changed in the submitted PR message. Otherwise, the PR will be closed immediately. After submitted PR, please use a web browser to visit PR, and check items one by one, and ticked them if no problem.

当前拉取/合并请求的状态 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

Comment thread components/drivers/misc/pin.c Outdated
_hw_pin.parent.type = RT_Device_Class_Pin;
_hw_pin.parent.rx_indicate = RT_NULL;
_hw_pin.parent.tx_complete = RT_NULL;
_hw_pin.parent.type = RT_Device_Class_Miscellaneous;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这块为何改回到了RT_Device_Class_Miscellaneous

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

抱歉,合并后只是检测了功能,没有认真的check diff, 我这个代码是很早已经在自已的项目中修改了,合并到这个分支中,没有用patch合并,是直接全文件copy,所以也了这样的问题。马上修改

Comment thread components/drivers/misc/pin.c Outdated
}
MSH_CMD_EXPORT(pinGet, get pin number from hardware pin);
#else
FINSH_FUNCTION_EXPORT_ALIAS(rt_pin_get, pinGet, get pin number from hardware pin);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FINSH_FUNCTION_EXPORT_ALIAS 这个可以删掉了

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

好的

@xfwangqiang xfwangqiang requested a review from mysterywolf May 6, 2022 07:37
Comment thread components/drivers/misc/pin.c Outdated
FINSH_FUNCTION_EXPORT_ALIAS(rt_pin_mode, pinMode, set hardware pin mode);
#ifdef FINSH_USING_MSH
#define PINMODE_USAGE "\r\npinMode pin_xxx mode_xxx\r\n\tpin_xxx : 1\r\n\tmode_xxx : output or input\r\n"
static void pinMode(int argc, char *argv[])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

函数命名请参照RT-Thread编程规范修改。修改为小写字母加下划线的方式。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

已经修改

@mysterywolf

mysterywolf commented May 9, 2022

Copy link
Copy Markdown
Member

感谢,有一个问题,shell命令不要设置成大小写pinMode这样子,这个是区别大小写的,敲命令的时候还需要大小写转换很难受。都改成小写,pinmode

@xfwangqiang

Copy link
Copy Markdown
Contributor Author

感谢,有一个问题,shell命令不要设置成大小写pinMode这样子,这个是区别大小写的,敲命令的时候还需要大小写转换很难受。都改成小写,pinmode

其实我也不喜欢pinMode这样子的命名方式,我这是为了兼容之前Finsh的命令,要不再讨论下,看是不是这次把命名更合理些?

@mysterywolf

Copy link
Copy Markdown
Member

之前finsh的C-Style模式已经停止维护并移除仓库了,因此可以直接重新命名新的MSH命令即可。

@mysterywolf

Copy link
Copy Markdown
Member

@Guozhanxin 郭老师命名这块有没有啥建议?

@Guozhanxin

Copy link
Copy Markdown
Member

@Guozhanxin 郭老师命名这块有没有啥建议?

弄一个统一的命令?

pin [option] [pin_num] ...

pin get_num PA16
pin mode 12 output/input/xxx
pin read 12
pin write 12 0/1

@mysterywolf

Copy link
Copy Markdown
Member

命令就最好别用下划线了吧

@xfwangqiang

Copy link
Copy Markdown
Contributor Author

@mysterywolf @Guozhanxin ,那改成这样?
pin [option] [pin_num] ...

pin num PA16
pin mode 12 output/input/xxx
pin read 12
pin write 12 0/1 or high/low or on/off
pin help

@Guozhanxin

Copy link
Copy Markdown
Member

我感觉可以,就是模式那里可能要在加几种

@mysterywolf

Copy link
Copy Markdown
Member

@mysterywolf @Guozhanxin ,那改成这样? pin [option] [pin_num] ...

pin num PA16 pin mode 12 output/input/xxx pin read 12 pin write 12 0/1 or high/low or on/off pin help

我觉得这种方式可以的

@mysterywolf mysterywolf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

请等待郭老师的review意见后一起修改 感谢抽出时间提交代码

Comment thread components/drivers/misc/pin.c Outdated
rt_kprintf("\thelp\t: this help list\r\n");
}

// e.g. MSH >pin num PA16

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

RT-Thread 中用// 做注释 用/* */

Comment thread components/drivers/misc/pin.c Outdated
value = rt_pin_read(pin);
if (value == PIN_HIGH)
{
rt_kprintf("pin[%d] = on\r\n", pin);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

所有的\r\n 需要改成\n

@mysterywolf mysterywolf requested a review from Guozhanxin May 25, 2022 02:14
Comment thread include/rtthread.h Outdated
rt_bool_t rt_isint(char *strvalue);
int rt_strtoint(char *strvalue);
rt_bool_t rt_ishex(char *strvalue);
int rt_strtohex(char *strvalue);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这些函数,只有MSH使用,加到这里感觉有点草。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

我也很迷茫,但又没有找到更好的地方,郭老师有什么建议没?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

请稍等 正在讨论

Comment thread src/kservice.c Outdated
}
return value * sign;
}
RTM_EXPORT(rt_strtoint);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这些函数使用频次怎么样,有必要放在 kservice里,并且作为符号导出吗

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

应该不用,我马上删除,当时看到其它函数好像都加了,没有注意这点

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

请 在component/finish文件夹中创建 msh_parse.c/.h 文件,这几个函数把rt_前缀改成msh_前缀 放到这个里边去即可 @Guozhanxin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

行,这么办吧

Comment thread include/rtthread.h Outdated
#define rt_strlen(src) strlen(src)
#endif /*RT_KSERVICE_USING_STDLIB*/


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

请删除这个多余的空行

Comment thread components/finsh/msh_parse.h Outdated
rt_bool_t msh_ishex(char *strvalue);
int msh_strtohex(char *strvalue);

#endif /* MSH_PARSE_H */ No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

请再加一个回车

Comment thread src/kservice.c Outdated
* 2021-02-28 Meco Man add RT_KSERVICE_USING_STDLIB
* 2021-12-20 Meco Man implement rt_strcpy()
* 2022-01-07 Gabriel add __on_rt_assert_hook
* 2022-04-29 WangQiang add some function for process arguments in MSH

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

请删除掉这里

Comment thread components/drivers/misc/pin.c Outdated
}
}
MSH_CMD_EXPORT_ALIAS(pin_cmd, pin, pin operate command);
#endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif /* FINSH_USING_MSH */

@mysterywolf mysterywolf requested a review from Guozhanxin May 25, 2022 04:00
@mysterywolf mysterywolf added the +1 Agree +1 label May 25, 2022
@mysterywolf

Copy link
Copy Markdown
Member

唔 这里好不人性 我来改一下

@xfwangqiang

Copy link
Copy Markdown
Contributor Author

唔 这里好不人性 我来改一下

命令确实不人性化,应该是大小写通用,长度应该也不用在意前面的0,我在命令中做处理吧,我还限制在兼容之前命令的思路上,确实不好

@mysterywolf

Copy link
Copy Markdown
Member

xfwangqiang#2 请把这个合并一下
另外你实现的AT32驱动是有问题的,可以参考一下其他的bsp 是PE.13不是PE13
因此msh中的提示也需要对应的修改

@mysterywolf

Copy link
Copy Markdown
Member

我测了一下 STM32下, PE.07和PE.7都是正确的

@mysterywolf

Copy link
Copy Markdown
Member

pin 框架这块没有什么问题,我已经测试过了 只需要把msh提示里加个.

@xfwangqiang

xfwangqiang commented May 29, 2022

Copy link
Copy Markdown
Contributor Author

pin 框架这块没有什么问题,我已经测试过了 只需要把msh提示里加个.

我看了下主线上开始支持这个rt_pin_get是在这个PR #3897 下,我可能前两年就在用这个特性了,没有太注意这里。
我一会把AT32的drv_gpio也改下,不过我实在不太明白这里为什么要加个".",一般使用字母加数字的GPIO命名方式都是“PA01, PA02”,使用全数字的才是“P0.1, P0.2”.

@xfwangqiang

xfwangqiang commented May 29, 2022

Copy link
Copy Markdown
Contributor Author

pin 框架这块没有什么问题,我已经测试过了 只需要把msh提示里加个.

我感觉好像没有必要把这个rt_get_pin这个函数在Driver里实现了,可以直接在PIN架构里实现,实现只需要每个Driver里定义宏GET_PIN就可以了。按目前的思路,每个BSP里都要实现一遍,实现的代码也是一样的,完全可以复用。 @mysterywolf @Guozhanxin 两位你们觉得呢?

@mysterywolf

mysterywolf commented May 29, 2022 via email

Copy link
Copy Markdown
Member

@mysterywolf mysterywolf reopened this May 29, 2022
@mysterywolf

Copy link
Copy Markdown
Member

pin 框架这块修改的没有什么问题,可以先合并我给你提的PR,然后如果AT32驱动不想加的话可以撤掉。

@xfwangqiang

Copy link
Copy Markdown
Contributor Author

pin 框架这块修改的没有什么问题,可以先合并我给你提的PR,然后如果AT32驱动不想加的话可以撤掉。

我点的“Close with comment”,怎么Close这个PR了,看到你reopen了,我才发现,搞得我一脸懵逼。

@mysterywolf

Copy link
Copy Markdown
Member

合并我的pR之后不要再force push了,否则我的PR会被冲掉,上一个给你提PR就没了

@xfwangqiang

Copy link
Copy Markdown
Contributor Author

合并我的pR之后不要再force push了,否则我的PR会被冲掉,上一个给你提PR就没了

好像是origin的Master分支修改了,rebase后,就提交不到我的仓库了,只能force了

@xfwangqiang

xfwangqiang commented May 29, 2022 via email

Copy link
Copy Markdown
Contributor Author

@mysterywolf

Copy link
Copy Markdown
Member

xfwangqiang#3 你好 请把这个再合并一下就差不多了

我感觉好像没有必要把这个rt_get_pin这个函数在Driver里实现了,可以直接在PIN架构里实现,实现只需要每个Driver里定义宏GET_PIN就可以了。按目前的思路,每个BSP里都要实现一遍,实现的代码也是一样的,完全可以复用。

我觉得你这个建议是对的 但是需要留到下一个PR,目前这块让driver去处理字符串确实可能会重复以及每个driver实现的都不一样。设备框架处理起来就很难受。

@mysterywolf mysterywolf added +1 Agree +1 and removed in progress PR/issue in progress. labels May 29, 2022
@Guozhanxin Guozhanxin added the +2 Agree +2 label May 30, 2022
@Guozhanxin Guozhanxin merged commit e8d775f into RT-Thread:master May 30, 2022
levizh added a commit to levizh/rt-thread that referenced this pull request May 31, 2022
…hc32_pr

* 'master' of https://github.com/RT-Thread/rt-thread:
  Hc32 pr (RT-Thread#6003)
  [add] winsock implement on windows simulator. (RT-Thread#6010)
  [spi device] remove _spi_bus_device_control (RT-Thread#5898)
  [net][lwip] Support windows simulator (RT-Thread#5993)
  [componnets][pin]add pin operate command in MSH (RT-Thread#5892)
  [device][adc] implement adc_get_vref (RT-Thread#5988)
  [drivers][hwcrypto] Correct function return value definition (RT-Thread#5984)
  [STM32][RTC] add support for STM32L0 series. (RT-Thread#5994)
  Fix bug when restarting and getting interrupts that are not processed. (RT-Thread#5997)
  [kernel][timer] fixed bug (RT-Thread#6004)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+1 Agree +1 +2 Agree +2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants