ducalex / retro-go

Retro emulation for the ODROID-GO and other ESP32 devices
GNU General Public License v2.0
569 stars 130 forks source link

Byteboi rev.1 support #162

Closed birefringence closed 2 weeks ago

birefringence commented 3 weeks ago

I have ported retro-go to the first version of the CiruitMess Byteboi hardware. The second version (which I don't have) seems to have significant differences, so it will require a separate port.

ducalex commented 3 weeks ago

Happy to see AW9523 support being fixed :)

Are you sure that the internal DAC wants unsigned values? I guess it makes sense when you're using a single pin, can't really go negative relative to ground.

But I think your new math should be in the if (differential) block, otherwise it will also affect external DACs (granted no device currently uses RG_AUDIO_USE_INT_DAC 1/2 + external DAC, but still).

eg:

// feel free to rename `differential` to `use_internal_dac` or something more appropriate.
if (differential)
{
    // [current code...]
#if RG_AUDIO_USE_INT_DAC == 1
    left = ((int32_t)left + (int32_t)right)/2 + 32768; // the internal DAC expects unsigned data
    right = 0;
#elif RG_AUDIO_USE_INT_DAC == 2
    left = 0;
    right = ((int32_t)left + (int32_t)right)/2 + 32768;
#endif
}
birefringence commented 3 weeks ago

You are right about the side effects, I'll rework the audio code.

I'm quite certain about the need for unsigned values. I couldn't find any official documentation on this, only some googled code snippet that was doing it like that.

I first tried the original "differential" code but this effectively resulted in silence on the left channel. Then, I tried the original signed values which resulted in an extremely clipped and distorted sound. After converting them to unsigned, it now sounds OK.

There is only one other target (qtpy-gamer), which also only uses one channel (2 in this case, which is the "right" channel). The right channel is handled differently by the "differential" code and may very well result in something that sounds OK. I will, however, again propose something that does the same thing if only one channel is used, independent of whether it is the left or the right one.

birefringence commented 3 weeks ago

OK, do you think this is better? I'm now just using the signed buffer to avoid the extra declaration (technically, you would need to switch between signed / unsigned for external / internal DAC). Since there are no arithmetic operations performed on it, I believe it can be relied upon to relay the correct value. The sound seems OK like that.

ducalex commented 3 weeks ago

Yeah your update seems perfectly fine 👍.

However I've just noticed that you are targeting the master branch, I would prefer if it was the dev branch.

Is it possible to change that in the PR? Otherwise I can merge as is and move the changes to the dev branch myself, but it's not ideal.

birefringence commented 2 weeks ago

OK, I created a new pull request against the dev branch.