miketeachman/micropython-i2s-examples

Root location of audio should probably not be hard coded

diputssi opened this issue · 5 comments

If device doesn't have an SD card reader, it's currently not possible to pass a filename into the play method with out changing the underlying code.

For example in wavplayer.py:

def play(self, wav_file, loop=False):
    if self.state == WavPlayer.PLAY:
        raise ValueError("already playing a WAV file")
    elif self.state == WavPlayer.PAUSE:
        raise ValueError("paused while playing a WAV file")
    else:
        self.wav = open("/sd/{}".format(wav_file), "rb")

/sd is hard coded. What if I don't have this device? A non-breaking alternative would be to create a default variable into the method like this:

def play(self, wav_file, loop=False, rootpath="/sd"):

Then the open code "could" be something like:

self.wav = open(roopath.rstrip("/") + "/" + wav_file.lstrip("/"), "rb")

This way it shouldn't break existing code, but allows more devices to use the library without modifying underlying code.

Also, the library should probably check if the file exists instead of throwing a cryptic difficult to find and resolve error.

Thanks for submitting this issue. It's a very good point your raise. I assumed that everyone will be using an SD card to store the wav files. Obviously not a good assumption. I'm presently updating all the examples to support the rPi Pico. At the same time I'll fix this problem.

A question: What do you think about adding the rootpath argument to the constructor, rather than the play method? I imagine that users will either have the wav files on a SD card, or internal flash, but not both. If you have a SD card there seems to be no reason to store wav files in internal flash.

e.g. for wav files in internal flash

wp = WavPlayer(id=I2S_ID, 
                   sck_pin=Pin(SCK_PIN), 
                   ws_pin=Pin(WS_PIN), 
                   sd_pin=Pin(SD_PIN), 
                   ibuf=BUFFER_LENGTH_IN_BYTES,
                   rootpath="/")

This issue should be fixed by #9. I should be submitting this PR tomorrow, after I've submitted the PR for I2S on the main uPy repo.

This issue has been fixed

A question: What do you think about adding the rootpath argument to the constructor, rather than the play method?

Yes. I would think this would be fine and actually makes more sense.

So, tanget time thinking out loud... The only case I can think of where it wouldn't make sense is if we think about the source as being a stream buffer (which come to think of it would be really nice option to have). Then you would want to pass in the stream on the fly. It would allow things like changing "stations" and sources/destinations dynamically. How would you do a live stream router, distribution node, or load balancer? Hm, but then why wouldn't I just create multiple objects and stream between them..... Probably only useful in routing multiple streams because buffer copying would be slow.

Yeah. Right now I can't think of a good reason not just keep it and define it in constructor.

I imagine that users will either have the wav files on a SD card, or internal flash, but not both. If you have a SD card there seems to be no reason to store wav files in internal flash.

:-) Don't know that I would make that assumption when people are already creating peta byte disk arrays for their PI's. I think a lot of this could just be solved by targeting stream buffers in the future.

Maybe during construction, just pass in a thread safe circular buffer?

Oh, and thanks so much for working on this. It has been a blessing.