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

Allow enabling flow control also inside the shell #7704

Closed
depau opened this issue Feb 10, 2021 · 15 comments
Closed

Allow enabling flow control also inside the shell #7704

depau opened this issue Feb 10, 2021 · 15 comments

Comments

@depau
Copy link

depau commented Feb 10, 2021

  • fish, version 3.1.2
  • Linux frigo 5.10.9-arch1-1 Need a history built-in #1 SMP PREEMPT Tue, 19 Jan 2021 22:06:06 +0000 x86_64 GNU/Linux
  • sh -c 'env HOME=$(mktemp -d) fish' - yep, it still does it

When running on a serial console which I explicitly configured with ixon and ixoff, fish keeps disabling said options every time a new prompt is printed.

I understand that users might think their system is locked up if they press Ctrl+S by mistake and the terminal stops responding, however there are cases in which one may need software flow control.

Is it possible to disable this behavior?

Reproducing

  • Run fish in one terminal
  • Run tty to find the tty/pty it's attached to
  • In another terminal, run sudo stty -F <tty found above> ixon ixoff to enable software flow control
  • Run sudo stty -a -F <tty found above> to verify that ixon and ixoff have indeed been enabled
  • Go back to the initial fish terminal and press enter
  • Run sudo stty -a -F <tty found above> to verify that ixon and ixoff have been disabled by fish
  • Do the same thing with another shell, such as bash, to verify that it respects the user's decision

(using another terminal makes it easier to demonstrate the issue since otherwise fish resets the terminal right after stty is run)

(to whoever thought "who uses software flow control intentionally in 2021 anyway?", I do 😜)

@faho
Copy link
Member

faho commented Feb 10, 2021

Duplicate of #2315

@faho faho marked this as a duplicate of #2315 Feb 10, 2021
@faho faho closed this as completed Feb 10, 2021
@faho faho added the duplicate label Feb 10, 2021
@faho
Copy link
Member

faho commented Feb 10, 2021

Short answer: The next release will sanitize the terminal modes instead of just resetting them all, this allows you to enable flow control. It remains off by default.

@depau
Copy link
Author

depau commented Feb 10, 2021

(replying here because the other issue is locked)

Seeing that 3.2.0 looks close to being released according to the number of open issues in the GH milestone, I tested this on the latest git (2bab31a as I'm writing this) and it still disables ixon/ixoff, even if I set them before starting fish.

I see that it does restore the terminal to how it found it on exit, however my application needs flow control enabled *while* fish is running.

Does the new behavior you mentioned need to be enabled explicitly?

@faho
Copy link
Member

faho commented Feb 10, 2021

I see that it does restore the terminal to how it found it on exit, however my application needs flow control enabled while fish is running.

Ah, okay. Yeah, that's not supported.

Like I said, fish sanitizes the modes still, and flow control doesn't seem very useful inside the shell - it's already waiting for input, why do you want it stopped?

What are you using this for?

@depau
Copy link
Author

depau commented Feb 10, 2021

What are you using this for?

First of all, I'm sorry 😂 I know this is a very niche use-case.

I'm porting ttyd (https://github.com/tsl0922/ttyd/) to ESP8266. It My port exposes the UART console over WebSockets, with the use-case being having a cheap device that can be plugged into an SBC for remote debugging of boot and network connection issues, one that doesn't force you to shut down the device and move it somewhere more comfortable to work on with a USB UART adapter.

One of my goals is supporting relatively high baud rates such as 1500000, which is the default on many RockChip-based platforms.

It works fine for the most part, however with bulky streams of data (such as zmodem, s-tui or... nyancat) the transmission is bottlenecked by Wi-Fi/TCP - while the 802.11n rate is technically higher than 1500000bps it's still an ESP8266. The result is that when the WebSocket frame queue is full, some frames are dropped and the terminal gets garbled (or the zmodem transfer breaks).

This is very reliably solved with flow control - when the queue is full I keep buffering the incoming data from UART and send XOFF, then send XON when the internal UART buffer is almost empty.

Normal, low-bandwidth shell usage is fine, with or without fish and with or without flow control. However even ls-ing a huge directory or dumping the kernel log without flow control can easily fill up the WS queue at high baud rates.

@ridiculousfish
Copy link
Member

Flow control was initially disabled in #814. FWIW zsh supports this with setopt noflowcontrol.

For the case of ls or cat, then in fish 3.2 you can use stty ixon to enable flow control and it will stick. Maybe we should stop defaulting flow control off for external commands. Would that address this issue?

@faho
Copy link
Member

faho commented Feb 11, 2021

As I read it, the problem is that you don't know what is currently in control of the terminal when you have to make it slow down to catch up, so this does indeed need fish to stop as well.

Maybe we should stop defaulting flow control off for external commands.

Flow control is a horrible default, and if this needs it to stick for fish as well, that won't help.

How about we just turn off flow control on start (especially as we have important bindings for ctrl-s) and keep whatever setting comes later, i.e. copy it to the shell-modes from whatever modes the command ends with?

@depau
Copy link
Author

depau commented Feb 11, 2021

As I read it, the problem is that you don't know what is currently in control of the terminal when you have to make it slow down to catch up, so this does indeed need fish to stop as well.

Yes, exactly. ttyd does not have this issue since it uses pseudo-terminals: for what it's worth it can just stop reading and the pty driver will do its job. UART however needs the device on the other end to ask nicely.

Maybe we should stop defaulting flow control off for external commands.

Flow control is a horrible default, and if this needs it to stick for fish as well, that won't help.

How about we just turn off flow control on start (especially as we have important bindings for ctrl-s) and keep whatever setting comes later, i.e. copy it to the shell-modes from whatever modes the command ends with?

I was going to say the following:

The best option for me would be to just leave the flow control settings alone until a configuration option is read: fish doesn't change anything on start, then it verifies that some config variable is or is not present and decides how to behave from then on.

But then I realized that fish doesn't print anything until config.fish is read, so your solution works too. I could check what fish is attached to in config.fish and stty if it's a UART tty.

Overall, for my use-case, anything works as long as while fish is running it uses flow control settings that the user can configure, and as long as the implemented solution doesn't cause it to write to the terminal before ensuring the user hasn't configured different terminal settings.

@depau
Copy link
Author

depau commented Feb 11, 2021

as long as the implemented solution doesn't cause it to write to the terminal before ensuring the user hasn't configured different terminal settings.

I mean, if I can add something like this to the beginning of config.fish, it works for me:

string match -rq '/dev/tty[A-Za-z]+\d*' (tty) && stty ixon ixoff

@zanchey
Copy link
Member

zanchey commented Feb 11, 2021

FWIW (I think we should implement these changes anyway) when I was doing serial stuff many moons ago, we found hardware flow control to be a lot more reliable than software flow control. We did have a fair bit of control over the hardware being used though, including wiring our own cables a lot of the time.

@faho faho reopened this Feb 11, 2021
faho added a commit to faho/fish-shell that referenced this issue Feb 11, 2021
Since, unlike e.g. OPOST, this can sometimes be useful, just copy
whatever flow control settings the terminal ends up with.

We still *default* flow control to off (because it's an awful default
and allows us to bind ctrl-s), but if the user decides to enable it so
be it.

Note that it's _possible_ flow control ends up enabled accidentally, I
doubt this happens much and it won't render the shell unusable (and
good terminals might even tell you you've stopped the app).

Fixes fish-shell#7704
@faho faho changed the title Fish messes with UART software flow control Allow enabling flow control also inside the shell Feb 11, 2021
@depau
Copy link
Author

depau commented Feb 11, 2021

we found hardware flow control to be a lot more reliable than software flow control. We did have a fair bit of control over the hardware being used though, including wiring our own cables a lot of the time.

I agree, it would be a lot more reliable. However (for my particular use-case), while I expect that hardware flow control is technically feasible in most SBCs, it is not something that is normally exposed in the GPIO headers, or something that is properly configured in the device tree.

On top of that I also plan to use this on consumer routers, which normally have a 4-pin UART header, if any.

On the other hand software flow control is fairly easy to enable if needed and it works very well: I'm using ~1/8 of the ESP RAM as a UART buffer and I send XOFF quite early, so there's a very good chance that any data sent before XOFF is acknowledged will fit in the UART buffer while the WebSocket frame buffers are sent.

@faho
Copy link
Member

faho commented Feb 11, 2021

@depau Try #7708.

@depau
Copy link
Author

depau commented Feb 11, 2021

It works, thank you

@depau
Copy link
Author

depau commented Feb 11, 2021

Okay, with your changes it sticks with flow control if I stty ixon ixoff from within it.

If I set it from another shell, then run fish, it disables it and that's fine, as discussed.

However, if I then leave fish, regardless of the initial flow control state and regardless of whether I changed it while it was running, it will disable it.

Is it intended?

https://asciinema.org/a/f5WtDYP8yhLCkswppYGcOmjXL

@faho
Copy link
Member

faho commented Feb 11, 2021

Ah, no, that's my mistake - fish restores the terminal modes to whatever it found on startup before it exits, and I took a shortcut and modified those modes.

Fix incoming!

(note: This is the sort of thing that fits better as a comment on the PR)

faho added a commit to faho/fish-shell that referenced this issue Feb 12, 2021
Since, unlike e.g. OPOST, this can sometimes be useful, just copy
whatever flow control settings the terminal ends up with.

We still *default* flow control to off (because it's an awful default
and allows us to bind ctrl-s), but if the user decides to enable it so
be it.

Note that it's _possible_ flow control ends up enabled accidentally, I
doubt this happens much and it won't render the shell unusable (and
good terminals might even tell you you've stopped the app).

Fixes fish-shell#7704
faho added a commit to faho/fish-shell that referenced this issue Feb 14, 2021
Since, unlike e.g. OPOST, this can sometimes be useful, just copy
whatever flow control settings the terminal ends up with.

We still *default* flow control to off (because it's an awful default
and allows us to bind ctrl-s), but if the user decides to enable it so
be it.

Note that it's _possible_ flow control ends up enabled accidentally, I
doubt this happens much and it won't render the shell unusable (and
good terminals might even tell you you've stopped the app).

Fixes fish-shell#7704
@faho faho closed this as completed in 2be720b Feb 15, 2021
@zanchey zanchey added this to the fish 3.2.0 milestone Feb 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants