[En-Nut-Discussion] Contributions: vs1001 and microdelay

[Harald Kipp]

Pavel,

In general, definetly vs1001k.c contains major bugs, which
makes it working for very limited apps only. I got the strange
feeling, that something went miserably wrong. The shout demo
was the only app, that has been tested, as we use a different
driver in our commercial apps.

Anyway it cries for an update and I appreciate your time and
effort very much. But I'd like to discuss some issues. Your
comments were marked as //!! mine are //##

Harald

============================================================
VsRegWrite:

NutEnterCritical(); //!!is it really necessary to disable all interrupts? 
see VsEnterCritical() in new source

//## Agree. Just a minor comment: I usually avoid doing
these kind of redefinitions. If you are new to the code
you waste time to check, what these functions might do.
I'd recommend to define the constant number itself and
write cbi(EIMSK, VS_DREQ_BIT). IMHO that's easier to read.

============================================================
VsPlayerFeed:

//!!this function is really buggy - totally remade

//##Generally I don't think so. :-)

if (vs_failed || bit_is_clear(PINE, 6)) {
//!!why vs_failed is tested here, while it is set only on line 217 along 
with disabling the interrupt?
...
vs_running = 0; //!!we most likely pass through here only when called in 
non-interrupt context

//## Not really useless. Intention was to find other indications
for a failing decoder and set vs_failed. One would be to check the
number of interrupts within a certain period. However, this code
never made it into the distribution.

vs_running = 1; //!!we most likely NEVER pass through here - see line 215 
and 219
//## We would, because the satement is in a for() loop. vs_running
will be set, if enough bytes had been pushed into the decoder, so it
releases its DREQ line.

loop_until_bit_is_set(SPSR, SPIF);
//!!there is not reasonable purpose to do this here, this line should be 
rather in VsSdiP
utByte(), besides we NEVER clear the SPIF bit!

//## The reason _not_ to do it in VsSdiPutByte is speed. It fills the
SPDR, executes some other statements and then checks SPIF. This is faster
then polling SPIF first and execute the other statements after.
//## What do you mean, we never clear the SPIF bit. It is cleared
after SPDR has sent the byte, isn't it?

if (vs_datacnt >= 32) {
//!!we never output all of the data unless it is 32-bytes aligned ?!?

//## I do not understand this comment. Let me explain the purpose of
the code: The upper loop will stop as soon as DREQ is released. This
means that there are still 32 bytes available in the decoder buffer.
If 32 bytes (or more) are left in our memory buffer, we push an additional
32 bytes into the decoder to keep it busy a bit longer. In addition
the last 32 byte loop is extremely fast, much faster than the main
loop, so its worth the extra code.

============================================================
VsPlayerKick:

for (i = 0; i < 2048; i++) {
//!!it'd be better to do this in interrupt, besides vs_inplay is most 
likely NEVER set (see abo
ve) and moreover the empty block should be sent at the END of playback, not 
at the beginning

//## vs_inplay _is_ set, see above.
//## The datasheet explains: Between any two MP3 files, the decoder 
software has to be reset.
This is done by ...blabla...Then wait for at least 2 ¹s, then look at 
DREQ...blabla...When DREQ goes up,
write at least one zero to SDI. After this, you may continue playback as 
usual. If you want to make sure
VS1001k doesn’t cut the ending of low-bitrate data streams, it is 
recommended to feed 2048 zeros to the
SDI bus before activating the reset bit (DREQ must be respected just as 
with normal SDI data). This will
make sure all frames have been decoded before resetting the chip.

*(vs_readptr + 1) != 0xFF &&
//!!serious bug - this may cross the buffer boundary
//## It's not that serious as it looks like. :-) It will fail if
the MP3 header is exactly at the boundary, in which case it loses
a few milliseconds of music while looking for a new header. The
read pointer itself isn't incremented pass the boundary.

============================================================
END