X16Community / vera-module

Versatile Embedded Retro Adapter
MIT License
13 stars 5 forks source link

Add an ability to replay and loop FIFO data #24

Open visual-trials opened 7 months ago

visual-trials commented 7 months ago

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 commented 7 months ago

After (trying to) synthesize:

No analysis/review done.

visual-trials commented 7 months ago

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 HW test or analysis/review done.