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

RFC Any interest in collaborating? #11

Open
peterhinch opened this issue May 1, 2020 · 21 comments
Open

RFC Any interest in collaborating? #11

peterhinch opened this issue May 1, 2020 · 21 comments
Labels

Comments

@peterhinch
Copy link

peterhinch commented May 1, 2020

I see this code is declared as not working for playback of music files.

I am developing an asynchronous VS1053 driver under MicroPython, based on your code. So far I have tested with MP3 files at 128kbps. It produces glitch-free music. By my calculations there should be plenty of headroom for higher bit rates.

I provide methods to read and write to the I/O pins and cancellation of a playing stream. These have tested OK.

I have the SD card working, but at present playback from the SD card fails: there is a concurrency issue which I haven't yet identified.

My solution differs from yours to support asynchronous behaviour and differences in coding style but I believe there is scope for sharing observations and ideas.

To kick things off I think there is a bug here with the indices. It should read

return (self._SCI_SPI_BUFFER[2] << 8) | self._SCI_SPI_BUFFER[3]

A significant omission addressed by my driver is the sequence at the end of playback of a track. I would be interested in discussing the strategy for altering the SPI baudrate - my reading of the datasheet suggests a different approach (which works on my hardware instance).

@ladyada
Copy link
Member

ladyada commented May 1, 2020

hi we ended up implementing native MP3 decoding instead. if you find a bug please submit a PR :) we think maybe with samd51 boards one would be able to play mp3's off of QSPI memory

@peterhinch
Copy link
Author

If this driver is abandoned there seems little point in a PR. The code comments declare that it doesn't actually work so why is it available as a public repo? Someone rocked up in the MicroPython forum wondering why they were having trouble with it!

@ladyada
Copy link
Member

ladyada commented May 3, 2020

@peterhinch hi it isnt abandoned - when it was written no chip was fast enough. maybe it is now :) please submit a PR and we can collaboratively write a driver!

@ladyada ladyada reopened this May 3, 2020
@peterhinch
Copy link
Author

peterhinch commented May 4, 2020

Sorry, I misunderstood. PR #1 now raised.

Re chip speed: I am testing with a Pyboard D and have run with MP3's at 256KiB/s. My code is cross-platform. I expect it to work with ESP32 and ESP8266 and will test soon. I don't think working with FLAC or WAV files stands a chance on any platform, especially with the onboard SD card, owing to SPI bus speed.

Re design: My driver is asynchronous, using the new uasyncio. I think MicroPython users will want to do other things while an MP3 is playing (such as run a GUI). Perhaps crucially, cancelling playback is problematic in a synchronous design. Inevitably my solution requires more user experience compared to your design. Do you intend to stay with a synchronous solution?

Re collaboration: I plan to post my code in the next few days. I think the best way forward after that would be for me to write a short report detailing my findings and pointing to features I have implemented which you could copy/adapt. Should I post the report here or would you prefer some other means?

@peterhinch
Copy link
Author

Two issues related to this project.

I raised this PR against official sdcard.py. Your breakout's SD card slot shares the SPI bus with the VS1053; it turned out that sdcard.py doesn't like this. The fix was simple, but entirely ad hoc, being based on copying existing code rather than on knowledge of the protocol.

If you or anyone on your team has an understanding of SD cards and could review this PR I'd be very grateful. I'd also be interested in engaging in a discussion on SPI baudrates. The official driver uses 1.32MHz and I'd like to know the justification for this low rate.

@peterhinch
Copy link
Author

@ladyada To kickstart this I've posted code and docs. My report is in the temp directory.

@ladyada
Copy link
Member

ladyada commented May 5, 2020

awesome! thankx :)

@peterhinch
Copy link
Author

I wanted to get something working on ESPx so I produced a synchronous version. This works on ESP8266 and ESP32.

You may wish to use this as a starting point for porting to CircuitPython.

A bug in the async version that caused some hosts to throw "No VS1053 device found" exceptions is now fixed.

@ladyada
Copy link
Member

ladyada commented May 6, 2020

nice work!

@ladyada
Copy link
Member

ladyada commented May 7, 2020

@peterhinch regarding your question about our native mp3 playback synchronicity - mp3 playback is of course done in the background on interrupts, and can be looped or piped into a mixer with other mp3s.

@peterhinch
Copy link
Author

@ladyada Thanks. I doubt that approach is feasible in Python. A 128Kbps MP3 would generate an interrupt every 1.95ms. In the case of a VBR file it would be under 1ms. This might work on a Pyboard. The high ESPx interrupt latency and the fact that they now only support soft IRQ's would surely preclude it.

The design of the chip's DREQ signal is poor, resulting in a needlessly high IRQ rate. Neither of my drivers use interrupts, polling the pin (via select in the case of uasyncio).

For CircuitPython a synchronous driver is probably your best bet. The asynchronous one works only on Pyboards and even then won't support FLAC files. It may improve if Damien introduces an option to prioritise I/O, but the timings are tight. A uasyncio application would need careful design.

I appreciate you guys are busy, but when someone is free I'd appreciate advice from a developer with knowledge of SD cards. To recap I have two questions.

  • The official driver sets the SPI bit rate to 1.32MHz. My reading (and testing) indicates that modern cards work fine at 10MHz. What advice can you offer?
  • Review of this PR on bus sharing would be welcome. On initialisation the official driver sends >100 clocks with CS False. Why is this necessary? Should it to be repeated prior to each SD card access when the bus is shared? In my testing a single byte suffices. This is empirical hackery and I'd like to have some grasp of what I'm doing :)

@jepler
Copy link
Member

jepler commented May 8, 2020

@peterhinch I recently looked into the 1.32MHz figure and couldn't find any comment or commit message that explained why it was chosen. In the jeplayer mp3 playing app for CircuitPython I ended up using 12MHz, but it was not tested over a range of cards. The device it runs on, a pygamer, has a micro SD slot soldered to the PCB with relatively short traces. And it relies on software decoding rather than the VS1053. I don't have any experience with VS1053.

Some adafruit guides show connecting an SD breakout via solderless breadboard and jumper wires, it's possible (even likely?) that a relatively low rate is required in such circumstances.

I do not understand your figure behind interrupts every 1.95ms. It's been a bit of time since I was working with the mp3 decoding part, but I think that if you have a 44.1kHz MP3 file then you need to decode one 1152-sample frame every ~26ms; if the compression rate is 256kbit then you need to read a 512 byte block from SD on average once per 16ms. Perhaps this 1.95ms figure pertains to the VS1053, not to software decoding?

You mentioned problems sharing the SPI bus. In CircuitPython, we are aware of a problem where an SPI bus used by a background task can't be used by a foreground task or you get deadlocks. This may be what you've run into? At present we don't have a solution, except advising users that when a background process (displayio or audioio in our case) may require use of an SPI bus, no foreground code is permitted to use it. adafruit/circuitpython#2417 With async code you'd have similar problems, since anything running async would be similar to a "background" task in our parlance. However, I do believe that in CircuitPython sharing the SPI bus among multiple devices does work when only foreground or only background code is involved.

@ladyada
Copy link
Member

ladyada commented May 8, 2020

SD cards dont come up in SPI mode by default, its been many years since i wrote a low level driver, but you have to do some 'tricks' to convince the SD card to move from SDIO mode to SPI mode

@ladyada
Copy link
Member

ladyada commented May 8, 2020

fyi the best library we've seen for SD card wrangling is
https://github.com/greiman/SdFat

@tannewt
Copy link
Member

tannewt commented May 8, 2020

I did a bunch of work on the Python sdcard library years ago. I don't remember exactly why everything was done but I was reading the public "Simplified Spec", looking through SdFat and testing with a variety of cards.

I'd warn against simplifying the code because it works with one card. It's a game of whack-a-mole that never ends and simplifying the code will cause other cards to fail.

I recommend getting a variety of size classes of cards (sd, sdhc, sduc, sdxc) and speed "class" for testing.

@ladyada
Copy link
Member

ladyada commented May 8, 2020

different brands too - kingston, sandisk, generic... they all act a little differently!

@peterhinch
Copy link
Author

Thanks for the feedback. There are two issues, SD cards and interrupts.

SD cards

The aim is to support the SD card connector on the Adafruit breakout for music playback. I intended to use the official driver unchanged, altering the baudrate in the driver after the SD card was mounted.

@tannewt @ladyada My changes are the minimum to make it work but I'd appreciate review from someone who understands the theory. The evident need (in the official driver) to clock the bus with CS False is unfamiliar; SD cards seem choosy about what happens when CS is False and I'd like to understand this.

My issues are as follows.

Baudrate

Ideally this would be up to 10.752MHz. In a media application it seems reasonable to specify a certain class of card. Can we define this so as to ensure operation at this rate? @jepler I take the point about wiring: this may indeed be the reason for the official value of 1.32MHz. FWIW my tests are on a soldeless breadboard with standard jumpers.

Timeouts

The official driver has this timeout used in four places. In particular the one here caused occasional problems. I had to increase this presumably as a consequence of the baudrate. I would like to know what's going on here as the official code has timing critically dependent on baudrate.

Bus sharing

@jepler I suspect that the problem you describe is a different one, firstly because there is no true concurrency in my code and secondly because it is solved by a minor fix to the SD card driver. This line and similar in .writeblocks solves it. In this comment to my PR @tve pointed me at this link. This backs up my approach, but I would like to know if it is correct and can be expected to work across card types.

Interrupts

My aim here is to justify the use of polling.

This is down to the hardware design of the VS1053b and the poor performance of Espressif hosts in terms of interrupt handling.

The VS1053b has a 2048KiB FIFO. The only access to the state of the FIFO is the DREQ pin, which is asserted whenever the device can accept 32 bytes of data. The choice for flow control is between polling this pin or using an IRQ.

@jepler We can ignore the mechanics of MP3 decoding. The drivers take bytes from a stream and send them to the VS1053. With a 128Kbps MP3 stream we are handling 16K bytes/s, with 32 bytes being sent every 1.95ms. If an ISR "immediately" feeds data to the device, DREQ is rapidly deasserted. It will be reasserted 1.95ms later. Note that this period reduces to <1ms for 256Kbps or VBR.

There are two issues here, firstly the interrupt latency of ESPx and secondly the fact that the platforms (now) only support soft IRQ's. I think the overhead of trying to support interrupts at these rates would overwhelm the Python VM.

My view is that, in a cross-platform solution, interrupts are a non starter.

@jepler
Copy link
Member

jepler commented May 9, 2020

The evident need (in the official driver) to clock the bus with CS False is unfamiliar; SD cards seem choosy about what happens when CS is False and I'd like to understand this.

From "Considerations on multi-slave Configuration" at http://elm-chan.org/docs/mmc/mmc_e.html#spibus

In the SPI bus, each slave device is selected with separated CS signals, and plural devices can be attached to an SPI bus. Generic SPI slave device enables/disables its DO output by CS signal asynchronously to share an SPI bus. However MMC/SDC enables/disables the DO output in synchronising to the SCLK. This means there is a posibility of bus conflict with MMC/SDC and another SPI slave that shares an SPI bus. Right image shows the MISO line drive/release timing of the MMC/SDC (the DO signal is pulled to 1/2 vcc to see the bus state). Therefore to make MMC/SDC release the MISO line, the master device needs to send a byte after the CS signal is deasserted.

There is an important thing needs to be considered that the MMC/SDC is initially NOT the SPI device. Some bus activity to access another SPI device can cause a bus conflict due to an accidental response of the MMC/SDC. Therefore the MMC/SDC should be initialized to put it into the SPI mode prior to access any other device attached to the same SPI bus.
image

@peterhinch
Copy link
Author

peterhinch commented May 10, 2020

@jepler Thanks for that: I now see why the official driver writes 0xff with CS False after each transfer. I think that issue is covered in the official code.

I find it necessary to write 0xff with CS False prior to accessing the card. Why is this necessary?
[EDIT1]
I wondered if the VS1053b might have a similar issue but the datasheet indicates that it releases the bus asynchronously, soon after its CS is deasserted.

What I find odd is this. Accessing the VS1053 involves SCK cycles which occur with the SD card's CS False. MOSI and MISO will carry arbitrary data while this occurs. This process causes subsequent SD card access to fail. This is fixed, prior to access, by sending a single byte of 0xff with the SD card's CS False.
[EDIT2]
I adapted the script in PR6007 to run on a PCB where the SPI bus is not shared: this eliminates any issue with the VS1053b. Bus sharing is simulated by writing 0 with the SD card's CS line False. This is enough to trigger the fault.

from machine import SPI, Pin
import sdcard
import os
spi = SPI(1)
sdcs = Pin('X21', Pin.OUT, value=1)  # SD card CS
sd = sdcard.SDCard(spi, sdcs)
vfs = os.VfsFat(sd)
os.mount(vfs, '/fc')
os.listdir('/fc')
spi.write(b'\x00')  # Write to a hypothetical device
with open('/fc/yellow.mp3', 'rb') as f:  # ENOENT with official driver
    f.read(20)  # Works with modified driver

If sdcard.py sends 0xff at the start of .readblocks the script works.

@peterhinch
Copy link
Author

The synchronous driver now supports recording IMA ADPCM .wav files.

@peterhinch
Copy link
Author

I've taken this as far as I can: barring bugfixes my drivers are complete.

If you guys find time I'd appreciate feedback on the SPI issues. The bug discussed above is likely to affect other products where the bus is shared between an SD card and a chip.

My PR is still pending, probably because my fix is ad hoc. If you can report why it works (or if it needs amending), either here or in response to the PR, we might end up with a working upstream driver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants