Re: [hatari-devel] Falcon left-right sound swapper bug?

[David Savinkoff <dsavnkff@xxxxxxxxx>]

Hi,

This patch attempts to address what I mentioned below.
It compiles; so test it, criticize it, improve it. Thanks.

----- David Savinkoff wrote:
> ----- Eero Tamminen wrote:
> > Hi,
> > 
> > On torstai 17 huhtikuu 2014, David Savinkoff wrote:
> > > crossbar.c is the only file that has:
> > > dac.readPosition :: dac.writePosition
> > > and:
> > > adc2dac_readBufferPosition :: adc2dac_writeBufferPosition
> > > 
> > > These circular buffer indexes must check that they do not over-run each
> > > other.
> > > 
> > > dmaSnd.c maintains the circular buffer indexes with:
> > > static void DmaSnd_FIFO_Refill(void);
> > > static Sint8 DmaSnd_FIFO_PullByte(void);
> > > 
> > > crossbar.c has incomplete functions:
> > > void Crossbar_GetMicrophoneDatas(Sint16 *micro_bufferL, Sint16
> > > *micro_bufferR, Uint32 microBuffer_size); static void
> > > Crossbar_SendDataToDAC(Sint16 value, Uint16 sample_pos); void
> > > Crossbar_GenerateSamples(int nMixBufIdx, int nSamplesToGenerate);
> > 
> > There's following old bug in Hatari Crossbar DMA code:
> > http://listengine.tuxfamily.org/lists.tuxfamily.org/hatari-
> > devel/2012/02/msg00082.html
> > 
> > It doesn't check the indeces given by emulated code
> > which is really bad because emulated code can then
> > corrupt Hatari's own memory.  That might then have also
> > other effects.
> > 
> > 
> > 	- Eero
> > 
> The 'read buffer position' and 'write buffer position' pointers
> stay within the buffer's allocated space, but each are unaware
> of the others existence and can become unsynchronized.
> 
> 
> 

--- hatari/src/falcon/crossbar.c	2014-04-08 13:53:23.000000000 -0700
+++ hatari/src/falcon/crossbar.c	2014-04-17 19:09:03.000000000 -0700
@@ -268,6 +268,7 @@
 };
 
 struct codec_s {
+	int    FIFO_NbBytes;
 	Sint16 buffer_left[DACBUFFER_SIZE];
 	Sint16 buffer_right[DACBUFFER_SIZE];
 	Sint64 readPosition_float;
@@ -326,6 +327,7 @@
 	dac.readPosition_float = 0;
 	dac.readPosition = 0;
 	dac.writePosition = 0;
+	dac.FIFO_NbBytes = 0;
 
 	/* ADC inits */
 	memset(adc.buffer_left, 0, sizeof(adc.buffer_left));
@@ -333,6 +335,7 @@
 	adc.readPosition_float = 0;
 	adc.readPosition = 0;
 	adc.writePosition = 0;
+	adc.FIFO_NbBytes = 0;
 
 	/* DSP inits */
 	dspXmit.wordCount = 0;
@@ -1699,14 +1702,19 @@
 	idxPos = 0;
 
 	for (i = 0; i < size; i++) {
-		adc.writePosition = (adc.writePosition + 1) % DACBUFFER_SIZE;
-
 		adc.buffer_left[adc.writePosition] = micro_bufferL[bufferIndex];
 		adc.buffer_right[adc.writePosition] = micro_bufferR[bufferIndex]; 
 
 		idxPos += crossbar.frequence_ratio2;
 		bufferIndex += idxPos>>32;	
 		idxPos  &= 0xffffffff;			/* only keep the fractional part */
+
+		adc.writePosition = (adc.writePosition + 1) % DACBUFFER_SIZE;
+
+		if (++adc.FIFO_NbBytes > DACBUFFER_SIZE) {
+			adc.FIFO_NbBytes = DACBUFFER_SIZE;
+			adc.readPosition = adc.writePosition;
+		}
 	}
 }
 
@@ -1718,6 +1726,14 @@
 	Sint16 sample;
 	Uint16 frame;
 	
+		
+	if (--adc.FIFO_NbBytes < 0) {
+		adc.FIFO_NbBytes = 0;
+		adc.writePosition = adc.readPosition;
+		adc.buffer_left[adc.writePosition] = 0;
+		adc.buffer_right[adc.writePosition] = 0;
+	}
+
 	/* swap from left to right channel or right to left channel */
 	adc.wordCount = 1 - adc.wordCount;
 		
@@ -1770,6 +1786,11 @@
 		/* Right channel */
 		dac.buffer_right[dac.writePosition] = value;
 		dac.writePosition = (dac.writePosition + 1) % (DACBUFFER_SIZE);
+
+		if (++dac.FIFO_NbBytes > DACBUFFER_SIZE) {
+			dac.FIFO_NbBytes = DACBUFFER_SIZE;
+			dac.readPosition = dac.writePosition;
+		}
 	}
 }
 
@@ -1781,7 +1802,7 @@
 void Crossbar_GenerateSamples(int nMixBufIdx, int nSamplesToGenerate)
 {
 	int i, nBufIdx;
-	unsigned n;
+	unsigned n = 0;
 	Sint16 adc_leftData, adc_rightData, dac_LeftData, dac_RightData;
 	
 	if (crossbar.isDacMuted) {
@@ -1802,6 +1823,13 @@
 	{
 		nBufIdx = (nMixBufIdx + i) % MIXBUFFER_SIZE;
 
+		if ((dac.FIFO_NbBytes -= n) < 0) {
+			dac.FIFO_NbBytes = 0;
+			dac.writePosition = dac.readPosition;
+			dac.buffer_left[dac.writePosition] = 0;
+			dac.buffer_right[dac.writePosition] = 0;
+		}
+
 		/* ADC mixing (PSG sound or microphone sound for left and right channels) */
 		switch (crossbar.codecAdcInput) {
 			case 0:
@@ -1844,8 +1872,6 @@
 				/* Crossbar->DAC sound only */
 				dac_LeftData = dac.buffer_left[dac.readPosition];
 				dac_RightData = dac.buffer_right[dac.readPosition];
-				dac.buffer_left[dac.readPosition] = 0;
-				dac.buffer_right[dac.readPosition] = 0;
 				break;
 			case 3:
 				/* Mixing Direct ADC sound with Crossbar->DMA sound */
@@ -1853,8 +1879,6 @@
 						dac.buffer_left[dac.readPosition];
 				dac_RightData = ((adc_rightData  * crossbar.gainSettingRight) >> 14) +
 						dac.buffer_right[dac.readPosition];
-				dac.buffer_left[dac.readPosition] = 0;
-				dac.buffer_right[dac.readPosition] = 0;
 				break;
 		}