Re: [hatari-devel] (raw) MIDI connection reliability?

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 18.6.2022 16.19, Eero Tamminen wrote:
> On 18.6.2022 16.05, Eero Tamminen wrote:
> > > Anyway, since EOF is guaranteed to be negative, it shouldn't
> > > really matter much in this case, so never mind, let's just
> > > leave the code as it is.
> >
> > I think I need to update logging to check errno instead of return 
> > value.    Now it ignores all errors as they all get mapped to EOF 
> > return value.
>
> Does attached patch look OK?  I mean, can we trust on errno being set 
> (glibc fgetc manual does not say anything about errno)...

When trying to debug MIDI issues, I noticed that MIDI write side had no 
logging.  New patch adds that too, and moves error handling to more 
logical place.


	- Eero

From 815274a448904a41aac256ff50d927737d4a86ba Mon Sep 17 00:00:00 2001
From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
Date: Sun, 19 Jun 2022 10:54:44 +0300
Subject: [PATCH 1/2] Fix MIDI read error logging

It turned out I had misunderstood badly worded Glibc fgetc() manual
page.  Thomas pointed out much more precise POSIX text, which
explicitly states fgetc() to return only EOF on errors.

=> Change error logging to use errno instead and call clearerr()
   only when there's real EOF.

Fixes: 1c415e4c4b

Add similar logging also to MIDI writes.
---
 src/midi.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/midi.c b/src/midi.c
index 6c951b80..04603cb1 100644
--- a/src/midi.c
+++ b/src/midi.c
@@ -23,6 +23,7 @@
 const char Midi_fileid[] = "Hatari midi.c";
 
 #include <SDL_types.h>
+#include <errno.h>
 
 #include "main.h"
 #include "configuration.h"
@@ -243,7 +244,6 @@ void Midi_Data_ReadByte(void)
 void Midi_Data_WriteByte(void)
 {
 	Uint8 nTxDataByte;
-	bool ok;
 	
 	ACIA_AddWaitCycles ();						/* Additional cycles when accessing the ACIA */
 
@@ -274,10 +274,11 @@ void Midi_Data_WriteByte(void)
 	if (!ConfigureParams.Midi.bEnableMidi)
 		return;
 
-	ok = Midi_Host_WriteByte(nTxDataByte);
-
-	/* If there was an error then stop the midi emulation */
-	if (!ok)
+	if (Midi_Host_WriteByte(nTxDataByte))
+	{
+		LOG_TRACE(TRACE_MIDI, "MIDI: write byte -> $%x\n", nTxDataByte);
+	}
+	else
 	{
 		LOG_TRACE(TRACE_MIDI, "MIDI: write error -> stop MIDI\n");
 		Midi_UnInit();
@@ -313,9 +314,9 @@ void Midi_InterruptHandler_Update(void)
 
 	/* Read the bytes in, if we have any */
 	nInChar = Midi_Host_ReadByte();
-	if (nInChar >= 0)
+	if (nInChar != EOF)
 	{
-		LOG_TRACE(TRACE_MIDI, "MIDI: Read character -> $%x\n", nInChar);
+		LOG_TRACE(TRACE_MIDI, "MIDI: read byte -> $%x\n", nInChar);
 		/* Copy into our internal queue */
 		nRxDataByte = nInChar;
 		MidiStatusRegister |= ACIA_SR_RX_FULL;
@@ -323,14 +324,6 @@ void Midi_InterruptHandler_Update(void)
 		/* Do we need to generate a receive interrupt? */
 		MIDI_UpdateIRQ ();
 	}
-#ifndef HAVE_PORTMIDI
-	else if (pMidiFhIn)
-	{
-		if (nInChar != EOF)
-			LOG_TRACE(TRACE_MIDI, "MIDI: read error %d (does not stop MIDI)\n", nInChar);
-		clearerr(pMidiFhIn);
-	}
-#endif
 
 	/* Set timer */
 	CycInt_AddRelativeInterrupt ( MIDI_TRANSFER_BYTE_CYCLE , INT_CPU_CYCLE , INTERRUPT_MIDI );
@@ -583,8 +576,17 @@ static int Midi_Host_ReadByte(void)
 {
 #ifndef HAVE_PORTMIDI
 	if (pMidiFhIn && File_InputAvailable(pMidiFhIn))
-		return fgetc(pMidiFhIn);
-	else return EOF;
+	{
+		int ret = fgetc(pMidiFhIn);
+		if (ret != EOF)
+			return ret;
+		if (errno && errno != EAGAIN)
+		{
+			LOG_TRACE(TRACE_MIDI, "MIDI: read error: %s\n", strerror(errno));
+		}
+		clearerr(pMidiFhIn);
+	}
+	return EOF;
 #else
 	// TODO: should these be reset with Midi_Init()?
 	static Uint8 msg[4];
-- 
2.30.2