Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Fix console echo handling #2233

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kasjer
Copy link
Contributor

@kasjer kasjer commented Mar 9, 2020

  1. Echo could be turned on after some nlip bytes handling, now if user code disabled echo with console_echo(0) it will not be enabled in nlip code.

  2. Additionally nlip handling can be disabled to reduce code size.

  3. When application calls console_echo(0), console will not insert anything to the output by itself (echo, prompt, completion)

@kasjer
Copy link
Contributor Author

kasjer commented Mar 9, 2020

Hi @dwld could you please checked if this solution fixes problem that you detected?

@kasjer kasjer force-pushed the kasjer/fix-console-echo-handling branch from 3c88a94 to d029f1d Compare March 9, 2020 14:18
@utzig
Copy link
Member

utzig commented Mar 9, 2020

         default:
             insert_char(&input->line[cur], byte, end);
-            /* Falls through. */
+        /* Falls through. */
         case '\r':
-            /* Falls through. */
+        /* Falls through. */
         case '\n':

Funny one, the style checker does not know what "fall through" means and thinks that your comment applies to the next case!

@dwld
Copy link
Contributor

dwld commented Mar 13, 2020

Hi @dwld could you please checked if this solution fixes problem that you detected?

Hi @kasjer, thanks for your work. In my current project, I do not require NLIP. I just tested this PR with CONSOLE_ECHO: 0 and CONSOLE_NLIP: 0. Unfortunately, the new line is still there, since it is printed by lines 1168 and 1169 in console.c (console_filter_out('\r'); and console_filter_out('\n');).

Maybe we could add an option like your introduced CONSOLE_NLIP_ECHO_LF for NLIP to enable/disable (whatever is more reasonable) the new line echo in non-NLIP mode.

@kasjer kasjer force-pushed the kasjer/fix-console-echo-handling branch from d029f1d to 1a0809d Compare March 16, 2020 13:48
@kasjer kasjer changed the title Fix console echo handling [RFC] Fix console echo handling Mar 16, 2020
kasjer added 4 commits March 16, 2020 15:44
Add syscfg switch that allows builds with NLIP code disabled.
It should reduce console code size if NLIP is not needed.
Function console_echo(1) was called in some cases during
nlip handling. This resulted in console echo being turned on
while user code could turn it off on purpose before.

Now console echo will not turned of if user setting/code did
not wanted to have echo enabled.
Code was echoing back LF character even when application
requested to silence non-NLIP data on input.
Now this behaviour is still possible but require explicit
setting CONSOLE_NLIP_ECHO_LF which is disabled by default.
Prompt is output on console that did not originated in application code.
This is similar to echo characters that goes out of console outside
of application decision.

This disable printing prompt when echo is turned off so console
code does not add anything to the output.
@kasjer kasjer force-pushed the kasjer/fix-console-echo-handling branch from 1a0809d to 5abaebe Compare March 16, 2020 14:50
@apache-mynewt-bot
Copy link

Style check summary

Our coding style is here!

sys/console/minimal/src/console.c

@@ -323,7 +323,7 @@
             ev = NULL;
             console_is_interactive = echo;
             return 0;
-        /* Ignore characters if there's no more buffer space */
+            /* Ignore characters if there's no more buffer space */
         } else if (byte == CONSOLE_NLIP_PKT_START2) {
             /* Disable echo to not flood the UART */
             console_is_interactive = 0;
@@ -352,9 +352,9 @@
 #endif
         default:
             insert_char(&input->line[cur], byte, end);
-            /* Falls through. */
+        /* Falls through. */
         case '\r':
-            /* Falls through. */
+        /* Falls through. */
         case '\n':
             if (byte == '\n' && prev_endl == '\r') {
                 /* handle double end line chars */

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.

4 participants