mnaberez / py65

Emulate 6502-based microcomputer systems in Python
BSD 3-Clause "New" or "Revised" License
234 stars 68 forks source link

Proposed fix for #47 Save and restore termios settings with helper functions in utils.console #48

Closed SamCoVT closed 4 years ago

SamCoVT commented 6 years ago

After playing around with this for a while, I believe the best location to change the termios settings for linux (should also work for other posix environments like OSX and cygwin, but I can't test that as I only have Linux) is in Monitor._run. It's only when the 65C02 software is actually being run that we want non-echoed input. I also changed the method of getting non-blocking reads to the standard method used when using termios (rather than using fcntl), which is to set the minimum number of characters to zero. I also set the time to wait for a character to 1/10th of a second (the minimum time available using termios) to stop it from using 100% CPU while waiting for a character. This is the same amount of time that was being used by the original select() method. Like select(), it will return immediately if data is available before the timeout.

With this change, I can now paste large input into py65mon's terminal window and it will all be passed to the software running inside py65mon. If CTRL-C is used to get back to the monitor, the normal input is restored.

I don't believe this will change mlauke's issue #46, except that it might move the error to monitor.py instead. I was not able to reproduce that error at all, so I can't say for sure.

mnaberez commented 6 years ago

From the failed build:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I tried restarting the build but it timed out again in the same way. I also restarted the build for the current master and it passes. This suggests that the timeout is caused by something in this patch.

mnaberez commented 6 years ago

Thanks for digging into this! I made a couple of comments about these changes above. I'm also uncertain of the best way to fix this, but I would like to fix it if we can. If you are able to address those comments, I'll test it on a macOS system and then merge if it works.

SamCoVT commented 5 years ago

I think that's everything you asked for. I've tested this on Linux, Cygwin, and Windows. It allows for pasting of input on Linux and Cygwin, with no change in the Windows behavior. OSX is the only platform I don't have access to, so hopefully you can test it there. I don't expect any issues there. Let me know if there is anything you want done differently.

On a side note, you may want to consider modifying your tests for -l and -r to not use the locations reserved for I/O, but I've made it handle that situation as well.

SamCoVT commented 5 years ago

I've been using this patched version for a while, and it has definitely solved my cut&paste and occasional duplicate characters issues on Linux, but I've run into another issue that is caused by my current solution:

After running some code with a "goto" command, when CTRL-C is pressed and control is returned back to the built-in monitor, echo is still disabled. You can type commands and they run fine, but you can't see what you are typing. I think a call to console.restore_mode() will fix it, but I'm not sure where that call should go. I'll do some digging and see if I can figure out exactly where the CTRL-C is caught and where control returns to in the program, but any hints/suggestions are welcome.

SamCoVT commented 5 years ago

I was able to reliably reproduce mlauke's error in issue #46. I had removed the try/catch in console.getch() as I thought that my solution would fix mlauke's issue as well, but it looks like I need to put it back (but in the new console.noncanonical_mode() function).

As a point of order, should I close this pull request and issue a new one once I think I have all of these things fixed up, or should I leave this pull request open and let you know when I think I have a complete solution?

mnaberez commented 5 years ago

As a point of order, should I close this pull request and issue a new one once I think I have all of these things fixed up, or should I leave this pull request open and let you know when I think I have a complete solution?

Sorry for the delay in responding here, I've been off doing other things lately. I will test the current version of the patch on macOS and report back. I think it would be fine to keep this current pull request going instead of opening a new one. Feel free to add new commits or force push and I'll hold off on merging it until I hear back from you. Thanks!

SamCoVT commented 5 years ago

That sounds good. I should have some time this weekend to look at it in detail and see if I can come up with a fix that works for all of the situations mentioned above. I'll let you know how that turns out.

SamCoVT commented 5 years ago

I have tested this in Linux (Fedora Core 27), Cygwin (latest, 64-bit, on Win7), and Windows (7), and it appears to work properly and handle CTRL-C properly on those systems. I've also tested for redirecting stdin on Linux and Cygwin (I believe this was mlauke's issue and I used that same solution to fix it) and that works properly as well.

Redirecting stdin on Windows doesn't work (and didn't work previously) because it's calling the windows console functions directly, so that behavior is the same. Unlike POSIX systems, Windows treats the console very differently than other files, so I don't know if there is any better method than the current implementation.

I don't have access to OSX, so I can't test it there, but I suspect it works fine there as well. If you can test it there before merging it, that would be awesome. My current test (using taliforth-py65mon.bin which needs the 65C02 processor) is:

py65mon -m 65c02 -r taliforth-py65mon.bin

When you see the "Type 'bye' to exit" message, copy and paste the following (including the newline after the period) into the window:

2 3 + .

You should see: 2 3 + . 5 ok as the result. That means the non-blocking read is working properly (previously, only the first character 2 showed up).

Next up, test the CTRL-C handling by hitting CTRL-C. It should drop you back to the command line of your OS. This was the previous behavior when the -r option is used and CTRL-C was pressed because the -r starts running the 65c02 software from within init, and I kept this same behavior while also making sure to restore the input settings so echo would be back on at the OS prompt.

Next up, run py65mon and load the rom from py65mon's pompt: At the OS command line: py65mon -r 65c02 and then at py65mon's prompt:

load taliforth-py65mon.bin 8000
goto f010

The F010 is the reset vector. This should start up Tali Forth 2 just like before. You can run the same input copy/paste test if you want. The difference is in the CTRL-C behavior. Hit CTRL-C. You should be dropped back to the py65mon prompt and echo of characters should be restored so you can see what you are typing.

mnaberez commented 5 years ago

I tested this patch today on macOS 10.14.2 (Mojave) and, sadly, it does not work. I tried it with Taliforth as above. I also tested it with EhBASIC like this:

$ uname -a
Darwin cbm720-4.local 18.2.0 Darwin Kernel Version 18.2.0: Mon Nov 12 20:24:46 PST 2018; root:xnu-4903.231.4~2/RELEASE_X86_64 x86_64

$ python -V
Python 2.7.15

$ py65mon --load https://github.com/mnaberez/py65/blob/f34b4efa5f5974b386297f510b47640b26fa2307/examples/ehbasic.bin?raw=true --goto ff80
Wrote +65536 bytes from $0000 to $ffff

6502 EhBASIC [C]old/[W]arm ?

With this patch, key input does not work on both EhBASIC and TaliForth. Nothing is echoed and the emulated program does not seem to be receiving any key presses. I tested it against the current master and key input does work. This is a disappointing result, considering that it works on the other environments you tested (and all the work you've put in - thanks!). I'm not sure what's going on here, but I'll leave the PR open in the meantime.

SamCoVT commented 5 years ago

That's a bit sad, although I suppose it is good news that Tali and EhBASIC have the same behavior. I've ordered an old macbook on ebay with Lion on it to see if I can figure out what's going on. I'll have an older version of OSX than you, but if I can fix it there I suspect it will work fine on Mojave - and that will give us two OSX version test points for this problem. I'll let you know if I can reproduce the results you got and I'll push an update if I can figure out how to fix it.

SamCoVT commented 5 years ago

Status Update:

I now have an old MacBook 4,1 (Intel Core 2 Duo) running OSX Lion (10.7.5) and python 2.7.15. I had a difficult time getting homebrew installed and working due to the old version of Ruby that Lion comes with, so I'm using python and pip installed with macports instead. The good news is that I am able to reproduce your behavior.

The other good news is that everything runs so slowly on this hardware that I was able to discover that if you type input BEFORE the 6502 software tries to read it, it will be read by the program. I was able to get TaliForth2 and EhBASIC to both respond this way, and TaliForth2 starts up slowly enough that I was able to type several lines in. It's only once it attempts to read with no input available that it stops accepting input. Hopefully I should have some time this weekend to have a real sit-down session with this setup and see if I can figure out what is happening, but I am encouraged that I was able to get at least some input to be accepted by the 6502 software.

SamCoVT commented 5 years ago

I believe I have Tali2 and EhBASIC working properly now on Linux/OSX/Win on Python 2.7, however I cannot get ehbasic.bin (included in the py65mon) working properly on Python 3.6.6 under linux or on 3.6.8 under OSX. It has the same behavior on both systems: it properly gets the C or W at the cold/warm prompt (and it reprompts for any other letters), but I get stuck at the memory size? input. It seems to be always trying to read characters no matter what I type. I was previously able to hit ENTER here, and I also tried typing in numbers such as 8000. When I CTRL-C, it always seems to be trying to read more input. Do you have an assembly version of that .bin so I can try to figure out what's going on? If so, what assembler do you assemble it with?

I was able to assemble the swapcase.asm (after editing it for my assembler) and that works fine on all versions. TaliForth2 also now works fine on 2.7.5 and 3.6.x, so it's only EhBASIC that's showing this odd behavior at this point.

SamCoVT commented 5 years ago

TLDR version

I think it works now. Please test it on your end and let me know if you want it reorganized a certain way or want changes to the code.

The long version

I was able to figure it out - ENTER was reading as character 10 (a linefeed), as expected, but it was somehow getting through the code that converts it to a CR (character 13) instead. ehbasic was looking for the CR but never got one, which explains why it was confused. TaliForth2 accepts either line ending, which is why it works.

This is likely caused by the fact that I switched stdin to binary (in order to be able to completely turn of buffering, which is what was screwing up OSX), so the automagic-line-ending-transformations that python does are disabled. An odd side effect is that a linefeed from stdin is not equal to the string "\n" anymore - I had to switch to comparing the ASCII values using ord() to make it work.

The way it now works: On non-windows systems, the original parameters for STDIN are saved so they can be restored later.

Next, STDIN is duplicated on a separate file handle. This allows it to be reopened with different options, and in this case it is now opened in binary mode (as opposed to text mode) and buffering is disabled. The file has to be opened in binary mode in order to disable the buffering. If any of this can't be done for any reason, the original (buffered) version of STDIN will be used instead.

When running 6502 software, the input is switched to noncanoical mode, which is basically immediate characters, as they are pressed, with no echo. It is up to the 6502 software to echo the character if it wants to.

When the 6502 software stops running, STDIN is restored to its previous operating setup, which is normally ECHO turned on. This makes it so that the user can see what they are typing at the monitor prompt.

Special care has also been taken to restore the input under several circumstances where CTRL-C are pressed.

Because STDIN may have been opened a second time on a different file descriptor, both files need to be properly closed in order for STDIN to close properly. I added a destructor to the Monitor that only closes the unbuffered STDIN if was created properly.

BigEd commented 5 years ago

@mnaberez did you manage to retest this proposal?

SamCoVT commented 5 years ago

This was passing all of the tests when I last left it. If you look into the failures, they appear to be a problem with virtualenv when trying to start the tests:

virtualenv.py: error: no such option: --no-download

Perhaps something has changed in the test setup?

BigEd commented 5 years ago

Is there possibly a problem with the travis.yml file, @mnaberez ? Causing a hopefully false fail of the CI.

mnaberez commented 5 years ago

I have not made any changes to the Travis setup in quite some time. The recent CI failures may be the result of a change in one of the Python package dependencies or Travis itself. It will need to be investigated. We'll likely have to make a change to the Travis config to fix it.

mnaberez commented 4 years ago

Confirmed locally that the issue noted above is now resolved. Merging, thanks!