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

Add an ability to replay and loop FIFO data #24

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

visual-trials
Copy link

@visual-trials visual-trials commented Nov 24, 2023

This is a re-submission of a PR created by @akumanatt. See #11. The changes Natt made were simply re-applied to the (now refactored due to the FX merge) main branch. In fact only the changes to top.v had to be adjusted. This was done by hand as was therefore prone to human error.

IMPORTANT NOTE: the submitter (JeffreyH) did NOT create this change and does (as of yet) NOT know whether it would work in real HW!

Orignal text from @akumanatt (Jun 10, 2023):

This update adds 2 write bits to VERA registers: AUDIO_CTRL bit 6 and AUDIO_RATE bit 7.

Writing 1 to AUDIO_CTRL bit 6 will only reset FIFO's read position to 0. This allows a sample data in the buffer to be replayed any time without refilling it again. AUDIO_RATE bit 7 set will enable looped playback where the read position is automatically reset to 0 if the buffer becomes empty in the next sample. Although an AUDIO_RATE value of 128 will still play at the maximum rate without looping.

Note that a position of 0 here means an internal buffer's address. Which means a FIFO reset (write 1 to AUDIO_CTRL bit 7) is required in order to reset the write position to 0 too before uploading a sample data and make these methods above play correctly.

Comment/review by JeffreyH (Jun 16, 2023)

I have tried to include these changes into the fx-branch. After doing so I found out a few things:

The change raised the LUT count by 46, which is more than you would expect.
Attempts to reduce the LUT-count were partially successful (down to 16 LUTs) but resulted in a different way of interfacing with the looping/reset settings, which may be undesired.
The change resets rdidx_r (in audio_fifo.v) when a restart of the read position is triggered. However, the state_r (in pcm.v) is not reset. This means that the fifo-reading might be in an intermediate state (say: 8-bit stereo, and only the left byte has been read, but not the right one) when the fifo read position is reset. This may cause strange/unwanted effects.
It is probably required to wait for the state to be DONE or IDLE before executing a read-reset. This means some logic needs to be added and/or adjusted.
It is probably prudent to re-think how the logic should work and how the interface should work for this feature. Then implement that. Then LUT-optimization (and review) can be performed.

JeffreyH

Comment by @akumanatt (12 Aug, 2023):

I updated the loop method to be a combination of AUDIO_CTRL bits 6-7 now. Also, I made it reset only in IDLE state.

@visual-trials visual-trials marked this pull request as ready for review November 24, 2023 04:09
@visual-trials
Copy link
Author

After (trying to) synthesize:

  • Place and Routing failed.

    ERROR - All 1 iterations failed with design error(s).
    It is recommended to correct all design errors before running multiple iterations.
    Please check the Place and Route reports (.par) for the individual iterations under the directory
    "...\vera-module-fifo\fpga\impl\vera_module_impl_par.dir\5_1.par".
    Done: error code 1

  • Cost: +26 LUTs (5015 -> 5041 LUT usage)

No analysis/review done.

@visual-trials
Copy link
Author

After removing the syn_hier="hard" attribute (committed and pushed) in pcm.v and in psg.v the issue is resolved. This attribute was added (in the FX update) for a more efficient LUT-usage, but this can sometimes causes Place and Route issues. This does however come at a small LUT cost.

After synthesizing:

  • No timing errors during Place and Routing
  • Cost: +31 LUTs (5015 -> 5046 LUT usage)

No HW test or analysis/review done.

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.

1 participant