search

sle118 / squeezelite-esp32

ESP32 Music streaming based on Squeezelite, with support for multi-room sync, AirPlay, Bluetooth, Hardware buttons, display and more
1.27k stars 115 forks source link

Fix for I2S noise burst when ESP32 panic occurs #437

Closed digidocs closed 2 months ago

digidocs commented 3 months ago

When a panic occurs during playback, the I2S interface can produce a loud noise burst. This PR intercepts the system panic handler to "emergency stop" the I2S interface to prevent the noise burst from happening. This PR also adds an "abort" command to the command interface that can be used to test this feature.

philippe44 commented 2 months ago

Looks good as well

digidocs commented 2 months ago

Thanks for reviewing. Anything else needed to get this one merged?

philippe44 commented 2 months ago

No, I don't think so

sle118 commented 2 months ago

Thank you for your contribution!

philippe44 commented 2 months ago

@digidocs: I'm re-opening that b/c I was working on the s3 version and unfortunately, this PR was esp32-specific. I've put it somewhere else but I'm not sure it works b/c it does a critical section request within the panic handler... Could you try the latest code I've pushed?

digidocs commented 1 month ago

@philippe44 I worry that calling i2s_stop could inconsistently cause deadlocks or other faults during the panic process. The system is already crashing so it's best to minimize the footprint of other code that we call. If I have some time I can try to look at other ways of doing this that might work. I'm not as familiar with the S3 portions of the codebase. Is there anything I should know about there as I think about other ideas?

philippe44 commented 1 month ago

I fully agree with you but the problem is that the way it is done now is by adding code to i2s.c which is a part of esp-idf. So it required a local "override" copy of the whole file so in case esp-idf changes, we missed it. It's necessary for esp32 4.3 b/c espressif had a bug in the pll loop calculation that they never corrected, but on s3 I was able to have no override at all for any file. To do as you did, we would need again an override copy for s3. Not great...