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

Fixed inconsistent CR/LF #9657

Merged
merged 3 commits into from Apr 1, 2020
Merged

Fixed inconsistent CR/LF #9657

merged 3 commits into from Apr 1, 2020

Conversation

MadQ
Copy link
Contributor

@MadQ MadQ commented Mar 30, 2020

  • 2 lines used "\n\r" instead of "\r\n". Consistent use of "\r\n" will allow the Configurator CLI code to be simplified.

  • This PR is in preparation for some changes and enhancements in the Configurator CLI that I am currently working on.

  • The Configurator CLI code currently has code that is interned to treat carriage returns and line feeds differently, depending on the CR/LF semantics of the operating system. This is actually irrelevant because the firmware CLI code is entirely agnostic about those semantics, and also treats CR and LF identically when processing input characters.

  • This change makes it easier for the Configurator CLI to process serial reads in chunks, instead character by character, which dramatically reduces CPU usage. On the very old laptop I am currently using, a dump all can take up to 20 - 30 seconds, but is near instantaneous when processing serials reads in chunks.

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!!

@McGiverGim
Copy link
Member

The only strange thing is that we have three commits for that. Are you able to squash and update the PR?

Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Never do the line feed before the carriage return - somebody who has practical experience in why ;-)

@mikeller mikeller added this to the 4.2 milestone Mar 31, 2020
@mikeller mikeller modified the milestones: 4.2, 4.1.6 Mar 31, 2020
@mikeller
Copy link
Member

@MadQ:

The Configurator CLI code currently has code that is interned to treat carriage returns and line feeds differently, depending on the CR/LF semantics of the operating system.

This was done way back when, before there were buttons to save / load the contents of the CLI, to make it possible for users of different operating systems to copy / paste CLI contents without getting them garbled.

These days we could probably do away with it, and do the translation to / from the native line break style in the save / load implementations.

@MadQ
Copy link
Contributor Author

MadQ commented Mar 31, 2020

The only strange thing is that we have three commits for that. Are you able to squash and update the PR?

Yes, I can. As soon as I figure out how. This is my first GitHub PR ever. I swear this this kind of stuff was easier to learn when I was 20 years younger.

@MadQ
Copy link
Contributor Author

MadQ commented Mar 31, 2020

The Configurator CLI code currently has code that is interned to treat carriage returns and line feeds differently, depending on the CR/LF semantics of the operating system.

This was done way back when, before there were buttons to save / load the contents of the CLI, to make it possible for users of different operating systems to copy / paste CLI contents without getting them garbled.

These days we could probably do away with it, and do the translation to / from the native line break style in the save / load implementations.

Correct. The Configurator CLI already translates "\r\n" to <br> tags, which the DOM innerText property of the <pre> element (used for copy and save) handles correctly. I.e. the <br>s are converted to the OS's flavor of line breaks (AFAIK).

(Just bringing this up for discussion) So that also begs the question... does anyone still use a direct serial connection or telnet to the FC instead of using Configurator? I'm guessing the answer might be "Yes", but if not, we could probably do away with the VT stuff, like the handling of backspaces, <CTRL>+D, and VT sequences to clear screen/line, etc., and simplify the auto-completion on TAB.
The only other application I have used is an Android app on my phone, but it only uses line-mode, and is not really a VT.

On second thought, this might be opening too big a can of worms.

@mikeller
Copy link
Member

(Just bringing this up for discussion) So that also begs the question... does anyone still use a direct serial connection or telnet to the FC instead of using Configurator? I'm guessing the answer might be "Yes", but if not, we could probably do away with the VT stuff, like the handling of backspaces, +D, and VT sequences to clear screen/line, etc., and simplify the auto-completion on TAB.
The only other application I have used is an Android app on my phone, but it only uses line-mode, and is not really a VT.

On second thought, this might be opening too big a can of worms.

Definitely can of worms. One use case to consider is the one of developers working on the firmware - most of them (myself included) will have configurator side autocompletion disabled, as it does not play well with new commands or ones added temporarily for debugging, and the auto-detection run when opening the CLI tab often interferes with testing. So removing all of the smarts from the firmware and expecting the CLI to fill in will most likely make the lives of this group harder.

@mikeller mikeller merged commit 1b5b061 into betaflight:master Apr 1, 2020
mikeller added a commit that referenced this pull request Apr 1, 2020
Fixed inconsistent CR/LF
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants