Re: [hatari-devel] floppy issue

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 18/02/2014 08:58, David Savinkoff a écrit :
> ----- Nicolas Pomarède wrote:
> > Le 17/02/2014 03:31, David Savinkoff a écrit :
> > > ----- Thomas Huth wrote:
> >
> > > > The only related change to the DMA status that I can see is this patch
> > > > here:
> > > >
> > > >    http://hg.tuxfamily.org/mercurialroot/hatari/hatari/rev/f84eb81fa309
> > > >
> > > > Nicolas, any ideas what could be wrong here?
> > > >
> > > >    Thomas
> > >
> > > Hi,
> > >
> > > Here is a patch that assumes the DMA error is transient.
> > > EmuTOS works with this patch.
> >
> > Hello
> >
> > calling "FDC_SetDMAStatus ( false )" outside of FDC_DMA_FIFO_Pull()
> > should not be required as DMA Status is completely handled in
> > FDC_DMA_FIFO_Push() and FDC_DMA_FIFO_Pull().
> >
> > Except there was a bug in FDC_DMA_FIFO_Pull() and FDC_DMA.SectorCount
> > was set to 0 too early (when the last 16 bytes need to be transferred),
> > resulting in DMA.Status being set to "error", but also forcing the last
> > 15 bytes of each sector to 0, which would seriously damage any disk :(
> >
> > This is now fixed and hopefully no one damaged some ST/MSA images.
> >
> > Nicolas
>
> Hi Nicolas,
>
> 1) An unsigned compare for less than zero produces the compiler warning:
>
> hatari/src/fdc.c: In function ‘FDC_DMA_FIFO_Pull’:
> hatari/src/fdc.c:981: warning: comparison is always false
> due to limited range of data type

Hi, that's right, my gcc didn't report a warning for this ; I changed 
BytesInSector to signed int.


> 2) I noticed that FDC_DMA_FIFO_Push() and FDC_DMA_FIFO_Pull()
> now have different conditions for similar sections of code.
>
> FDC_DMA_FIFO_Push() has:
> if ( FDC_DMA.BytesInSector <= 0 )
> and
> FDC_DMA_FIFO_Pull() has:
> if ( FDC_DMA.BytesInSector < 0 )

Yes, conditions are slightly different because in Push, we 1st store a 
byte, then update the sector count ; and in Pull, we update sector 
count, then return a byte.
First case could be :

FDC_DMA_FIFO_Push() has:
 if ( FDC_DMA.BytesInSector == 0 )

I agree this could look confusing, but that's the way SectorCount should 
be updated.

Nicolas