Skip to content

3 bug fixes#113

Open
dmantione wants to merge 2 commits into
MEGA65:masterfrom
dmantione:master
Open

3 bug fixes#113
dmantione wants to merge 2 commits into
MEGA65:masterfrom
dmantione:master

Conversation

@dmantione

Copy link
Copy Markdown

Hello,

The low-level serial API cannot work as it is now, due to a few bugs. 3 bug fixes in the serial code:

  • Ciout did store the byte to be transmitted in BSOUR while iec_tx_byte expects the byte to be transmitted in TBTCNT
  • iex_tx_flush did call iec_tx_byte, but that is wrong is JiffyDOS is used, the byte needs to be transmitted using JiffyDOS protocol
  • jiffydos_tx_byte tries tro avoid changing the VIC-II bank, but did not do that for the last 2 bits.

With these changes the code works and is able to do serial communication.

…e iec_tx_byte expects it

  * iec_tx_flush calls iec_tx_dispatch, because JiffyDOS protocol can be in use.
  * jiffydos_tx_byte now preserves VIC-II page during last tuple of a byte
Comment thread src/kernal/iec/ciout.s
@dmantione

Copy link
Copy Markdown
Author

Yes certainly.

  • In the Open ROMs, the routine iec_tx_byte expects the byte to be transmitted in TBTCNT
  • In the original KERNAL, the equivalent routine ISOUR expects the byte to be transmitted in BSOUR

So because the Open ROM implementation uses a different variable than the original ROM, CIOUT has to be adapted as well. If you store in BSOUR while iec_tx_byte expects it in TBTCNT, it just doesn't work.

@FeralChild64

Copy link
Copy Markdown
Collaborator

So the proper fix would be to adapt iec_tx_byte to take the value from BSOUR. Some other tests and adaptations might be needed.

@dmantione

Copy link
Copy Markdown
Author

Yes, if you want to make the code more similar to the original KERNAL, this is what needs to be done. However, I don't believe using TBTCNT is inherently broken: It can work and it makes the code more different from the original, so might be an advantage from a copyrights point of view. It is a matter of taste: If you want all zero page locations to have their original function, iec_tx_byte (and its JiffyDOS counterpart) need to be changed.

@FeralChild64

Copy link
Copy Markdown
Collaborator

If you think this change would be too risky/problematic, we keep it this way for now - but please add appropriate comment in ciout.s and a remark in the STATUS.md document. it should be clear that we have deviated here.

@dmantione

Copy link
Copy Markdown
Author

Changing from TBTCNT to BSOUR would certainly require a lot more extensive testing, for example LOAD/SAVE also uses these routines and would need modification+testing. So this one line change is certainly the fastest and lowest-risk fix at the moment. A good approach would be to keep it this way and migrate if future problems are discovered.

I have added a notice in both ciout.s and STATUS.md.

Comment thread STATUS.md

## Serial buffer byte location

The KERNAL CIOUT routine buffers the byte to be sent into a zero page location, because only when the next byte is sent

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The chapter is not necessary - a short note in the table below is what we need :)

Image

@FeralChild64

Copy link
Copy Markdown
Collaborator

One more thing - we have a tool which detects similarities between out code and the original ROMs (see https://github.com/MEGA65/open-roms/blob/master/README.md#defending-against-potential-copyright-infringement-claims), to use it one needs to put kernal and basic files to 3rdparty/ROMs directory and execute a make similarity command. I see we now have one similarity in the KERNAL code over 3 bytes long:

$0CD3 = $3FFD : 00 00 00 00 00 00 03

Could you also add an explanation file, similar to https://github.com/MEGA65/open-roms/blob/master/strings/0000000003 (file needs to be named 00000000000003)?

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