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

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

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

Conversation

dwld
Copy link
Contributor

@dwld dwld commented Feb 18, 2020

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

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

Signed-off-by: Stefan Diewald <[email protected]>
Adaptions to match style guide

Signed-off-by: Stefan Diewald <[email protected]>
@apache-mynewt-bot
Copy link

Style check summary

No suggestions at this time!

@kasjer
Copy link
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
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
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