WRAM double read is only needed for 32-bit values

[dewhisna]

I don't think it's necessary for this vs1053::Mp3ReadWRAM function to read the WRAM value twice. I assume the reason it's being done is because of section 10.11 in the datasheet, which reads:

Notice that reading two-word variables through the SCI_WRAMADDR and SCI_WRAM interface is not protected in any way. The variable can be updated between the read of the low and high parts. The problem arises when both the low and high parts change values. To determine if the value is correct, you should read the value twice and compare the results.

However, that paragraph is talking about reading 32-bit values (i.e. two-word variables), not a single 16-bit value. Their example, even, is illustrating the read of bytesLeft, which is a 32-bit value.

That makes sense, because there's nothing that keeps one 16-bit half of the value from changing while the code is reading the other half, but as I understand from the datasheet, an individual 16-bit value is latched during the read for the transfer to the SPI bus and is sent consistently as a unit, meaning that its upper and lower bytes should be correct paired.

It is possible for a value to change between consecutive reads, but the caller of this function will need to determine if that change affects its reading of a larger 32-bit value or not, and since this function is only for reading a single 16-bit location, I don't think it can determine that since it wouldn't know if the caller is even doing a 32-bit read.

If I'm wrong, please point me to the part of the datasheet that I missed.

Arduino_Library-vs1053_for_SdFat/src/vs1053_SdFat.cpp

Lines 1899 to 1932 in 11cf5fc

	/**
	* \brief Read a VS10xx WRAM Location
	*
	* \param[in] addressbyte of the VSdsp's WRAM to be read
	* \return result read from the WRAM
	*
	* Function to communicate to the VSdsp's registers, indirectly accessing the WRAM.
	* As per data sheet the result is read back twice to verify. As it is not buffered.
	*/
	uint16_t vs1053::Mp3ReadWRAM (uint16_t addressbyte){
	unsigned short int tmp1,tmp2;
	//Set SPI bus for write
	spiInit();
	SPI.setClockDivider(spi_Read_Rate);
	Mp3WriteRegister(SCI_WRAMADDR, addressbyte);
	tmp1 = Mp3ReadRegister(SCI_WRAM);
	Mp3WriteRegister(SCI_WRAMADDR, addressbyte);
	tmp2 = Mp3ReadRegister(SCI_WRAM);
	if(tmp1==tmp2) return tmp1;
	Mp3WriteRegister(SCI_WRAMADDR, addressbyte);
	tmp2 = Mp3ReadRegister(SCI_WRAM);
	if(tmp1==tmp2) return tmp1;
	Mp3WriteRegister(SCI_WRAMADDR, addressbyte);
	tmp2 = Mp3ReadRegister(SCI_WRAM);
	if(tmp1==tmp2) return tmp1;
	return tmp1;
	}

[mpflaga]

Your statement sounds good.
It has been 5 years since I any significant going over. So, I do not recall any specifics. or issues.
But a quick look over, does seem to point out that all the reads, and given the response type is the uint16_t.
I do recall, at that time I was being overly cautious about double byte words and alike, from other codes that I had inherited. I had additional issues with other interrupt driven events breaking up bytes and such, leading to over caution on this subject.

It could also have been I was thinking of 8bit words or something. like that. I see from the Data sheet
W Word. In VS_DSP, instruction words are 32-bit and data words are 16-bit wide.

I do not think there is much of a performance hit, as I recall Mp3ReadWRAM() is not commonly called.

[dewhisna]

Yeah, I think the only 32-bit value even listed in the parametric addresses in your header file is para_positionMsec_0 and para_positionMsec_1, and I don't see it being used anywhere in the code -- only mentioned in the docs.

If code were to use that, I think the correct read method is to read the MSWord first, then read the LSWord, then reread the MSWord again. If the second read matches the first, you know the LSWord didn't rollover in the middle of the read and you can keep the MSWord/LSWord pair you have. But, if they don't match, reread the LSWord and use it with the second MSWord read, because you know the LSWord rolled over between the two reads of MSWord, but you don't know if it was before or after the first LSWord read.

But, that 32-bit value doesn't seem to be read anywhere in the code. And like you say, the extra overhead of the extra reads of the 16-bit values is minimal, but I think it could be removed.

I'm not actually using this repo code directly. I'm actually working on a very different project involving a STM32F446 main processor, a Spartan 6 FPGA, and a VS1053 as a DSP coprocessor/codec for driving Musical Tesla Coils. While working on its code, I was searching the web for existing projects using the VS1053 to get a feel for its subtleties and saw the double-reads here and so I was trying to figure out if they were really necessary and if perhaps I missed something in the datasheet.

But, not finding anything in the datasheet to support it, I figured I'd open an issue to both verify directly and let you know that I think that function can be greatly simplified.

[mpflaga]

Thank you for your detailed investigation.
I am glad you found this repo helpful and best wishes on your project.

Back then there were a lot of support requests on the original fork. A great many were on a forum that bill porter spun up. Not sure if that is still working though. http://www.billporter.info/forum/search/VS1053/. So I added all the doxgen to self-document itself. If you have not already found it, it may be helpful GitHub project page.

I spent a lot of time reviewing all the VSDSP Forum VS1053. I am sure there will be many since.