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

Add support for the DECREQTPARM report #7852

Closed
j4james opened this issue Oct 7, 2020 · 2 comments · Fixed by #7939
Closed

Add support for the DECREQTPARM report #7852

j4james opened this issue Oct 7, 2020 · 2 comments · Fixed by #7939
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Oct 7, 2020

Description of the new feature/enhancement

DECREQTPARM was a sequence used on the VT100 to report the serial communication parameters - things like baud rate, parity, etc. There's absolute no use for it nowadays, and I don't think it was even supported on later DEC terminals, but it seems to be quite widely implemented by modern terminal emulators. This includes XTerm (in VT100 mode), VTE (since around v0.53.0 I think), Konsole, Urxvt, Mintty, and many others.

I have no idea why everyone supports it, but I'd like for us to do so too. It's an easy addition, and If nothing else, it lets us pass another test in Vttest.

Proposed technical implementation details (optional)

It's fairly straightforward to implement. Everyone just returns a hard coded response (technically two responses, since the first response value depends on the request parameter). I've got a PR ready to go if you're happy to include it, but it's branched from PR #7799, so I'm waiting for that to merge first.

@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Oct 7, 2020
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Oct 7, 2020
@DHowett
Copy link
Member

DHowett commented Oct 7, 2020

Happy indeed to include it. 😄

@DHowett DHowett added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 7, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 7, 2020
@DHowett DHowett added this to the Console Backlog milestone Oct 7, 2020
@ghost ghost added the In-PR This issue has a related PR label Oct 15, 2020
DHowett pushed a commit that referenced this issue Oct 15, 2020
This PR adds support for the `DECREQTPARM` (Request Terminal Parameters)
escape sequence, which was originally used on the VT100 terminal to
report the serial communication parameters. Modern terminal emulators
simply hardcode the reported values for backward compatibility.

The `DECREQTPARM` sequence has one parameter, which was originally used
to tell the terminal whether it was permitted to send unsolicited
reports or not. However, since we have no reason to send an unsolicited
report, we don't need to keep track of that state, but the permission
parameter does still determine the value of the first parameter in the
response.

The response parameters are as follows:

| Parameter        | Value  | Meaning                  |
| ---------------- | ------ | ------------------------ |
| response type    | 2 or 3 | unsolicited or solicited |
| parity           | 1      | no parity                |
| data bits        | 1      | 8 bits per character     |
| transmit speed   | 128    | 38400 baud               |
| receive speed    | 128    | 38400 baud               |
| clock multiplier | 1      |                          |
| flags            | 0      |                          |

There is some variation in the baud rate reported by modern terminal
emulators, and 9600 baud seems to be a little more common than 38400
baud, but I thought the higher speed was probably more appropriate,
especially since that's also the value reported by XTerm.

## Validation Steps Performed

I've added a couple of adapter and output engine tests to verify that
the sequence is dispatched correctly, and the expected responses are
generated. I've also manually tested in Vttest and confirmed that we now
pass the `DECREQTPARM` test in the _Test of terminal reports_.

Closes #7852
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Oct 15, 2020
DHowett pushed a commit that referenced this issue Oct 19, 2020
This PR adds support for the `DECREQTPARM` (Request Terminal Parameters)
escape sequence, which was originally used on the VT100 terminal to
report the serial communication parameters. Modern terminal emulators
simply hardcode the reported values for backward compatibility.

The `DECREQTPARM` sequence has one parameter, which was originally used
to tell the terminal whether it was permitted to send unsolicited
reports or not. However, since we have no reason to send an unsolicited
report, we don't need to keep track of that state, but the permission
parameter does still determine the value of the first parameter in the
response.

The response parameters are as follows:

| Parameter        | Value  | Meaning                  |
| ---------------- | ------ | ------------------------ |
| response type    | 2 or 3 | unsolicited or solicited |
| parity           | 1      | no parity                |
| data bits        | 1      | 8 bits per character     |
| transmit speed   | 128    | 38400 baud               |
| receive speed    | 128    | 38400 baud               |
| clock multiplier | 1      |                          |
| flags            | 0      |                          |

There is some variation in the baud rate reported by modern terminal
emulators, and 9600 baud seems to be a little more common than 38400
baud, but I thought the higher speed was probably more appropriate,
especially since that's also the value reported by XTerm.

## Validation Steps Performed

I've added a couple of adapter and output engine tests to verify that
the sequence is dispatched correctly, and the expected responses are
generated. I've also manually tested in Vttest and confirmed that we now
pass the `DECREQTPARM` test in the _Test of terminal reports_.

Closes #7852

(cherry picked from commit 30e363e)
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7939, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants