You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "no1wudi (via GitHub)" <gi...@apache.org> on 2023/02/10 09:52:27 UTC
[GitHub] [nuttx-apps] no1wudi opened a new pull request, #1559: nsh: Decouple with CONFIG_FILE_STREAM
no1wudi opened a new pull request, #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559
## Summary
Replace all fwrite/fread/fgets/... to write/read/... .
Before:
```
text data bss dec hex filename
100527 317 5324 106168 19eb8 nuttx/nuttx
```
After:
```
text data bss dec hex filename
99073 317 5324 104714 1990a nuttx/nuttx
```
After with CONFIG_FILE_STREAM disabled:
```
text data bss dec hex filename
96759 317 5324 102400 19000 nuttx/nuttx
```
## Impact
nsh
## Testing
QEMU
--
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] [nuttx-apps] no1wudi commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1442673298
> @no1wudi As for esp32-devkitc, I found out how to reproduce the issue. Please try wapi_smp without CONFIG_NET_TCP_WRITE_BUFFERS=y. You would see the same symptom as spresense:wifi_smp In this case (i.e. esp32-devkitc:wapi_smp without CONFIG_NET_TCP_WRITE_BUFFERS=y), reverting this PR would not cause the issue.
>
BTW, in my test, I'm using esp32**c3**-devkit, is'a single core chip.
--
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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1442679294
>BTW, in my test, I'm using esp32c3-devkit, is'a single core chip.
I've just confirmed that non-SMP can reproduce the issue too.
```
--- a/boards/xtensa/esp32/esp32-devkitc/configs/wapi_smp/defconfig
+++ b/boards/xtensa/esp32/esp32-devkitc/configs/wapi_smp/defconfig
@@ -67,7 +67,6 @@ CONFIG_NET_ICMP=y
CONFIG_NET_ICMP_SOCKET=y
CONFIG_NET_STATISTICS=y
CONFIG_NET_TCP=y
-CONFIG_NET_TCP_WRITE_BUFFERS=y
CONFIG_NET_UDP=y
CONFIG_NFS=y
CONFIG_NSH_ARCHINIT=y
@@ -89,8 +88,6 @@ CONFIG_RTC_DRIVER=y
CONFIG_RTC_NALARMS=2
CONFIG_SCHED_LPWORK=y
CONFIG_SIG_DEFAULT=y
-CONFIG_SMP=y
-CONFIG_SMP_NCPUS=2
CONFIG_SPI=y
CONFIG_SPIFFS_NAME_MAX=48
CONFIG_STACK_COLORATION=y
```
--
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] [nuttx-apps] no1wudi commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1103727946
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
In this patch we need to use readline() to replace fgets() but don't want to echo the input on console (like input password),
so can pass a invalid outfd here to disable echo temporal.
The echo of readline will be removed later with echo character in serial driver.
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
In this patch we need to use readline() to replace fgets() but don't want to echo the input on console (like input password), so can pass a invalid outfd here to disable echo temporal.
The echo of readline will be removed later with echo character in serial driver.
--
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] [nuttx-apps] xiaoxiang781216 merged pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1103530397
##########
nshlib/nsh_altconsole.c:
##########
@@ -62,7 +62,7 @@ static int nsh_clone_console(FAR struct console_stdio_s *pstate)
/* Flush stderr: we only flush stderr if we opened the alternative one */
- fflush(stderr);
+ fsync(STDERR_FILENO);
Review Comment:
le's remove fsync call since fflush just flush the buffer at stdio layer, not vfs layer. So, we don't need do anything here after changing file stream to file handle.
##########
nshlib/nsh_altconsole.c:
##########
@@ -85,7 +85,7 @@ static int nsh_clone_console(FAR struct console_stdio_s *pstate)
/* Flush stdout: we only flush stdout if we opened the alternative one */
- fflush(stdout);
+ fsync(STDOUT_FILENO);
Review Comment:
ditto
--
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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1442540354
@no1wudi
>However, the above movies were based on spresense:wifi_smp
The same issue happens with spresense:rndis_smp too.
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1113432158
##########
nshlib/nsh_console.c:
##########
@@ -115,33 +82,22 @@ static int nsh_openifnotopen(struct console_stdio_s *pstate)
*
****************************************************************************/
-#ifdef CONFIG_FILE_STREAM
static void nsh_closeifnotclosed(struct console_stdio_s *pstate)
{
- if (pstate->cn_outstream == OUTSTREAM(pstate))
+ if (OUTFD(pstate) >= 0)
Review Comment:
```suggestion
if (OUTFD(pstate) >= 0 && OUTFD(pstate) != STDOUT_FILENO)
```
##########
nshlib/nsh_session.c:
##########
@@ -194,34 +191,33 @@ int nsh_session(FAR struct console_stdio_s *pstate,
* occurs. Either will cause the session to terminate.
*/
- ret = cle(pstate->cn_line, g_nshprompt, CONFIG_NSH_LINELEN,
- INSTREAM(pstate), OUTSTREAM(pstate));
+ ret = cle_fd(pstate->cn_line, g_nshprompt, CONFIG_NSH_LINELEN,
+ STDIN_FILENO, STDOUT_FILENO);
Review Comment:
```suggestion
INFD(pstate), OUTFD(pstate));
```
##########
nshlib/nsh_session.c:
##########
@@ -194,34 +191,33 @@ int nsh_session(FAR struct console_stdio_s *pstate,
* occurs. Either will cause the session to terminate.
*/
- ret = cle(pstate->cn_line, g_nshprompt, CONFIG_NSH_LINELEN,
- INSTREAM(pstate), OUTSTREAM(pstate));
+ ret = cle_fd(pstate->cn_line, g_nshprompt, CONFIG_NSH_LINELEN,
+ STDIN_FILENO, STDOUT_FILENO);
if (ret < 0)
{
- fprintf(pstate->cn_errstream, g_fmtcmdfailed, "nsh_session",
+ dprintf(ERRFD(pstate), g_fmtcmdfailed, "nsh_session",
"cle", NSH_ERRNO_OF(-ret));
continue;
}
#else
/* Display the prompt string */
- fputs(g_nshprompt, pstate->cn_outstream);
- fflush(pstate->cn_outstream);
+ write(OUTFD(pstate), g_nshprompt, strlen(g_nshprompt));
/* readline() normally returns the number of characters read, but
* will return EOF on end of file or if an error occurs. EOF
* will cause the session to terminate.
*/
- ret = readline(pstate->cn_line, CONFIG_NSH_LINELEN,
- INSTREAM(pstate), OUTSTREAM(pstate));
+ ret = readline_fd(pstate->cn_line, CONFIG_NSH_LINELEN,
+ STDIN_FILENO, STDOUT_FILENO);
Review Comment:
```suggestion
INFD(pstate), OUTFD(pstate));
```
--
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] [nuttx-apps] no1wudi closed pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi closed pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
URL: https://github.com/apache/nuttx-apps/pull/1559
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1104723398
##########
system/readline/readline_fd.c:
##########
@@ -32,19 +32,6 @@
#include "system/readline.h"
#include "readline.h"
-/****************************************************************************
- * Private Type Declarations
- ****************************************************************************/
-
-struct readline_s
Review Comment:
revert, keep in source file
##########
system/readline/readline_common.c:
##########
@@ -494,7 +497,10 @@ ssize_t readline_common(FAR struct rl_common_s *vtbl, FAR char *buf,
/* <esc>[K is the VT100 command that erases to the end of the line. */
#ifdef CONFIG_READLINE_ECHO
- RL_WRITE(vtbl, g_erasetoeol, sizeof(g_erasetoeol));
+ if (rl->outfd >= 0)
Review Comment:
let's move the check to readline_putc/readline_write
##########
nshlib/nsh_telnetlogin.c:
##########
@@ -207,18 +206,17 @@ int nsh_telnetlogin(FAR struct console_stdio_s *pstate)
#ifdef CONFIG_NSH_PLATFORM_CHALLENGE
platform_challenge(challenge, sizeof(challenge));
- fputs(challenge, pstate->cn_outstream);
+ write(OUTFD(pstate), challenge, strlen(challenge));
#endif
/* Ask for the login password */
- fputs(g_passwordprompt, pstate->cn_outstream);
- fflush(pstate->cn_outstream);
+ write(OUTFD(pstate), g_passwordprompt, strlen(g_passwordprompt));
nsh_telnetecho(pstate, TELNET_NOTUSE_ECHO);
password[0] = '\0';
- if (fgets(pstate->cn_line, CONFIG_NSH_LINELEN,
- INSTREAM(pstate)) != NULL)
+ if (readline_fd(pstate->cn_line, CONFIG_NSH_LINELEN,
+ INFD(pstate), OUTFD(pstate)) >= 0)
Review Comment:
align
##########
nshlib/nsh_console.c:
##########
@@ -115,33 +101,21 @@ static int nsh_openifnotopen(struct console_stdio_s *pstate)
*
****************************************************************************/
-#ifdef CONFIG_FILE_STREAM
static void nsh_closeifnotclosed(struct console_stdio_s *pstate)
{
- if (pstate->cn_outstream == OUTSTREAM(pstate))
+ if (OUTFD(pstate) >= 0 && OUTFD(pstate) != STDOUT_FILENO)
Review Comment:
why need check OUTFD(pstate) != STDOUT_FILENO
##########
nshlib/nsh_telnetlogin.c:
##########
@@ -178,21 +178,20 @@ int nsh_telnetlogin(FAR struct console_stdio_s *pstate)
/* Present the NSH Telnet greeting */
- fputs(g_telnetgreeting, pstate->cn_outstream);
- fflush(pstate->cn_outstream);
+ write(OUTFD(pstate), g_telnetgreeting, strlen(g_telnetgreeting));
/* Loop for the configured number of retries */
for (i = 0; i < CONFIG_NSH_LOGIN_FAILCOUNT; i++)
{
/* Ask for the login username */
- fputs(g_userprompt, pstate->cn_outstream);
- fflush(pstate->cn_outstream);
+ write(OUTFD(pstate), g_userprompt, strlen(g_userprompt));
username[0] = '\0';
- if (fgets(pstate->cn_line, CONFIG_NSH_LINELEN,
- INSTREAM(pstate)) != NULL)
+ if (readline_fd(pstate->cn_line, CONFIG_NSH_LINELEN,
+ INFD(pstate), OUTFD(pstate)) >= 0)
Review Comment:
align
##########
nshlib/nsh_telnetlogin.c:
##########
@@ -65,8 +66,7 @@ static void nsh_telnetecho(FAR struct console_stdio_s *pstate,
optbuf[1] = (is_use == TELNET_USE_ECHO) ? TELNET_WILL : TELNET_DO;
optbuf[2] = 1;
optbuf[3] = 0;
- fputs((FAR char *)optbuf, pstate->cn_outstream);
- fflush(pstate->cn_outstream);
+ write(OUTFD(pstate), optbuf, strlen((const char *)optbuf));
Review Comment:
add FAR to the cast
##########
nshlib/nsh_console.c:
##########
@@ -115,33 +101,21 @@ static int nsh_openifnotopen(struct console_stdio_s *pstate)
*
****************************************************************************/
-#ifdef CONFIG_FILE_STREAM
static void nsh_closeifnotclosed(struct console_stdio_s *pstate)
{
- if (pstate->cn_outstream == OUTSTREAM(pstate))
+ if (OUTFD(pstate) >= 0 && OUTFD(pstate) != STDOUT_FILENO)
{
- fflush(OUTSTREAM(pstate));
- pstate->cn_outfd = OUTFD(pstate);
+ close(OUTFD(pstate));
}
- else
+
+ if (ERRFD(pstate) != OUTFD(pstate))
Review Comment:
ERRFD(pstate) >= 0 && ERRFD(pstate) != STDERR_FILENO && ERRFD(pstate) != OUTFD(pstate)
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1103211731
##########
nshlib/nsh.h:
##########
@@ -699,7 +697,7 @@ struct nsh_parser_s
#endif
#ifndef CONFIG_NSH_DISABLESCRIPT
- FILE *np_stream; /* Stream of current script */
+ int np_fd; /* Stream of current script */
Review Comment:
```suggestion
int np_fd; /* Stream of current script */
```
##########
nshlib/nsh_altconsole.c:
##########
@@ -100,23 +100,11 @@ static int nsh_clone_console(FAR struct console_stdio_s *pstate)
/* Setup the stderr */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(pstate->cn_errfd, "a");
- if (!pstate->cn_errstream)
- {
- free(pstate);
- return -EIO;
- }
+ ERRFD(pstate) = 2;
/* Setup the stdout */
- pstate->cn_outfd = 1;
- pstate->cn_outstream = fdopen(pstate->cn_outfd, "a");
- if (!pstate->cn_outstream)
- {
- free(pstate);
- return -EIO;
- }
+ OUTFD(pstate) = 1;
Review Comment:
```suggestion
OUTFD(pstate) = 1;
```
##########
nshlib/nsh_altconsole.c:
##########
@@ -100,23 +100,11 @@ static int nsh_clone_console(FAR struct console_stdio_s *pstate)
/* Setup the stderr */
- pstate->cn_errfd = 2;
- pstate->cn_errstream = fdopen(pstate->cn_errfd, "a");
- if (!pstate->cn_errstream)
- {
- free(pstate);
- return -EIO;
- }
+ ERRFD(pstate) = 2;
Review Comment:
```suggestion
ERRFD(pstate) = 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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1442586062
@no1wudi
The same issue also happens with rv-virt:netnsh_smp (qemu) on ubuntu 18.04 (x86_64)
However, the host PC is fast so the symptom is not worse than the spresense.
--
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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1441228570
@no1wudi
I’m on holiday today so I can not confirm the config now.
However, the above movies were based on spresense:wifi_smp
--
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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1442658039
@no1wudi
As for esp32-devkitc, I found out how to reproduce the issue.
Please try wapi_smp without CONFIG_NET_TCP_WRITE_BUFFERS=y.
You would see the same symptom as spresense:wifi_smp
In this case (i.e. esp32-devkitc:wapi_smp without CONFIG_NET_TCP_WRITE_BUFFERS=y), reverting this PR would not cause the issue.
```
--- a/boards/xtensa/esp32/esp32-devkitc/configs/wapi_smp/defconfig
+++ b/boards/xtensa/esp32/esp32-devkitc/configs/wapi_smp/defconfig
@@ -67,7 +67,6 @@ CONFIG_NET_ICMP=y
CONFIG_NET_ICMP_SOCKET=y
CONFIG_NET_STATISTICS=y
CONFIG_NET_TCP=y
-CONFIG_NET_TCP_WRITE_BUFFERS=y
CONFIG_NET_UDP=y
CONFIG_NFS=y
CONFIG_NSH_ARCHINIT=y
```
--
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] [nuttx-apps] no1wudi commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1442664973
@masayuki2009 Thanks for your information, I'll continue debug it.
--
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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1441572686
@no1wudi
>Could you check the CONFIG_TELNET_TXBUFFER_SIZE value
```
$ grep -i CONFIG_TELNET_TX .config
CONFIG_TELNET_TXBUFFER_SIZE=256
```
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1104766372
##########
nshlib/nsh_console.c:
##########
@@ -88,21 +84,11 @@ static int nsh_openifnotopen(struct console_stdio_s *pstate)
* descriptor may be opened on a different task than the stream.
*/
- if (!pstate->cn_outstream)
- {
- pstate->cn_outstream = fdopen(pstate->cn_outfd, "w");
- if (!pstate->cn_outstream)
- {
- return ERROR;
- }
-
-#if !defined(CONFIG_NSH_ALTCONDEV)
- /* If the alternative console is not enabled then stderr = stdout */
+#ifndef CONFIG_NSH_ALTCONDEV
+ /* If the alternative console is not enabled then stderr = stdout */
- pstate->cn_errfd = pstate->cn_outfd;
- pstate->cn_errstream = pstate->cn_outstream;
+ ERRFD(pstate) = OUTFD(pstate);
Review Comment:
let's remove nsh_openifnotopen and move the initialization to nsh_newconsole
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1103641543
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
why need add the special check here?
##########
system/readline/readline_fd.c:
##########
@@ -117,6 +117,13 @@ static void readline_putc(FAR struct rl_common_s *vtbl, int ch)
DEBUGASSERT(priv);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
ditto
##########
nshlib/nsh_script.c:
##########
@@ -155,8 +152,9 @@ int nsh_script(FAR struct nsh_vtbl_s *vtbl, FAR const FAR char *cmd,
/* Now read the next line from the script file */
- pret = fgets(buffer, CONFIG_NSH_LINELEN, vtbl->np.np_stream);
- if (pret)
+ ret = readline_fd(buffer, CONFIG_NSH_LINELEN, vtbl->np.np_fd, -1);
+
Review Comment:
remove the blank line
##########
nshlib/nsh_console.c:
##########
@@ -462,20 +416,16 @@ FAR struct console_stdio_s *nsh_newconsole(bool isctty)
pstate->cn_vtbl.np.np_flags = NSH_NP_SET_OPTIONS_INIT;
#endif
-#ifdef CONFIG_FILE_STREAM
pstate->cn_vtbl.redirect = nsh_consoleredirect;
pstate->cn_vtbl.undirect = nsh_consoleundirect;
/* Initialize the error stream */
- pstate->cn_errfd = ERRFD(pstate);
- pstate->cn_errstream = ERRSTREAM(pstate);
+ ERRFD(pstate) = STDERR_FILENO;
/* Initialize the output stream */
- pstate->cn_outfd = OUTFD(pstate);
- pstate->cn_outstream = OUTSTREAM(pstate);
-#endif
+ OUTFD(pstate) = STDOUT_FILENO;
Review Comment:
```suggestion
OUTFD(pstate) = STDOUT_FILENO;
```
##########
nshlib/nsh_console.c:
##########
@@ -406,10 +362,8 @@ static void nsh_consoleundirect(FAR struct nsh_vtbl_s *vtbl,
FAR struct serialsave_s *ssave = (FAR struct serialsave_s *)save;
Review Comment:
```suggestion
FAR struct serialsave_s *ssave = (FAR struct serialsave_s *)save;
```
##########
nshlib/nsh_console.c:
##########
@@ -88,21 +84,11 @@ static int nsh_openifnotopen(struct console_stdio_s *pstate)
* descriptor may be opened on a different task than the stream.
*/
- if (!pstate->cn_outstream)
- {
- pstate->cn_outstream = fdopen(pstate->cn_outfd, "w");
- if (!pstate->cn_outstream)
- {
- return ERROR;
- }
-
-#if !defined(CONFIG_NSH_ALTCONDEV)
+#ifndef CONFIG_NSH_ALTCONDEV
/* If the alternative console is not enabled then stderr = stdout */
- pstate->cn_errfd = pstate->cn_outfd;
- pstate->cn_errstream = pstate->cn_outstream;
+ ERRFD(pstate) = OUTFD(pstate);
Review Comment:
```suggestion
ERRFD(pstate) = OUTFD(pstate);
```
##########
nshlib/nsh_parse.c:
##########
@@ -527,11 +523,8 @@ static int nsh_execute(FAR struct nsh_vtbl_s *vtbl,
*/
#ifdef CONFIG_NSH_BUILTIN_APPS
-#ifdef CONFIG_FILE_STREAM
ret = nsh_builtin(vtbl, argv[0], argv, redirfile, oflags);
-#else
- ret = nsh_builtin(vtbl, argv[0], argv, NULL, 0);
-#endif
+
Review Comment:
remove the blank line
##########
nshlib/nsh_script.c:
##########
@@ -155,8 +152,9 @@ int nsh_script(FAR struct nsh_vtbl_s *vtbl, FAR const FAR char *cmd,
/* Now read the next line from the script file */
- pret = fgets(buffer, CONFIG_NSH_LINELEN, vtbl->np.np_stream);
- if (pret)
+ ret = readline_fd(buffer, CONFIG_NSH_LINELEN, vtbl->np.np_fd, -1);
+
+ if (ret)
Review Comment:
```suggestion
if (ret >= 0)
```
##########
nshlib/nsh_script.c:
##########
@@ -171,15 +169,15 @@ int nsh_script(FAR struct nsh_vtbl_s *vtbl, FAR const FAR char *cmd,
ret = nsh_parse(vtbl, buffer);
}
}
- while (pret && (ret == OK || (vtbl->np.np_flags & NSH_PFLAG_IGNORE)));
+ while (ret && (ret == OK || (vtbl->np.np_flags & NSH_PFLAG_IGNORE)));
Review Comment:
```suggestion
while (ret >= 0 || (vtbl->np.np_flags & NSH_PFLAG_IGNORE));
```
##########
nshlib/nsh_console.c:
##########
@@ -462,20 +416,16 @@ FAR struct console_stdio_s *nsh_newconsole(bool isctty)
pstate->cn_vtbl.np.np_flags = NSH_NP_SET_OPTIONS_INIT;
#endif
-#ifdef CONFIG_FILE_STREAM
pstate->cn_vtbl.redirect = nsh_consoleredirect;
pstate->cn_vtbl.undirect = nsh_consoleundirect;
/* Initialize the error stream */
- pstate->cn_errfd = ERRFD(pstate);
- pstate->cn_errstream = ERRSTREAM(pstate);
+ ERRFD(pstate) = STDERR_FILENO;
Review Comment:
```suggestion
ERRFD(pstate) = STDERR_FILENO;
```
--
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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1440117333
@no1wudi @xiaoxiang781216
I noticed that the telnet output is too slow with this PR.
https://user-images.githubusercontent.com/25843920/220650403-e7b9ab4e-521b-4607-97f9-36e4ee6e7248.mov
--
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] [nuttx-apps] masayuki2009 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1440123318
>@no1wudi @xiaoxiang781216
>I noticed that the telnet output is too slow with this PR.
Without this PR.
https://user-images.githubusercontent.com/25843920/220651338-00b5de65-62aa-446e-aabc-28e02931ea8f.mov
--
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] [nuttx-apps] no1wudi commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1442707518
@masayuki2009 The root case is that `write` ops in telnet driver is nearly unbuffered ,so there are many 1 byte write to socket without stdio's buffer , I'll fix it soon.
--
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] [nuttx-apps] no1wudi commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1441113476
> > @no1wudi @xiaoxiang781216
> > I noticed that the telnet output is too slow with this PR.
>
> Without this PR.
Thanks, I'll look at it soon.
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1104469786
##########
system/readline/readline_common.c:
##########
@@ -733,7 +749,11 @@ ssize_t readline_common(FAR struct rl_common_s *vtbl, FAR char *buf,
#ifdef CONFIG_READLINE_ECHO
/* Echo the character to the console */
- RL_PUTC(vtbl, ch);
+ if (rl->outfd > 0)
Review Comment:
zero is valid fd, change all check to >= 0
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1104279879
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
If so, why we check here again?
--
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] [nuttx-apps] no1wudi commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1104339728
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
> Yes, this is already in readline's implementation (if readline's echo support is enabled).
Emm, I mean this change is it.
--
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] [nuttx-apps] xiaoxiang781216 commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1103741528
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
should we move the check to readline/readline_fd? So, we don't need add the check in all caller of readline/readline_fd.
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
should we move the check to readline/readline_fd? So, we don't need add the check in all callers of readline/readline_fd.
--
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] [nuttx-apps] xiaoxiang781216 commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1426304788
CI break is fixed by https://github.com/apache/nuttx/pull/8498
--
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] [nuttx-apps] no1wudi commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1432562707
Split to 2 PR (readline relative and nsh)
--
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] [nuttx-apps] no1wudi commented on a diff in pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#discussion_r1104155231
##########
system/readline/readline_fd.c:
##########
@@ -149,6 +156,13 @@ static void readline_write(FAR struct rl_common_s *vtbl,
FAR struct readline_s *priv = (FAR struct readline_s *)vtbl;
DEBUGASSERT(priv && buffer && buflen > 0);
+ /* If output fd is invalid, return directly. */
+
+ if (priv->outfd < 0)
Review Comment:
Yes, this is already in readline's implementation (if readline's echo support is enabled).
--
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] [nuttx-apps] no1wudi commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1441209287
> @masayuki2009 Which board/config used in your testing ? Could you check the CONFIG_TELNET_TXBUFFER_SIZE value ?
I've tested telnetd on esp32c3-devkit, and I can't reproduce this situation on it.
--
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] [nuttx-apps] no1wudi commented on pull request #1559: nsh: Decouple with CONFIG_FILE_STREAM
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1559:
URL: https://github.com/apache/nuttx-apps/pull/1559#issuecomment-1441137960
@masayuki2009 Which board/config used in your testing ? Could you check the CONFIG_TELNET_TXBUFFER_SIZE value ?
--
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