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

Fix ES8388Source & ES7243 initialization. #3377

Conversation

christianpatterson
Copy link
Contributor

Update ES8388Source::initialize and ES7243::initialize method signatures to match I2SSource::initialize so that when initialize is called on a AudioSource pointer the child class's method is used.

Update ES8388Source::initialize and ES7243::initialize method signatures to match I2SSource::initialize so that when initialize is called on a AudioSource pointer the child class's method is used.
@blazoncek
Copy link
Collaborator

IMO it should be the other way around.
You should remove two obsolete parameters in I2SSource.
Looks like we forgot to do that.

@softhack007
Copy link
Collaborator

softhack007 commented Sep 17, 2023

You should remove two obsolete parameters in I2SSource.
Looks like we forgot to do that.

I agree with @blazoncek. The only place where this specific initialize is called, is here (and likewise in case 2 for es7243)

case 6:
DEBUGSR_PRINTLN(F("AR: ES8388 Source"));
audioSource = new ES8388Source(SAMPLE_RATE, BLOCK_SIZE);
delay(100);
if (audioSource) audioSource->initialize(i2swsPin, i2ssdPin, i2sckPin, mclkPin);
break;

The two extra parameters were meant for SDA and SDL (I2C) but with the change to only use global I2C pins, these are not needed any more.

@christianpatterson
Copy link
Contributor Author

Updated, but I was only able to test it with ES8388 and Generic I2S since that's all the hardware I have.

@blazoncek
Copy link
Collaborator

blazoncek commented Sep 18, 2023

I think this is also a problem @netmindz was facing when moving upstream code to MM.

BTW Nobody seems to have ES7243 any more. Perhaps @FHeilmann?

@netmindz
Copy link
Contributor

Move was actually in the other direction. Removal of I2C from source broke MM as I think the global I2C hadn't actually been merged into MM at that stage

@blazoncek blazoncek added this to the 0.14.0 candidate milestone Sep 18, 2023
* debug messages added to different initializers
* SPH0654::initialize() was having a wrong signature: uint8 instead of int8.

C++ can be a real bastard ;-)
@softhack007 softhack007 merged commit eb66a40 into Aircoookie:main Sep 19, 2023
12 checks passed
@softhack007
Copy link
Collaborator

thanks for this PR :-) well spotted 👍

KSuondProject pushed a commit to KSuondProject/WLED that referenced this pull request Sep 22, 2023
…ive-initialize-i2ssource-bugfix

Fix ES8388Source & ES7243 initialization.
@christianpatterson christianpatterson deleted the audioreactive-initialize-i2ssource-bugfix branch September 22, 2023 14:11
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.

None yet

4 participants