Code tidying; emulator support; Python REPL; 122KEY_EN fixes/enhancements; terminfo; new key map; login/shell parameter

[dcoshea]

Apologies, this is a collection of lots of different things. I hope that you find this preferable to lots of separate pull requests! I'm happy to split it into separate pull requests with fewer commits if you'd prefer, and just have one such pull request open at a time. The commits are split out into separate topics and are probably best reviewed individually:

I made this change since - as noted in the commit message - I'd like to find out if the parameter can be removed in the future:

bb963a5 Assert that transmitCommandOrPoll()'s destination is redundant

These are fairly minor things which I think make the code easier to understand, but I realise that's very subjective. I think these sorts of changes will also help toward an eventual goal of splitting the VT52 emulation from the code that communicates with the converter:

a88f62e Clarify the code by removing the global variable m
2f1fe05 Extract decoding methods from VT52_to_5250

This is a minor change which is required for the emulation code I'm working on, although if you'd prefer to address this some other way, e.g. by having a command-line option to disable the ioctl() call rather than just logging a message which might be easy for someone to miss, I'm happy to change it:

53a7b79 Print a message but continue on failure of TIOCMBIS ioctl()

This is a minor enhancement:

8635e89 Add CLI command to enter Python interpreter/REPL

I think the commits related to the 122KEY_EN scancode map are all either well-justified fixes which make the key map more "normal" by PC standards, or enhancements (such as Alt-letter and function key handling) which shouldn't affect existing users.

This commit has some justification in the commit message, and I wanted to do it before changing the way -l is handled in a subsequent commit:

b370fb1 Factor out command-line parsing and use argparse

Here is what the usage information looks like with all of these commits as a result of using argparse:

$ ../5250_usb_converter-venv/bin/python3 5250_terminal.py --some-invalid-param
usage: 5250_terminal.py [-h] [-c] [-i] [-k] [-s] [-t TTY-FILE] [-l PATH] [-u] [-p] [-d] [TERM-DEFINITION [TERM-DEFINITION ...]]
5250_terminal.py: error: unrecognized arguments: --some-invalid-param
$ ../5250_usb_converter-venv/bin/python3 5250_terminal.py --help
usage: 5250_terminal.py [-h] [-c] [-i] [-k] [-s] [-t TTY-FILE] [-l PATH] [-u] [-p] [-d] [TERM-DEFINITION [TERM-DEFINITION ...]]

positional arguments:
  TERM-DEFINITION       Terminal definition(s) of the form
                        STATION_ADDRESS[:[SCANCODE_DICT][:[SLOW_POLL][:[EBCDIC_CODEPAGE][:[EXTRA_FEATURES]]]]]

optional arguments:
  -h, --help            show this help message and exit
  -c                    Enable extra connection debugging (default: disabled)
  -i                    Enable debugging of input/output from terminal (default: disabled)
  -k                    Enable keystroke scancode debug (default: disabled)
  -s                    Disable keyboard clicker (default: enabled)
  -t TTY-FILE           Use the given TTY device file (default: /dev/ttyACM0)
  -l PATH, --login PATH
                        Login/shell executable (default: etc/twinax_login_default)
  -u                    Enable Unix Domain Socket (default: disabled)
  -p                    Enable Telnet Service at port 5251 (default: disabled)
  -d                    Enable daemon mode (default: disabled)

These should be nice enhancements that shouldn't break anything for existing users, except that a value will need to be passed for the -l option now:

deb07c0 Add custom terminfo file and 122KEY_EN_CUSTOM scancode mapping
354f2e4 Accept a login/shell path via -l parameter; add default and example scripts

[inmbolmie]

In general that is a lot of improvement indeed. The project will benefit from some properly coded stuff like this so many thanks. The main thing here is making sure that all the command line parameters are working properly.

[dcoshea]

Thanks very much for looking at this!

Hi, Still don't know why but "ultra slow poll" doesn't seem to be working.

Oops, I see what is going on here:

def parseTermDef(arg):
    ...
    if len(termdef) > 2 and termdef[2]!="":
        ...
        if int(termdef[2]) == 2:
            # Ultra-slow poll MODE
            SLOW_POLL_MICROSECONDS = ULTRA_SLOW_POLL_MICROSECONDS

I think this worked fine when it was in the global scope, but now that I've moved it into a function, I think it's instead setting a local variable in parseTermDef() called SLOW_POLL_MICROSECONDS which shadows the global definition but then goes out of scope.

If that's correct, then adding a global SLOW_POLL_MICROSECONDS statement should fix that, but I'd prefer to avoid having a global variable that affects the behavior between terminals if possible - it looks like currently if you set one terminal to ultra slow polling, then if any other terminals are set for slow (but not ultra slow) polling, they'll also get ultra slow polling.

I think I could fix that without too much trouble, but is that valid - is there any reason that the polling speed for one terminal needs to affect the other because of the communication over the wire? I assume not, but thought I should check.

[inmbolmie]

Thanks very much for looking at this!

Hi, Still don't know why but "ultra slow poll" doesn't seem to be working.

Oops, I see what is going on here: [..] I think I could fix that without too much trouble, but is that valid - is there any reason that the polling speed for one terminal needs to affect the other because of the communication over the wire? I assume not, but thought I should check.

You are right, that is the reason. I then suppose that the polling interval will have to be a parameter when instantiating the terminal object like the others (address, dictionary etc) Maybe instead of returning a boolean the slowPoll parameter could be zero if disabled and the polling interval in microseconds if enabled. Like instead of using SLOW_POLL_MICROSECONDS use terminal.getLowSpeedPollingInterval() or something like that.

I think there is no particular reason not to have different polling rates per terminal other than the "ultra slow poll" is only intended for debugging a terminal and makes not much sense to have several terminals hooked when using that.

In fact it would be useful to be able to specify different polling rates for then different connected terminals as the "optimal" value seems to change from terminal to terminal. It could be like, if you put 0 or omit the parameter if is the default value, if you put 1 it is the default "slow interval", if you put 2 is is the default "ultra slow" and if you put anything else bigger than some threshold (something like 100 microseconds?) it will be used as the polling interval itself.

Thanks and best regards.

[dcoshea]

Thanks for confirming my understanding and for the additional explanation! I will start working on that fix now. Your solution sounds good, but I have a question about this part:

if you put anything else bigger than some threshold (something like 100 microseconds?) it will be used as the polling interval itself

Is it actually worth having a minimum threshold, when (if I understand correctly) not specifying slow polling means that there isn't any minimum interval between polls, i.e. polling occurs as frequently as possible?

I think one good reason to have a minimum is to try to avoid confusion between the magic values 1 and 2 and the values in microseconds, but I think a better solution for that would be to require a us suffix on values in microsecond, so 1 means "slow", 2 means "ultra slow", and for example 10000us gives you half the frequency of "slow". Does that sound okay? A possible future (or immediate) enhancement would be to also accept ms and s suffixes.

I suppose that I should update documentation, usage messages, etc. to make it clear that whatever value is provided is the minimum interval between polls, and that polls may occur less frequently than specified based on CPU load, how quickly devices respond, etc.? In that case it might be a bit clearer that using very small values isn't going to necessarily be all that useful, without having to come up with a particular minimum value that is accepted and then having to justify it.

[inmbolmie]

Yes, your solution of tagging the microsecond value with "us" or whatever time unit is fine. I wouldn't put too much effort trying to explain it on the documentation though as I think that anyone using this functionality will be knowledgeable enough to understand what's going on. Anyway using too low values is essentially the same as not using any value.

[dcoshea]

I force-pushed/rewrote the branch to replace the erroneous commit with the new one e8a0344 which has the summary "Factor out command-line parsing; use argparse; accept poll delay in us" and an updated commit message. The two commits following it (toward HEAD) obviously have new hashes now, but I didn't rewrite them.

[inmbolmie]

Many thanks, looking very nice now. I have merged it.