Skip to content

Full console: suppress new line when echo is disabled#2196

Open
dwld wants to merge 2 commits intoapache:masterfrom
dwld:console_suppress_echo
Open

Full console: suppress new line when echo is disabled#2196
dwld wants to merge 2 commits intoapache:masterfrom
dwld:console_suppress_echo

Conversation

@dwld
Copy link
Copy Markdown
Contributor

@dwld dwld commented Feb 18, 2020

When echo is switched off via console_echo(0), also new line should be suppressed.

dwld added 2 commits February 18, 2020 12:36
When echo is switched off via console_echo(0), also new line should be suppressed.

Signed-off-by: Stefan Diewald <dwld@users.noreply.github.com>
Adaptions to match style guide

Signed-off-by: Stefan Diewald <dwld@users.noreply.github.com>
@apache-mynewt-bot
Copy link
Copy Markdown

Style check summary

No suggestions at this time!

@kasjer
Copy link
Copy Markdown
Contributor

kasjer commented Feb 25, 2020

Hi @dwld, I tell you why this takes so much time with such simple reasonable change.
For a long time console code that was modified by a few people did not really handled few control characters resulting in some echo while console was configured to be silent except for NLIP protocol.
In late 2019 I changed it so not echo for CR LF (and other control characters showed) but one company that uses mynewt was affected since they relayed on this not so obvious behavior.
So 153dfb8 restored echo for CR/LF just as not to break backward compatibility.
I probably should have add conditional code for allowing this echo if someone really needs it, instead of always, which is not something that would be expected from the code.
So that is why no one is rushing to accept this.

@dwld
Copy link
Copy Markdown
Contributor Author

dwld commented Feb 26, 2020

Hi @kasjer, thanks for the explanation. I understand. For our use cases, this behavior breaks the asynchronous communication. Would the introduction of another switch to suppress the echo for CR/LF be an option?

@kasjer
Copy link
Copy Markdown
Contributor

kasjer commented Mar 9, 2020

Hi @dwld quick question, are you using nlip protocol or some other custom protocol and you are not really interested in nlip handling?
If you are not going to use nlip (that seem like reasonable option) that I will add switch to just disable nlip handling and this will reduce code size and runtime cpu usage.
But what I've noticed when looking at you proposal is that console_echo() function that is public api for disabling console echo and it should not be used inside console code to turn on/off echo for other reason as it is done currently. And this also cries for fix.
btw you proposal for call console_echo(echo); would not make any difference and could be just totally dropped to achieve same goal.
Fixes are on the way...

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.

3 participants