fruit-bat / pico-zxspectrum

ZX Spectrum for Raspberry Pico Pi RP2040
453 stars 48 forks source link

Audio over HDMI? #111

Open msolajic opened 5 months ago

msolajic commented 5 months ago

Hi, would it be possible to output audio over HDMI, too? There is a fork of Wren's PicoDVI library that supports audio over HDMI, over at https://github.com/mlorenzati/PicoDVI

fruit-bat commented 5 months ago

That looks interesting; I will see if I can get it working. Thanks for the link 🙂.

ikjordan commented 3 months ago

I took the code at https://github.com/mlorenzati/PicoDV and made minor changes to address some timing and memory issues and add 50Hz display modes. The repo for this is: https://github.com/ikjordan/PicoDVI/tree/audio

I then used the library to add audio over HDMI to a Pico based ZX81 emulator https://github.com/ikjordan/picozx81.

Hopefully that code may give some tips for adding audio over HDMI to this project? The code within pico-zxspectrum has given me many great ideas in the past :-)

fruit-bat commented 3 months ago

Thanks @ikjordan, I have managed to make some progress with this now! I think the scanline callback is not working properly in this version of PicoDVI, which had me confused for some time. I've swapped the callback for a loop and I now have a nice stable display. Now I need to look at actually sending some audio.

ikjordan commented 3 months ago

@fruit-bat, that's great news! The 3 audio examples in the repo appeared to work ok, but my testing was not extensive. Up to now the ZX81 emulator has been black and white, with the same tmds encode duplicated for the 3 colour channels, so I've not been driving the encode as hard as you will need to do. I am now adding emulation support for a colour ZX81 peripheral Chroma 81. So I'd be grateful if, at some point, you could let me know what changes you needed to make to the PicoDVI fork. Thanks :-)

fruit-bat commented 3 months ago

Early days still, but I have some rather good sound coming from my monitor and the emulator is running OK :-)

I've not tried accessing the menu yet... job for tomorrow.

https://github.com/fruit-bat/pico-zxspectrum/tree/feature/hdmi-audio

https://github.com/fruit-bat/PicoDVI/tree/bugfix/underrun

RattyDAVE commented 3 months ago

If you need another test subject then I am game!

RP2040-PiZero - ZxSpectrumPiZero.uf2

Dave

fruit-bat commented 3 months ago

If you need another test subject then I am game!

RP2040-PiZero - ZxSpectrumPiZero.uf2

Dave

Thanks Dave, please let me know if this works... ZxSpectrumPiZero.uf2.zip

ikjordan commented 3 months ago

This is excellent! I built the pico dv version and gave it a very quick test, it sounds good :-)

When you are happy, I'll merge the PicoDVI changes back into my repo and also make the audio branch the master branch.

fruit-bat commented 3 months ago

This is excellent! I built the pico dv version and gave it a very quick test, it sounds good :-)

When you are happy, I'll merge the PicoDVI changes back into my repo and also make the audio branch the master branch.

Glad it's working well for you :-)

I was wondering about fixing the terminal demo app in the PicoDVI library but not had time to give it much thought. It may even work now the audio underrun is sorted. If you find a fix before me let me know!

I get a warning when compiling my version which is giving me pause for thought...

In function 'compute_subpacket_parity',
    inlined from 'set_audio_sample' at /home/neo/pico/PicoDVI_fruitbat/software/libdvi/data_packet.c:243:9:
/home/neo/pico/PicoDVI_fruitbat/software/libdvi/data_packet.c:139:34: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  139 |     data_packet->subpacket[i][7] = encode_BCH_7(data_packet->subpacket[i]);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/neo/pico/PicoDVI_fruitbat/software/libdvi/data_packet.c:1:
/home/neo/pico/PicoDVI_fruitbat/software/libdvi/data_packet.h: In function 'set_audio_sample':
/home/neo/pico/PicoDVI_fruitbat/software/libdvi/data_packet.h:73:13: note: at offset 39 into destination object 'subpacket' of size 32
   73 |     uint8_t subpacket[4][8];

Any idea what this is about?

RattyDAVE commented 3 months ago

If you need another test subject then I am game! RP2040-PiZero - ZxSpectrumPiZero.uf2 Dave

Thanks Dave, please let me know if this works... ZxSpectrumPiZero.uf2.zip

Not good I'm afraid.

With the SD card in it wont "boot".

Without the SD card it boots fine. But no HDMI audio. (No keyboard clicks or BEEPs).

Reverted to previous version and SD card fine.

ikjordan commented 3 months ago

Any idea what this is about?

I've not really had time for a proper look, but when I rebuild the "before" and "after" repos the warning occurs only in the "after" repo. Is it due to the changes in dvi_update_data_packet_ ?

To simplify buffer wrapping handling int n = MIN(4, MIN(sample_pos_24, read_size)); is used so that a buffer index wrap does not occur within a single call to dvi_update_data_packet_. This is done by reducing the number of buffers written to avoid a wrap, even if they are available. The extra packets are then accommodated in the next few writes

Now that the logic has been changed in this area, is there a possibility of an off by one error? The error quotes 39 which is the same as as writing to [4][7]?

Of course the error may always have been there, and now the change in logic has allowed the compiler to detect it...

How did the buffer underruns manifest? I don't think I've noticed them before?

fruit-bat commented 3 months ago

Any idea what this is about?

I've not really had time for a proper look, but when I rebuild the "before" and "after" repos the warning occurs only in the "after" repo. Is it due to the changes in dvi_update_data_packet_ ?

To simplify buffer wrapping handling int n = MIN(4, MIN(sample_pos_24, read_size)); is used so that a buffer index wrap does not occur within a single call to dvi_update_data_packet_. This is done by reducing the number of buffers written to avoid a wrap, even if they are available. The extra packets are then accommodated in the next few writes

Now that the logic has been changed in this area, is there a possibility of an off by one error? The error quotes 39 which is the same as as writing to [4][7]?

Of course the error may always have been there, and now the change in logic has allowed the compiler to detect it...

How did the buffer underruns manifest? I don't think I've noticed them before?

I was having a problem when my emulator was not emitting audio samples, e.g. when the menu is displayed and sometimes during SD card access. The monitor would go blank and stay blank until a reboot. Demo code that consistently floods the audio buffer works fine.

fruit-bat commented 3 months ago

Any idea what this is about?

I've not really had time for a proper look, but when I rebuild the "before" and "after" repos the warning occurs only in the "after" repo. Is it due to the changes in dvi_update_data_packet_ ? To simplify buffer wrapping handling int n = MIN(4, MIN(sample_pos_24, read_size)); is used so that a buffer index wrap does not occur within a single call to dvi_update_data_packet_. This is done by reducing the number of buffers written to avoid a wrap, even if they are available. The extra packets are then accommodated in the next few writes Now that the logic has been changed in this area, is there a possibility of an off by one error? The error quotes 39 which is the same as as writing to [4][7]? Of course the error may always have been there, and now the change in logic has allowed the compiler to detect it... How did the buffer underruns manifest? I don't think I've noticed them before?

I was having a problem when my emulator was not emitting audio samples, e.g. when the menu is displayed and sometimes during SD card access. The monitor would go blank and stay blank until a reboot. Demo code that consistently floods the audio buffer works fine.

Pushed some changes to get rid of the warning. Looks slightly more efficient now anyhow.

fruit-bat commented 3 months ago

If you need another test subject then I am game! RP2040-PiZero - ZxSpectrumPiZero.uf2 Dave

Thanks Dave, please let me know if this works... ZxSpectrumPiZero.uf2.zip

Not good I'm afraid.

With the SD card in it wont "boot".

Without the SD card it boots fine. But no HDMI audio. (No keyboard clicks or BEEPs).

Reverted to previous version and SD card fine.

oh, that's a shame... maybe this one...

ZxSpectrumPiZero.uf2.zip

RattyDAVE commented 3 months ago

If you need another test subject then I am game! RP2040-PiZero - ZxSpectrumPiZero.uf2 Dave

Thanks Dave, please let me know if this works... ZxSpectrumPiZero.uf2.zip

Not good I'm afraid. With the SD card in it wont "boot". Without the SD card it boots fine. But no HDMI audio. (No keyboard clicks or BEEPs). Reverted to previous version and SD card fine.

oh, that's a shame... maybe this one...

ZxSpectrumPiZero.uf2.zip

We have a winner!

Sound works, SC card works.

Just an observation not a problem: Sound could be a louder. For example, on the TV, I have the sound on 43. Normally I have have the sound on about 10.

Great work!

ikjordan commented 3 months ago

I was having a problem when my emulator was not emitting audio samples, e.g. when the menu is displayed and sometimes during SD card access. The monitor would go blank and stay blank until a reboot. Demo code that consistently floods the audio buffer works fine.

Thanks, that makes sense. In the ZX81 emulator I run a timer interrupt at all times which triggers audio samples to be sent to the driver, so I have avoided the HDMI sound buffer underruns. Of course, all I've done is moved the problem up the stack, I still have to make sure that silence buffers are generated when menus are displayed. I like your approach of having more tolerant lower layers that convert an absence of data into silence samples.

Just an observation not a problem: Sound could be a louder. For example, on the TV, I have the sound on 43. Normally I have have the sound on about 10.

I also noticed that the default sound level was quite quiet. When I went into the menu I saw that the audio level was at 1, when I increased that to 5 the level was good for me.

RattyDAVE commented 3 months ago

In the F1 menu the volume is up to full (halfway on the bar?)

msolajic commented 3 months ago

Could you please provide a test build for Murmulator HDMI platform, as I don't have a toolchain set up to build from source?

fruit-bat commented 3 months ago

Could you please provide a test build for Murmulator HDMI platform, as I don't have a toolchain set up to build from source?

Yup, I've not got one of these so not sure if this will work. Please let me know if it works. It does not support Audio In at the moment... that involves syncing two fixed rate channels and the CPU and I've not done it yet.

ZX-MURMULATOR_HDMI.uf2.zip

(note that you need to unzip this and copy the uf2 to the pico)

fruit-bat commented 3 months ago

I also noticed that the default sound level was quite quiet. When I went into the menu I saw that the audio level was at 1, when I increased that to 5 the level was good for me.

This any better...

ZxSpectrumPiZero.uf2.zip

(it might be a bit loud now, if so, I will try another value)

RattyDAVE commented 3 months ago

I also noticed that the default sound level was quite quiet. When I went into the menu I saw that the audio level was at 1, when I increased that to 5 the level was good for me.

This any better...

ZxSpectrumPiZero.uf2.zip

(it might be a bit loud now, if so, I will try another value)

That's perfect!

No need for any more changes.

msolajic commented 3 months ago

I tested a few games, but I found issues with 128k games. For Example, Dreamwalker - Alter Ego 2 doesn't start, and Sokoban plays music very fast. They both work with the last HDMI firmware from the repository. I made a recording of this, see https://youtu.be/K85j5zWgLmw

And on Dreamwalker you can see, the fadeout of the title screen and colour changing on the first line of text at the introduction, it is much faster on the HDMI Audio version than on the analog audio one.

fruit-bat commented 3 months ago

I tested a few games, but I found issues with 128k games. For Example, Dreamwalker - Alter Ego 2 doesn't start, and Sokoban plays music very fast. They both work with the last HDMI firmware from the repository. I made a recording of this, see https://youtu.be/K85j5zWgLmw

And on Dreamwalker you can see, the fadeout of the title screen and colour changing on the first line of text at the introduction, it is much faster on the HDMI Audio version than on the analog audio one.

Thanks for the feedback. I'm away from the computer for a few days but will try to figure out what is going wrong when I get back.

fruit-bat commented 3 months ago

@msolajic give this one a go...

ZX-MURMULATOR_HDMI.uf2.zip

msolajic commented 3 months ago

@msolajic give this one a go...

ZX-MURMULATOR_HDMI.uf2.zip

Can confirm this works as intended. Audio is OK, no strange issues with games.

fruit-bat commented 3 months ago

@ikjordan I've pull-requested changes back to the audio branch of your PicoDvi repository. Hopefully, made all the alterations I need. Thanks !

ikjordan commented 3 months ago

@ikjordan I've pull-requested changes back to the audio branch of your PicoDvi repository. Hopefully, made all the alterations I need. Thanks !

Excellent, thank you. I'm away at the moment, but will aim to accept the changes over the weekend.

ikjordan commented 3 months ago

I've now merged your changes back. I then: 1) Updated the Readme to describe your addition 2) Fixed the build warnings in the demo apps directory (they are all from the upstream repo, nothing that you had added) 3) Merged the 3 recent additions made in the upstream repo.

You should be able to resync to the audio branch without issues.

Now the DVI repo supports 50Hz framerates, you could have a 50 Hz version of pico-zxspectrum ;-)

fruit-bat commented 3 months ago

@ikjordan Many thanks for your help :-) A 50 Hz would be good and might get a few more programs working.

@msolajic Please would you let me know if these work (There is a PWM and HDMI audio variant): ZX-MURMULATOR_HDMI.uf2.zip

@RattyDAVE Please would you let me know if this works: ZxSpectrumPiZero.uf2.zip

I would like to know if all the other changes I have made have broken these builds. Thanks in advance.

msolajic commented 3 months ago

Yes, both Murmulator variants work OK.

fruit-bat commented 2 months ago

@ikjordan 50Hz versions now available :-)