earlephilhower / ESP8266Audio

Arduino library to play MOD, WAV, FLAC, MIDI, RTTTL, MP3, and AAC files on I2S DACs or with a software emulated delta-sigma DAC on the ESP8266 and ESP32
GNU General Public License v3.0
2.01k stars 432 forks source link

document best practices #74

Open mcuadros opened 6 years ago

mcuadros commented 6 years ago

I am using the library intensively I found that sometimes cause the resent, for example, if you are using AudioFileSourceICYStream you should check that the URL request was ok, Otherwise may cause a

  file = new AudioFileSourceICYStream(url);
  if (!file->isOpen()) {
    Serial.println("Unable to open URL");
    return;
  }

Also, AudioFileSourceBuffer has isOpen but I am not sure if the buffer may fail.

Another good example is how to clean up the objects to avoid memory leaks like;

  mp3->stop();
  buff->close();
  file->close();
  delete mp3;
  delete buff;
  delete file;
  mp3 = NULL;

That's I am asking if a complex example or something similar can be written in order to help newbies like me, to have a rock solid code.

earlephilhower commented 6 years ago

This is actually my note to myself in #19 . I've been distracted with rewriting the Arduino SSL setup and an interesting opportunity to add I2S input.

mcuadros commented 6 years ago

I must to say that I learned a lot from the WebRadio example, but it's hard to follow, and even with it, I found some weird things, like:

https://github.com/earlephilhower/ESP8266Audio/blob/master/examples/WebRadio/WebRadio.ino#L314 Why the bool returned by begin is unhandled?

https://github.com/earlephilhower/ESP8266Audio/blob/master/examples/WebRadio/WebRadio.ino#L304 Why the isOpen isn't checked at this point instead of using isRunning for detecting if the HTTP request was unsuccessful?

earlephilhower commented 6 years ago

The examples are not nearly as scrubbed as the library code, you're right.

https://github.com/earlephilhower/ESP8266Audio/blob/master/examples/WebRadio/WebRadio.ino#L314 Why the bool returned by begin is unhandled?

It's ignored because in 2 lines it checks decoder->isRunning() which returns false when the begin fails. It's safe here, but you could also stop and error out at the begin.

Not sure where this is point to or what you're asking, but the code segment there basically tries to get everything into a steady state and checks the decoder is functioning after building the bits, not while building them. It's just simpler that way. W/o exceptions, lots of individual error handling ends up with a 10-level if-else clause or lots of returns in the middle of the code.

https://github.com/earlephilhower/ESP8266Audio/blob/master/examples/WebRadio/WebRadio.ino#L304 Why the isOpen isn't checked at this point instead of using isRunning for detecting if the HTTP request was unsuccessful?

mcuadros commented 6 years ago

I am trying to identify witch things can make crash the board, for example: https://github.com/earlephilhower/ESP8266Audio/blob/master/examples/StreamMP3FromHTTP/StreamMP3FromHTTP.ino#L74-L81

If the server has a weird behavior, the board crashes. So the isOpen requires being checked.

I know I am being picky but, I am trying to do a resilient code, ensuring that is rock solid. I guess that is my Golang background of the last years.

Thanks for your excellent library.

mixographer commented 6 years ago

What is the best practice for selecting which ESP32 board to use? I have the DevKit, and I cannot select 160MHz for that board. Is there a recommended board?

earlephilhower commented 6 years ago

@mixographer No ESP32 can run at 160MHz that I know of (at least w/Arduino). This lib was developed by me on the 8266, where all the boards can do 160MHz (but the core defaults to 80MHz). 240MHz or whatever ESP32 default speed is, is plenty (and 2-8x faster than the ESP8266 @ 160MHz since code runs from internal RAM!).

So, basically, any board of either 8266 or 32 varieties will do for you, assuming the required I2S pins are pulled out (i.e. ESP-01 with only 2 GPIOs pulled to the edge is not gonna work so well...)

mixographer commented 6 years ago

Good to know. I'm running up against buffer underflow errors, so I'll keep working on that.

Connected
+0 0x3ffb1f38
Running for 4578 ms...
STATUS(buffer) '2' = 'Refilling buffer'
STATUS(mp3) '565' = 'Decoding error 'bad main_data_begin pointer' at byte offset 0'
STATUS(buffer) '3' = 'Buffer underflow'
Running for 5621 ms...
STATUS(buffer) '2' = 'Refilling buffer'
STATUS(buffer) '3' = 'Buffer underflow'
MP3 stop, len==0
MP3 done