You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/12/17 06:41:21 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #5017: Term
xiaoxiang781216 opened a new pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017
## Summary
since many functions aren't related to termios directly：
- libc: Implement terminal api regardless of CONFIG_SERIAL_TERMIOS setting
- sched: Remove the condition guard from getppid prototype
## Impact
More functions can be called when CONFIG_SERIAL_TERMIOS isn't selected.
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#discussion_r773343212
##########
File path: drivers/serial/serial.c
##########
@@ -1332,12 +1333,28 @@ static int uart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
-#ifdef CONFIG_SERIAL_TERMIOS
case TCFLSH:
{
/* Empty the tx/rx buffers */
- irqstate_t flags = enter_critical_section();
+ irqstate_t flags;
+
+ /* tcdrain is a cancellation point */
+
+ if (enter_cancellation_point())
Review comment:
According to the latest version of the Open Group standard, `tcdrain` is required to be a thread cancellation point:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05_02
So, strictly speaking, the most correct would be to set the cancellation point inside `tcdrain()`.
Even so, it wouldn't compromise the points raised by @xiaoxiang781216, since it would still continue to be a userspace wrapper around the `TCDRN` ioctl call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] normanr commented on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
normanr commented on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-1002600790
I think I started with `bl602evb:gpio`, but I wanted to enable `TTY_SIGINT` and `SIG_DEFAULT` so had `SERIAL_TERMIOS` enabled, then when I saw this commit I tried with `SERIAL_TERMIOS` disabled, and it gave the error above. I think it's because `tc_lflag` is inside `SERIAL_TERMIOS`, but `drivers/serial/serial_io.c` doesn't have a guard (because it used to be protected via `TTY_SIGINT`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei merged pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#discussion_r773343212
##########
File path: drivers/serial/serial.c
##########
@@ -1332,12 +1333,28 @@ static int uart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
-#ifdef CONFIG_SERIAL_TERMIOS
case TCFLSH:
{
/* Empty the tx/rx buffers */
- irqstate_t flags = enter_critical_section();
+ irqstate_t flags;
+
+ /* tcdrain is a cancellation point */
+
+ if (enter_cancellation_point())
Review comment:
According to the latest version of the Open Group standard, `tcdrain` is required to be a thread cancellation point:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05_02
So, strictly speaking, the most correct would be to set the cancellation point inside `tcdrain()`, as suggested by @Ouss4.
Even so, it wouldn't compromise the points raised by @xiaoxiang781216, since it would still continue to be a userspace wrapper around the `TCDRN` ioctl call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#discussion_r773565811
##########
File path: drivers/serial/serial.c
##########
@@ -1332,12 +1333,28 @@ static int uart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
-#ifdef CONFIG_SERIAL_TERMIOS
case TCFLSH:
{
/* Empty the tx/rx buffers */
- irqstate_t flags = enter_critical_section();
+ irqstate_t flags;
+
+ /* tcdrain is a cancellation point */
+
+ if (enter_cancellation_point())
Review comment:
> According to the latest version of the Open Group standard, `tcdrain` is required to be a thread cancellation point: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05_02
>
> So, strictly speaking, the most correct would be to set the cancellation point inside `tcdrain()`, as suggested by @Ouss4
tcdrain still contain the cancellation point indirectly through ioctl::TCFLSH.
There is no real behavior difference before and after this patch:
The thread cancellation point check still has to be kept inside the kernel space since both enter_cancellation_point/leave_cancellation_point can't be called from user space.
The real difference is that syscall entry point switch from tcdrain to ioctl, but it's just a implementation detail.
>. Even so, it wouldn't compromise the points raised by @xiaoxiang781216, since it would still continue to be a userspace wrapper around the `TCDRN` ioctl call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-1002610812
Some patch doesn't upstream yet, @liguiding is preparing them, so wait a moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-1000017076
@Ouss4 @gustavonihei how about the new change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#discussion_r772452911
##########
File path: drivers/serial/serial.c
##########
@@ -1332,12 +1333,28 @@ static int uart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
-#ifdef CONFIG_SERIAL_TERMIOS
case TCFLSH:
{
/* Empty the tx/rx buffers */
- irqstate_t flags = enter_critical_section();
+ irqstate_t flags;
+
+ /* tcdrain is a cancellation point */
+
+ if (enter_cancellation_point())
Review comment:
> Why was the cancellation point check moved into the switch statement inside the ioctl?
This change try to unify terminal function implementation:
1. All functions are simple userspace wrappers around ioctl
2. No special syscall function(tcdrain) anymore
> We usually have it in the API itself (as it was), which make it easy to read and identify.
How about add uart_tcdrain static function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-1002610812
Some patch doesn't upstream yet, @liguiding is preparing them, so wait for a moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-1002563451
> fyi, I get the following with `CONFIG_SERIAL_TERMIOS` disabled (fine without):
>
> ```
> make[1]: Entering directory '/home/normanr/src/nuttx/nuttx/drivers'
> CC: serial/serial_io.c
> serial/serial_io.c: In function 'uart_recvchars':
> serial/serial_io.c:210:32: error: 'uart_dev_t' {aka 'struct uart_dev_s'} has no member named 'tc_lflag'
> if (dev->pid >= 0 && (dev->tc_lflag & ISIG) &&
> ^~
> serial/serial_io.c:249:15: error: 'uart_dev_t' {aka 'struct uart_dev_s'} has no member named 'tc_lflag'
> if ((dev->tc_lflag & ISIG) && ch == CONFIG_TTY_FORCE_PANIC_CHAR)
> ^~
> make[1]: *** [Makefile:111: serial_io.o] Error 1
> make[1]: Leaving directory '/home/normanr/src/nuttx/nuttx/drivers'
> make: *** [tools/LibTargets.mk:89: drivers/libdrivers.a] Error 2
> ```
which config you are using? or share your terminal related config?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#discussion_r773571003
##########
File path: drivers/serial/serial.c
##########
@@ -1332,12 +1333,28 @@ static int uart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
-#ifdef CONFIG_SERIAL_TERMIOS
case TCFLSH:
{
/* Empty the tx/rx buffers */
- irqstate_t flags = enter_critical_section();
+ irqstate_t flags;
+
+ /* tcdrain is a cancellation point */
+
+ if (enter_cancellation_point())
Review comment:
After review my change again, I found that I put the cancellation point check into the wrong space. Please review the new patch again, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-1002610812
Some patch doesn't upstream yet, @GUIDINGLI is preparing them, so wait a moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#discussion_r773565811
##########
File path: drivers/serial/serial.c
##########
@@ -1332,12 +1333,28 @@ static int uart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
-#ifdef CONFIG_SERIAL_TERMIOS
case TCFLSH:
{
/* Empty the tx/rx buffers */
- irqstate_t flags = enter_critical_section();
+ irqstate_t flags;
+
+ /* tcdrain is a cancellation point */
+
+ if (enter_cancellation_point())
Review comment:
> According to the latest version of the Open Group standard, `tcdrain` is required to be a thread cancellation point: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05_02
>
> So, strictly speaking, the most correct would be to set the cancellation point inside `tcdrain()`, as suggested by @Ouss4
tcdrain still contain the cancellation pointer indirectly through ioctl::TCFLSH.
There is no real behavior difference before and after this patch:
The thread cancellation point check still has to be kept inside the kernel space since both enter_cancellation_point/leave_cancellation_point can't be called from user space.
The real difference is that syscall entry point switch from tcdrain to ioctl, but it's just a implementation detail.
>. Even so, it wouldn't compromise the points raised by @xiaoxiang781216, since it would still continue to be a userspace wrapper around the `TCDRN` ioctl call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] normanr commented on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
normanr commented on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-1002561641
fyi, I get the following with `CONFIG_SERIAL_TERMIOS` disabled (fine without):
```
make[1]: Entering directory '/home/normanr/src/nuttx/nuttx/drivers'
CC: serial/serial_io.c
serial/serial_io.c: In function 'uart_recvchars':
serial/serial_io.c:210:32: error: 'uart_dev_t' {aka 'struct uart_dev_s'} has no member named 'tc_lflag'
if (dev->pid >= 0 && (dev->tc_lflag & ISIG) &&
^~
serial/serial_io.c:249:15: error: 'uart_dev_t' {aka 'struct uart_dev_s'} has no member named 'tc_lflag'
if ((dev->tc_lflag & ISIG) && ch == CONFIG_TTY_FORCE_PANIC_CHAR)
^~
make[1]: *** [Makefile:111: serial_io.o] Error 1
make[1]: Leaving directory '/home/normanr/src/nuttx/nuttx/drivers'
make: *** [tools/LibTargets.mk:89: drivers/libdrivers.a] Error 2
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#discussion_r771978425
##########
File path: drivers/serial/serial.c
##########
@@ -1332,12 +1333,28 @@ static int uart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;
-#ifdef CONFIG_SERIAL_TERMIOS
case TCFLSH:
{
/* Empty the tx/rx buffers */
- irqstate_t flags = enter_critical_section();
+ irqstate_t flags;
+
+ /* tcdrain is a cancellation point */
+
+ if (enter_cancellation_point())
Review comment:
Why was the cancellation point check moved into the switch statement inside the ioctl? We usually have it in the API itself (as it was), which make it easy to read and identify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5017: libc: Enable terminal api regardless of CONFIG_SERIAL_TERMIOS setting
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5017:
URL: https://github.com/apache/incubator-nuttx/pull/5017#issuecomment-997427958
@xiaoxiang781216 this has some conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org