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

kbhit optimized for telestrat #1393

Closed
wants to merge 6 commits into from
Closed

kbhit optimized for telestrat #1393

wants to merge 6 commits into from

Conversation

jedeoric
Copy link
Contributor

@jedeoric jedeoric commented Feb 2, 2021

As we discuss, here is kbhit optimized

@jedeoric jedeoric mentioned this pull request Feb 2, 2021
@oliverschmidt
Copy link
Contributor

Now that your original code is merged this pull request can't be applied as-is. Please update your repo and add a commit to this pull request.

ldx #$00
bcs @no_char_action
lda #$01
rts
@no_char_action:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does A have a defined value at this point from BRK_TELEMON XRD0? If yes, it might be worth mentioning in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses carry to set return value, but i have to look to source code to know what A returns. I try to have a look tonight

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my comment above THAT useful. What I actually wanted to express and what I probably should have written is: "I consider your recent proposal broken. The only way it could potentially work is if BRK_TELEMON XRD0 would be well known to return a certain value in A that you only have to override if necessary. But I rather presume that this isn't the case." You see, I'm not actually interested in the fact what BRK_TELEMON XRD0 returns in A. There's no need that you try to find out. Rather just go ahead and write code here which works independently from BRK_TELEMON XRD0' setting of A.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, Oliver, I cannot follow you.

lda #$01
rts
@no_char_action:
lda #$00
tax
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be "txa"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point. I will fix it tonight. Thanks for the point :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be "txa"

I don't agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure we're talking about the same code. This ias what you want to see? tax replaced with txa, right?

_kbhit:
        BRK_TELEMON XRD0
        ldx     #$00 
        bcs     @no_char_action
        lda     #$01
        rts
@no_char_action:
        lda     #$00
        txa        
        rts

My take is that the tax shouldn't be replaced with txa. Rather it should be removed altogether.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about something like:

_kbhit:
        BRK_TELEMON XRD0
        ldx     #$00 
        bcs     @no_char_action
        lda     #$01  ; return one
        rts
@no_char_action:
        txa        ; return zero
        rts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's not what you wrote above - or at least what I thought you wrote ;-) Anyhow, I guess the OP now has enough input on how to get it right...

@oliverschmidt
Copy link
Contributor

oliverschmidt commented Feb 4, 2021

Trying to bring this back on track...

A simple, straight forward option to write this - as suggested in #1392 (comment) - is:

.import return0, return1

_kbhit:
        BRK_TELEMON XRD0
        bcs     @no_char_action	
        jmp     return1
@no_char_action:
        jmp     return0

A "trying to be cool" option to write this is:

_kbhit:
        BRK_TELEMON XRD0
        ldx     #$00
        txa
        asl
        eor     #$01
        rts

@groessler
Copy link
Contributor

That's the best version IMO. Smaller and faster than the others. But, asl should be rol :-)

oliverschmidt added a commit that referenced this pull request Feb 5, 2021
@oliverschmidt
Copy link
Contributor

But, asl should be rol :-)

How embarrassing :-(

I just directly pushed the fixed version: 8551431

@jedeoric
Copy link
Contributor Author

jedeoric commented Feb 5, 2021

Thanks. For information, XRD0 returns in A ascii code of the key pressed

@greg-king5
Copy link
Contributor

For information, XRD0 returns in A ascii code of the key pressed.

In that case, both kbhit() and cgetc() must be redesigned. They must work together.

kbhit():
It sees that a key was tapped because it got a character! kbhit() must not lose that character. It must save that character somewhere. If kbhit() is called again before cgetc() is called, then it must return "true" immediately because of that saved character -- it mustn't ask Telemon for a new character.

cgetc():
It must behave in a similar way. If a saved character exists, then the function must return that character -- it mustn't ask Telemon for a new character (and, it can skip the cursor code). The function must "remove" the old character, in some way, so that the two functions will call Telemon the next time that they are called.

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.

None yet

4 participants