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

basic termios symbols support #53

Closed
wants to merge 24 commits into from
Closed

Conversation

jerch
Copy link
Collaborator

@jerch jerch commented Mar 18, 2017

rough proof of concept - do not merge.

Implements a low level interface to the termios data. Can be used in a C-ish style with bitwise operators, see termios_test.js for an example. Needs a better interface...

@jerch jerch mentioned this pull request Mar 19, 2017
@jerch
Copy link
Collaborator Author

jerch commented Mar 19, 2017

@Tyriar Do have an idea why with the last changes all the tests failed? I did not change anything in the C++ code but the native module is missing. Maybe related to the order of creation of the native and the TS module?

@Tyriar
Copy link
Member

Tyriar commented Mar 20, 2017

@jerch Error: Cannot find module '../build/Release/pty.node' means the build fails, check npm install step for details

@jerch
Copy link
Collaborator Author

jerch commented Mar 22, 2017

@Tyriar The new interface has landed. Since I used some C++11 features Travis still has some issues with outdated compilers.

@jerch
Copy link
Collaborator Author

jerch commented Mar 23, 2017

@Tyriar I cant get travis working with OSX, it seems the compiler suite does not support std::unordered_map. I dont know whats going on, maybe the OSX bundles on travis are to old. Any idea how to solve it? The last resort is to drop the unordered_map and all C++11 goodies and go with an older standard. But Im not convinced yet that OSX cant handle a 6 years old C++ standard.

@jerch
Copy link
Collaborator Author

jerch commented Mar 23, 2017

Holy moly getting closer to the real issues - the different platform dependent termios symbols. Travis containers are really outdated or have outdated defaults, it took me like 6h of googling around to get this running on OSX. Not cool :(

@jerch
Copy link
Collaborator Author

jerch commented Mar 23, 2017

@Tyriar Puuh, ready. Feel free to change the TS interface to your needs. The basic interface to C++ land works as following:

  • pty.tcgetattr(fd)
    Returns an object with on|off bit values as Boolean (all flag fields), character settings as strings (c_cc) and c_cc.VMIN and c_cc.VTIME as Number for file descriptor fd. fd is optional, if omitted the function returns an object with everything set to zero.
  • pty.tcsetattr(fd, attrs, action)
    Sets termios settings of fd to any known value from attrs. attrs can be any object, the setter will silently swallow false properties and values. Means any correct property and value in the shape of the returned object of pty.tcgetattr will be applied. Any others will be skipped.
    action must be a string representation of the optional_actions defined by termios.h.
  • termios symbols are gathered at compile time and added to string->value maps. The maps are initialized once upon module loading. Symbol evaluation happens at runtime.

Remarks:

  • I had to change the gyp file and the travis settings to get it working on travis for osx and linux, mainly due to outdated compiler settings of travis. Please have a look there for any possible drawbacks.
  • Windows is not supported yet. Since it is a total different story, I'd have a look into this by a separate PR.
  • I skipped the baud rate interface of termios. If this is an issue, it can be added by a later PR.

@Tyriar Tyriar added this to the 0.7.0 milestone Mar 23, 2017
@Tyriar
Copy link
Member

Tyriar commented Mar 23, 2017

Thanks @jerch, I'll try look at this sometime over the next week.

@jerch
Copy link
Collaborator Author

jerch commented Mar 26, 2017

@Tyriar Hmm thought about the termios thing - maybe it is better to put it into a separate module, that can be imported by node-pty? The termios settings are relevant for both ends of a pty, by separating the functionality it could be used by node apps at the slave side as well. And it would make it better testable in node-pty since it would enable checks from master to slave and vice versa. Once established it could also be used for serial connections in node with real tty devices.
What do you think?

@Tyriar
Copy link
Member

Tyriar commented Mar 26, 2017

@jerch that may be a good idea, why I'm deferring reviewing the PR is the sheer size of the change. There is also the fact that it doesn't apply on Windows (at least yet) which pushes further away from a unified API across Windows/Unix-like.

Maybe separating it further and allowing consumers to pull it in only if they need it? I believe all it needs is the fd right? We could an entry in examples/termios/ that did something like this:

var pty = require('node-pty');
var termios = require('node-termios');

// init terminal using pty...

var attributes = termios.getAttributes(terminal.fd);
// ...

@jerch
Copy link
Collaborator Author

jerch commented Mar 26, 2017

@Tyriar Ok, gonna put it into a separate module. Don't know yet if I'll start from scratch or use https://github.com/Gottox/node-termios as a basis (it is missing some functions).

Not sure either if windows compatibility to some degree can be achieved, most symbols are meaningless for console programs there. I think winpty does not emulate the higher level functions of unix's tty system (like line buffering with ICANON, separate setting of input/output/local/character modes etc.) Maybe @rprichard can tell.

@Tyriar
Copy link
Member

Tyriar commented Mar 26, 2017

node-termios seems to do pretty much everything we wanted already, no?

@jerch
Copy link
Collaborator Author

jerch commented Mar 26, 2017

Almost - it doesnt handle the c_cc settings atm. Also most of the convenient functions defined by termios are missing.

@Tyriar
Copy link
Member

Tyriar commented Mar 30, 2017

@jerch I think I'll close this off as the right direction imo is contributing to node-termios and make sure fd is included in the public API. I added the test fixes in #66, thanks heaps for looking into that

@Tyriar Tyriar closed this Mar 30, 2017
@Tyriar Tyriar removed this from the 0.7.0 milestone Jul 5, 2017
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.

2 participants