Add support for WM8960 to CI2SSoundBaseDevice

[matemaciek]

I've added i2c init commands for WM8960, based on raspiaudio Ultra++ hat.

Another change is trying to detect DAC type automatically, basing on found i2c devices.

[probonopd]

If you need someone to test something with the WM8960 feel free to ping me at any time. (This is currently the only DAC I have so I can't test whether it breaks something for other DACs.)

[probonopd]

Tested with MiniDexed and a WM8960 DAC, works for me! Both the headphone jack on the WM8960 board and also the speakers connected to the built in amplifier 👍

Here is the build:
https://github.com/probonopd/MiniDexed/suites/6210021520/artifacts/219385493

Now it would be good if someone with a PCM5102A or PCM5122 based DAC could also test this in order to ensure that it doesn't break those.

[matemaciek]

I confirmed it works with raspiaudio Audio+ V2 which I believe has PCM5102A, but it does not connect it to i2c bus, so essentially there's no change there.

[sebastienNEC]

Hello,
Thank you for the new device, I would also be able to contribute 2 or 3 new codecs. I feel however that it might be good to move such individual device support to other classes for these reasons:

1. CI2SSoundBaseDevice will become very crowded over time

2. There is no such thing as one codec configuration, eg some might want to use the microphone input of the codec, some the line input, etc. Supporting all the use cases would make the API too crowded.

I would suggest removing I2C from this class, and introduce another abstract CAudioCodec class with both I2C and I2S members. Daughter classes would implement adhoc functions for the codecs various capabilities.
Possibly, with the growing number of drivers in circle, one could introduce a src/driver directory ?

[dwhinham]

@sebastienNEC I agree 100%.

That is exactly what I had in mind when I wrote this TODO comment 2 years ago:
https://github.com/dwhinham/mt32-pi/blob/ae0e146141255425c117a1239b8dcc439358747a/src/mt32pi.cpp#L1227

The hardcoded array of register pokes was a quick hack that got a few more popular DACs working for a bunch of people, but it was never going to be a maintainable solution.

I would be very happy if I could remove it from mt32-pi if Circle gains a nicer solution.

[matemaciek]

I also felt that architecturarly this I2C code should ideally live somewhere else (yet still within Circle) - but I was aiming at enabling yet another bunch of people to use it. Abstract DAC configuration layer sounds like a tempting long-term goal (-:

[dwhinham]

A build of mt32-pi with its existing manual initialization removed in favour of Circle's auto-detection, with this patch also applied, is available here to aid testing: https://github.com/dwhinham/mt32-pi/actions/runs/2203991149

I have some PCM51xx devices at home, will try to find time to ensure nothing's regressed soon unless someone else wants to volunteer.

Note that I am unable to test WM8960.

From my side, I'll keep this in a test branch (https://github.com/dwhinham/mt32-pi/commits/dac-improvements) until a decision is made on this PR.

Cheers!

[probonopd]

A build of mt32-pi with its existing manual initialization removed in favour of Circle's auto-detection, with this patch also applied, is available here to aid testing: https://github.com/dwhinham/mt32-pi/actions/runs/2203991149

@dwhinham: Works on the WM8960!

[rsta2]

@matemaciek Thanks for the pull request. Before the WM8960 support can be integrated into Circle, I think there are some points to be cleared. From the current point I view, I would move the I2C initialization from CI2SSoundBaseDevice to one new class. Generally I'm only a friend of greater class architectures, when there is some gain from it. In the end I think that one advantage of Circle is, that it is small. The class CI2SSoundBaseDevice is easy to use, which should remain the case.

@sebastienNEC I'm not an expert for audio codecs. Can you please give more info, what methods the suggested abstract class CAudioCodec should have, so that I can understand better, what you want to do with it?

[probonopd]

I notice that the sound from the WM8960 headphone jack doesn't sound as clean as my cheap hack. Tested with high quality headphones. Maybe some initialization parameters can still be tweaked to get better sound?

[matemaciek]

Yeah, I reported this to Raspiaudio team, they're working on improving initialization procedure.

[rsta2]

@matemaciek Can you please check, if it is legal to use this code, you have used, in a GPLv3 project like Circle? Their project does not have a license, which is normally required, if one wants to copy and modify code for his own project.

[matemaciek]

@rsta2 OK, let's see what they'll answer: RASPIAUDIO/ULTRA#1

[DeuxVis]

@rsta2 OK, let's see what they'll answer: RASPIAUDIO/ULTRA#1

FYI they just added a GNU General Public License v3.0 LICENSE file 2 minutes ago.

[rsta2]

The added WM8960 support is at the develop branch now, where development takes place. I did rollback the I2C detection, because I'm not convinced, that the i2c-detect test, that you used, will work reliable in all cases. Also the WM8960 will be probed at last now. I tested the modified driver successfully with sample/29-miniorgan and with PCM5102A and PCM5122 DACs. It would be great, if you would test it with your WM8960 DAC. Thanks again for this contribution!

[probonopd]

Do you have a binary build for the RPi 3 at hand? Then I can test asap.

[rsta2]

This is sample/29-miniorgan, built for RPi 3 (AArch64) with I2C with auto-probing. Thanks!

kernel8.zip

[probonopd]

Works like a treat on the WM8960 👍

[matemaciek]

Please also include these changes in the develop branch.

[rsta2]

@matemaciek I have included the changes. Thanks!

@probonopd Thanks for testing!

[probonopd]

So auto-probing works now and doesn't destroy the PCM5xxx?

[rsta2]

@probonopd Seems so.

[probonopd]

Built https://github.com/probonopd/MiniDexed/suites/6230488535/artifacts/220979632 using the develop branch of Circle. WM8960 works. If PCM5xxx also still works, we have a winner 👍

[rsta2]

@probonopd Tested your image with PCM5102A and PCM5122 on RPi Zero 2 W. Both worked well. 👍

[sebastienNEC]

@rsta2 The CCaudioCodec class would initially just be a sort of proxy class with (initialized) I2S/I2C members and an abstract API to do things such as muting / setting the volume / jack detection. It would not be too useful per se but would enforce a common API for the many drivers that are likely to land in Circle over the years.
This class is probably not necessary for now but the greater question is: should circle include/enforce some sort of infrastructure/API for drivers ?

[rsta2]

@sebastienNEC I think, I understand. Give me until Thursday to prepare an suggestion. Thanks.

[rsta2]

I started a new discussion about improved sound support (#291). Feel free to answer on this. If you want to discuss other things, you can open another discussion. I think this PR has been merged (manually) already and can be closed.