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/03/09 08:29:11 UTC
[GitHub] [nuttx-apps] no1wudi opened a new pull request, #1650: system/cle: Enhance error handling for cle_getcursor
no1wudi opened a new pull request, #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650
## Summary
Fix https://github.com/apache/nuttx/issues/8731, and also prevent shell freezing without any information if character lost during cle_getcursor in some case (serial line break? or serial driver is not really ready on startup).
## Impact
cle
## Testing
esp32c3-devkit:usbconsole and lua with 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 a diff in pull request #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1130654745
##########
system/cle/cle.c:
##########
@@ -82,6 +84,14 @@
# define COLOR_OUTPUT VT100_GREEN
#endif
+/* Poll timeout during getcursor,
+ * 50ms should be enough to fetch a single byte from powerful PC (or telent)
+ */
+
+#define CLE_POLL_TIMEOUT 50
+
+#define CLE_GETCURSOR_RETRY_COUNT 5
Review Comment:
Maybe more, a response have about three bytes, will poll fd before get each byte.
So the 1s in your examples should be atleast 1s instead of up to 1s.
--
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] pkarashchenko commented on a diff in pull request #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1130648696
##########
system/cle/cle.c:
##########
@@ -82,6 +84,14 @@
# define COLOR_OUTPUT VT100_GREEN
#endif
+/* Poll timeout during getcursor,
+ * 50ms should be enough to fetch a single byte from powerful PC (or telent)
+ */
+
+#define CLE_POLL_TIMEOUT 50
+
+#define CLE_GETCURSOR_RETRY_COUNT 5
Review Comment:
Maybe 100ms + 10 tries so it will be up to 1s in total?
I'm not sure what the real use case numbers are.
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1131932472
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
but the response is enter by user, you can't expect how fast the user will generate the input.
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1131924268
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
should we fix the usb serial driver instead
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1131933197
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
No, the response here is sended by terminal auto.
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1130683171
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
In fact this issue is not due to ECHO but https://github.com/apache/nuttx-apps/commit/95f32fd018be7a66354942647d9e1c778ad5d488 ,
seems c3's usb-serial driver need more time to get ready. detail please refer to https://github.com/apache/nuttx/issues/8731#issuecomment-1461288702
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1130676745
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
but why the error doesn't happen before ECHO 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] [nuttx-apps] no1wudi commented on pull request #1650: system/cle: Remove cle_getcursor()
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#issuecomment-1463512436
@pkarashchenko @xiaoxiang781216 I updated this PR to another way, the new approach is to maintain the cursor position in
cle internal instead of query from terminal, this avoid the query operation before each line read operation.
Then the `cle`'s behavior is much more like `readline`, and also avoid some issue if the connection between nuttx and PC is not so stable.
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1132008935
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
but user may not open terminal program yet.
--
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] pkarashchenko commented on a diff in pull request #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1132103972
##########
system/cle/cle.c:
##########
@@ -329,6 +338,44 @@ static int cle_getch(FAR struct cle_s *priv)
return buffer;
}
+/****************************************************************************
+ * Name: cle_pollch
+ *
+ * Description:
+ * Get a single character from the console input device.
+ *
+ ****************************************************************************/
+
+static int cle_pollch(FAR struct cle_s *priv, FAR char *ch)
+{
+ int ret;
+ struct pollfd fd;
+
+ /* Preparing pollfd */
+
+ fd.fd = priv->infd;
+ fd.events = POLLIN;
+
+ ret = poll(&fd, 1, CLE_POLL_TIMEOUT);
+
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ if (fd.revents & POLLIN)
+ {
+ ret = read(priv->infd, ch, 1);
+ ret = ret == 1 ? 0 : -EAGAIN;
Review Comment:
why not
```suggestion
ret = ret == 1 ? 0 : ret;
```
?
##########
system/cle/cle.c:
##########
@@ -329,6 +338,44 @@ static int cle_getch(FAR struct cle_s *priv)
return buffer;
}
+/****************************************************************************
+ * Name: cle_pollch
+ *
+ * Description:
+ * Get a single character from the console input device.
+ *
+ ****************************************************************************/
+
+static int cle_pollch(FAR struct cle_s *priv, FAR char *ch)
+{
+ int ret;
+ struct pollfd fd;
+
+ /* Preparing pollfd */
+
+ fd.fd = priv->infd;
+ fd.events = POLLIN;
+
+ ret = poll(&fd, 1, CLE_POLL_TIMEOUT);
+
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ if (fd.revents & POLLIN)
Review Comment:
Optional
```suggestion
if ((fd.revents & POLLIN) != 0)
```
##########
system/cle/cle.c:
##########
@@ -469,16 +517,18 @@ static int cle_getcursor(FAR struct cle_s *priv, FAR uint16_t *prow,
{
/* Look for initial ESC */
- ch = cle_getch(priv);
- if (ch != ASCII_ESC)
+ ret = cle_pollch(priv, &ch);
+
Review Comment:
why do we need extra newlines?
--
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] pkarashchenko commented on a diff in pull request #1650: system/cle: Remove cle_getcursor()
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1137789607
##########
system/cle/cle.c:
##########
@@ -367,21 +363,49 @@ static void cle_cursoroff(FAR struct cle_s *priv)
*
****************************************************************************/
-static void cle_setcursor(FAR struct cle_s *priv, uint16_t column)
+static void cle_setcursor(FAR struct cle_s *priv, int16_t column)
{
char buffer[16];
int len;
+ int off;
- cleinfo("row=%d column=%d offset=%d\n", priv->row, column, priv->coloffs);
+ /* Sub prompt offset from real postion to get correct offset to execute */
- /* Format the cursor position command. The origin is (1,1). */
+ off = column - (priv->realpos - priv->coloffs);
- len = snprintf(buffer, 16, g_fmtcursorpos,
- priv->row, column + priv->coloffs);
+ cleinfo("column=%d offset=%d\n", column, off);
+
+ /* If cursor not move, retrun directly */
+
+ if (off == 0)
+ {
+ return;
+ }
+
+ /* If position after adjustment is belong to promot area,
+ * limit it to edge of the prompt.
+ */
+
+ if (off + priv->realpos < priv->coloffs)
+ {
+ off = priv->realpos - priv->coloffs;
+ }
+
+ /* Format the cursor position command.
+ * Move left or right depends on the current cursor position in buffer.
+ */
+
+ len = snprintf(buffer, 16,
Review Comment:
```suggestion
len = snprintf(buffer, sizeof(buffer),
```
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1132008935
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
but user may not open terminal yet
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1131927901
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
I think usb driver should be fixed, but this patch is also needed since nsh shold not hang here if can't read vt100 command response, should raise some error info atleast.
--
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 #1650: system/cle: Enhance error handling for cle_getcursor
Posted by "no1wudi (via GitHub)" <gi...@apache.org>.
no1wudi commented on code in PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650#discussion_r1131927901
##########
system/cle/cle.c:
##########
@@ -1124,11 +1149,20 @@ int cle_fd(FAR char *line, FAR const char *prompt, uint16_t linelen,
/* Get the current cursor position */
- ret = cle_getcursor(&priv, &priv.row, &column);
-
- if (ret < 0)
+ for (; ; )
{
- return ret;
+ ret = cle_getcursor(&priv, &priv.row, &column);
Review Comment:
I think usb driver should be fixed, but this patch is also needed since nsh shold not hang here if can't read vt100 command respons.
--
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] acassis merged pull request #1650: system/cle: Remove cle_getcursor()
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis merged PR #1650:
URL: https://github.com/apache/nuttx-apps/pull/1650
--
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