8000 Improvements to ADPCM-B implementation by aaronsgiles · Pull Request #40 · aaronsgiles/ymfm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improvements to ADPCM-B implementation #40

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Improvements to ADPCM-B implementation #40

wants to merge 13 commits into from

Conversation

aaronsgiles
Copy link
Owner
@aaronsgiles aaronsgiles commented Jul 18, 2022 •
edited
Loading

Specific fixes:

  • Implement EOS and PLAYING flags as sticky bits that require manual clearing on all implementations.
  • Implement correct end-of-sample behavior, skipping final 3 nibbles.
  • Implement read access patterns that match hardware tests.
  • Match behavior of "dummy" reads to hardware tests.
  • Hook up verified edge case behaviors for over-reading and over-writing data.
  • Add framework for running ADPCM tests to compare against hardware results.

@aaronsgiles aaronsgiles mentioned this pull request Jul 18, 2022
Closed
@aaronsgiles
Copy link
Owner Author

Continuing conversation from #38 over here.

hyano
hyano reviewed Jul 20, 2022
View reviewed changes
Copy link
Contributor
@hyano hyano left a comment

Choose a reason for hiding this comment

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

The latest adpcm-test code works for my project.

For unusual cases, I noticed two points that seem to be incorrect.

src/ymfm_adpcm.cpp Outdated
m_status = STATUS_BRDY;
set_reset_status(STATUS_EOS);
m_curaddress = LATCH_ADDRESS;
m_regs.write(0x00, m_regs.read(0x00) & ~0x40);
Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior is seen only when REPEAT=1.
When REPEAT=0, the second cycle is also a write operation (record mode).

if (m_regs.repeat())
    m_regs.write(0x00, m_regs.read(0x00) & ~0x40);

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmmm, so if REPEAT=0 then what happens? Does the write continue at the start address? Did you have a test for that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, so if REPEAT=0 then what happens? Does the write continue at the start address?

Yes.

Did you have a test for that case?

One of the examples is:
https://github.com/hyano/opna-analyze/blob/main/test/mem_rw.py
https://github.com/hyano/opna-analyze/blob/main/log/mem_rw_mden/mem_rw.txt

I've converted to your C++ framework. Please check it.
mem_rw.cpp.txt

Currently, I cannot understand actual meaning of REPEAT bit.

Copy link
Owner Author
@aaronsgiles aaronsgiles Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I have checked in your rw test. I notice that the previous work to fix dummy reads broke one of the tests, so I made some more changes to fix that.

I also made another change to initialize memory to 0x80 because the mem_rw test causes a read from uninitialized memory at the end of the "READ ADDRESS 0fff- CHANGE START(10/RESET)" test, so that the dummy reads in the next test "READ ADDRESS 0fff- CHANGE STOP" return that uninitialized memory. Your results showed 0x80 there, so I initialized my memory to 0x80 so that we'd match, but it would be better if the test were fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your results showed 0x80 there, so I initialized my memory to 0x80 so that we'd match, but it would be better if the test were fixed.

Yes, I also noticed it.
When I create mem_rw.py, dummy read mechanism was unclear and I ignored it.

I have updated my test to fill extended area. And confirmed reading expected values.
https://github.com/hyano/opna-analyze/blob/main/test/mem_rw.py
https://github.com/hyano/opna-analyze/blob/main/log/mem_rw_mden/mem_rw.txt

The log and signals are also recaptured, so you can refer if needed.
https://github.com/hyano/opna-analyze/blob/main/log/mem_rw_mden/

Thank you for using my tests even if it is very ad-hoc and experimental...

mem_rw.cpp.txt

@hyano
Copy link
Contributor
hyano commented Jul 23, 2022

I've added test for another unusual cases which READ & REC=1.

In that condition, I observed:

  • No memory access happens (confirmed by MDEN signal).
  • The internal data buffer becomes empty after reading 2bytes and BRDY becomes 0.

It would be a hint for behavior of internal data buffer.

mem_rw16.cpp.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyano hyano hyano left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aaronsgiles @hyano
0