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

[Mgr. Pavel Chromy]

Hello,

Monday, April 7, 2003, 8:15:51 PM Harald wrote:

> 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.

Ok.

> ============================================================
> VsPlayerFeed:

> //!!this function is really buggy - totally remade
> //##Generally I don't think so. :-)

> 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.

My fault, I missed that.

> loop_until_bit_is_set(SPSR, SPIF);
> //!!there is not reasonable purpose to do this here, this line should be 
> rather in VsSdiPutByte(), 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.

It is surely faster, but otherwise this solution is pure evil - for example
look at line 292 (decoder internal buffer flushing). Pretty tight loop
calling VsSdiPutByte() without waiting for SPIF bit!
If the speed is the issue, then better idea is to test SPIF at the beginning
of VsSdiPutByte() - simple trick providing the same speed, this just requires setting SPIF
in VsPlayerInit() by sending one zero byte.

> //## What do you mean, we never clear the SPIF bit. It is cleared
> after SPDR has sent the byte, isn't it?

Ooops, my mistake.
AVR Datasheet:
SPIF is cleared by hardware when executing the corresponding interrupt handling vector.
Alternatively, the SPIF bit is cleared by first reading the SPI Status
Register with SPIF set, then accessing the SPI Data Register (SPDR).

if (vs_datacnt >>= 32) {
> //!!we never output all of the data unless it is 32-bytes aligned ?!?
> //## I do not understand this comment.

Ignore this comment, I missed the VsSdiPutByte in the
for(;;) loop.

> 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.

Well, that's only half-true - see below.

> 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.

VS Datasheet:
The DREQ signal of the data interface is used in slave mode to signal if VS1001k’s FIFO is capable of
receiving more input data. If DREQ is high, VS1001k can take at least 32 bytes of data. When there is
less than 32 bytes of free space, DREQ is turned low, and the sender should stop transferring new data.
Because of the 32-byte safety area, the sender may send upto 32 bytes of data at a time without checking
the status of DREQ, making controlling VS1001k easier for low-speed microcontrollers.

That is: After DREQ goes low, there are less than 32 free bytes in the buffer,
(31 is a safe number in our case, since DREQ is tested with each
byte). The code works just because the decoder frees extra byte
in the meantime, which may not be true for lower bitrates.

> ============================================================
> 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.

Well, it is, but it is not in that rare case when mp3 is shorter that
the decoder buffer, because for(;;) loop is exited upon transferring last byte
from the main buffef to the decoder, which is really not a big issue.

But there is still a bug since vs_inplay is never cleared - just try to
search the source code. It's there only twice.

Moreover, in future extension, it prevents using VsPlayerKick() to
resume stopped playback, since it would insert zeros in the middle of
the stream - no assumption about how the decoder works inside should
be made. (Does the decoder discard incomplete frame at the end of the stream or
does it rather wait for rest of it? This code assumes the first case.)

> 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.

Sure, but would not be a good idea to do this at the end of the
stream (which is surely before activating reset bit)?
Then we can tell, yes, the stream has been completely played as soon
as it really has. But much more important is to do this in
interrupt avoiding loop_until_bit_is_set(PINE, 6).
Using such senteces for testing status of unreliable device is really
bad idea - it may hang! (I consider the decoder to be such as device, since it
may fail e.g. on corrupted stream.)

I think the main problem with the source is lack of it's transparency
- that's why there are so many small 'bugs'.

Pavel