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

OSIC1P bug on kbhit: it probably consumes the input #511

Closed
Fabrizio-Caruso opened this issue Nov 5, 2017 · 20 comments
Closed

OSIC1P bug on kbhit: it probably consumes the input #511

Fabrizio-Caruso opened this issue Nov 5, 2017 · 20 comments
Labels

Comments

@Fabrizio-Caruso
Copy link
Contributor

Fabrizio-Caruso commented Nov 5, 2017

The OSIC1P target has an issue with the keyboard input.

My game CROSS CHASE would simply freeze waiting in vain for a key press.
The game can be made sort of playable (by avoiding some code) only in turn-based mode.

It looks like as if kbhit would detect the key press and consumes the key press.
A subsequent cgetc waits in vain.

@smuehlst
Copy link
Contributor

smuehlst commented Nov 6, 2017

What I can reproduce is that kbhit() blocks in all calls but the first one. I will try to analyze the problem.

@Fabrizio-Caruso
Copy link
Contributor Author

Fabrizio-Caruso commented Nov 6, 2017

@smuehlst, kbhit should not consume the first key-press, i.e., a subsequent cgetc should read it without waiting. In

if(kbhit()) { movePlayerByKeyboard(cgetc());} 

the normal correct behaviour is:

  1. the key is pressed (e.g., key "A")
  2. kbhit becomes TRUE
  3. cgetc reads "A"

cgetc is correctly supposed to wait for a key-press if no key press is in the buffer; but, kbhit should NOT remove the detected key press from the buffer.

TEST: The following code should do what its name suggests (in a clean way, i.e., by flushing the buffer):

		void WAIT_PRESS(void)
		{
			while(kbhit())
				cgetc();
			while(!kbhit())
			{
			}
			cgetc();
                }

P.S.: In the latest version of CROSS CHASE for the OSI C1P target, I have removed some code in order to check whether the rest of the game works, and it DOES! :-) So, I only see a problem with the keyboard input.
As a consequence, once kbhit is fixed (I supposed cgetc is OK), my game should stop behaving as a turned-based game; and, it should revert to the action game it is supposed to be.

@Fabrizio-Caruso
Copy link
Contributor Author

@smuehlst you can simply test with the function WAIT_PRESS above. Any progress?

@Fabrizio-Caruso
Copy link
Contributor Author

@smuehlst libsrc/osic1p/knhit.s does more than detecting the input, it also reads the value and this is not what kbhit is supposed to do. kbhit should simply return true if any key is pressed (including still pressed and not just newly pressed) and the input buffer should remain unchanged.

@Fabrizio-Caruso
Copy link
Contributor Author

@smuehlst I do not know how the Ohio Scientific C1P works.
I am only trying to guess: the keyboard detection may not be fully supported by the hardware and if this is the case we should simulate what is missing...
I mean that it could be as for the Apple II where the key-release is not detected by the hardware. I am only trying to help you to investigate this issue...

@smuehlst
Copy link
Contributor

smuehlst commented Nov 9, 2017

@Fabrizio-Caruso: There is no progress yet. The implementation of kbhit() was done in collaboration with an other person, and I contacted the person for help. I got feedback, but it will take a while.

About the kbhit() implementation: The function checks with a low-level method whether a key is pressed, and if that is the case, it reads the character with the correspinding C1P ROM function and buffers it. The cgetc() functions is supposed to return the buffered character if there as one buffered by kbhit(), otherwise it should wait for the next character.

@Fabrizio-Caruso
Copy link
Contributor Author

Fabrizio-Caruso commented Nov 9, 2017

@smuehlst then maybe we know what and why it is wrong: if no key is pressed kbhit is false and a subsequent cgetc should wait for a key and if a key is pressed it should read the key press WITHOUT the need of kbhit.
What is filling the buffer? Just kbhit?
The function I have given you here is a good test.
The second cget above should be unlocked by a key press and not wait forever for kbhit to fill the buffer.

@smuehlst
Copy link
Contributor

if no key is pressed kbhit is false and a subsequent cgetc should wait for a key and if a key is pressed it should read the key press WITHOUT the need of kbhit.

This is exactly how the current implementation is intended to work, but for some reason is doesn't

What is filling the buffer? Just kbhit?

Only kbhit is filling the buffer. When cgetc() is invoked, it checks the buffer. If it contains a character, it returns that and clears the buffer. Otherwise it calls the ROM function to read a character.

@greg-king5
Copy link
Contributor

Side note: cc65 can build a self-loading program for the OSI Challenger 1P -- see http://cc65.github.io/doc/intro.html#ss6.6

I use this test program:

#include <conio.h>

void main(void)
{
    clrscr();
    cputs("Tap a key: ");
    while (kbhit()) cgetc();  // flush any key
    while (!kbhit()) { }  // wait for a key
    cputc(cgetc());  // get and show it
    cputs("\nSuccess. ");
//    cgetc();
}

When I press some keys, I see what we expect to see (the monitor's prompt again). But, when I press most keys, the program jams after it shows "Success.". Either way, it does wait, then get and show the tapped key.

jefftranter added a commit to jefftranter/cc65 that referenced this issue Dec 2, 2017
See my comments in the issue for more details.
@jefftranter
Copy link
Contributor

I spent some time looking at this issue.

The main problem is that kbhit() can block because it detects a key pressed but when it goes to read the key, sometimes the key is no longer pressed down and so it blocks until a key is pressed again. This is hard to solve because it is using a keyboard input routine in the OSI ROM.

I also found an issue that the OSI ROM input routine can corrupt some memory locations ($0213-$0126) that are used by code generated by cc65, which can cause applications to crash.

I worked on a new input routine that doesn't use the OSI ROM code. I started with a disassembly of the OSI code, but it is quite complicated and no source code was ever published for it. So I switched to writing new code from scratch in C.

I have a new implementation of kbhit() and cgetc() that seems to work very well. It works with a test program and I also tried the CROSS CHASE game program and it works well. All the testing was done on my Briel Superboard /// machine.

At some point I might look at converting the code to assembler to make it smaller.

I will submit a pull request for this code shortly.

@Fabrizio-Caruso
Copy link
Contributor Author

Fabrizio-Caruso commented Dec 2, 2017

Thanks! Be aware that the current version of CROSS CHASE avoids kbhit for the OSIC1P to get at least the game to work in turn-based mode. So in order to test it with my game, you have to change two lines in WAIT_PRESS and movePlayerByKeyboard.

@smuehlst
Copy link
Contributor

smuehlst commented Jan 5, 2018

@jefftranter submitted the new pull request #567. This fixes the issue that zero page locations can be corrupted, but it does not fix the issue that kbhit() blocks. As far as I understand it,this happens because of the implementation of debouncing and auto-repeat in cgetc(). There is currently no known way to fix this problem with the new code based on the OSI C1P ROM code.

Are there any implementations of kbhit() and cgetc() in conio for other targets that operate in a similar environment? On the OSI C1P we have this:

  • A memory location must be polled in kbhit() to determine whether any key is pressed.
  • In cgetc() the same memory location must be read and decoded, and debouncing and auto-repeat are implemented inside cgetc().

If such an implementation exists, its ideas maybe can be applied to the OSI C1P implementation.

@greg-king5
Copy link
Contributor

kbhit() has two bugs:

  1. It doesn't care whether or not cgetc() has read the character that was seen by a previous pass.
  2. It uses two independent keyboard scanners. That causes a "race condition" between the second scanner and a user. When the second scanner wins the race (by finding the pressed key that the first scanner saw), kbhit() works. But, when the user "wins" (by lifting the finger too soon), the second scanner sees nothing; then, it's like cgetc(), it waits for a second key. Therefore, kbhit() stalls.

I will put more details in PR #567.

@smuehlst
Copy link
Contributor

@Fabrizio-Caruso Pull request #933 was merged now. Can you please retest whether CROSS CHASE works now on OSI? I tried to do this myself, but it is unclear to me how to control the game. Maybe it still doesn't work as expected.

@Fabrizio-Caruso
Copy link
Contributor Author

Fabrizio-Caruso commented Oct 15, 2019

@smuehlst it does work sort of fine. It works like Apple //, i.e., auto-repeat is triggered after a short delay. This may be the way the real hardware behaves but I cannot tell you if it is OK.
My binaries are in:
https://github.com/Fabrizio-Caruso/CROSS-CHASE/releases/tag/osic1p
where turn-based versions work as previous versions (as expected)
and where non-turn-based versions produce an action game (which was impossible with the previous buggy cgetc/kbhit).

Your new code has to be tested with the non-turn-based verions:
https://github.com/Fabrizio-Caruso/CROSS-CHASE/releases/download/osic1p/TINY_osic1p_8k.lod
https://github.com/Fabrizio-Caruso/CROSS-CHASE/releases/download/osic1p/TINY_osic1p_8k.lod

The game is controlled with I J K L for movements and SPACE to shoot (need gun first).

@smuehlst
Copy link
Contributor

@Fabrizio-Caruso thanks for the explanation. I will try the game on my real machine later this week.

@Fabrizio-Caruso
Copy link
Contributor Author

@smuehlst I have rebuilt the game in order to make it less hard. Please load it again if already loaded.
I am not sure it is perfect because it looks like keeping a key pressed freezes the game for an instant before auto-repeat is triggered. In any case, this is MUCH better than before.
I have no idea if this is the way the real hardware behaves.

@smuehlst
Copy link
Contributor

@Fabrizio-Caruso I had tried it on the emulator last week, and today I loaded it onto the real machine. As far as I can tell it works, although I'm not yet really successful at playing it... The AI adversaries are overpowering. But nevertheless I'm currently the CROSS CHASE high score owner on a real OSI C1P machine in this universe with 3200 points 😁 I'll send you a link to a video once I managed to upload it to YouTube.

I think you can close this issue now, thanks for reporting it.

@Fabrizio-Caruso
Copy link
Contributor Author

@smuehlst Yes, I can close it. Is the lag between first and second repeated key as expected by real hardware.
If you use rom routines and you get that delay, then you have replicated the behavior. If there is no lag in rom routines, then we still have a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants