From ce537e1373e86eb6f6040e2c392e9a10dbe0f019 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20S=C3=B6lver?= <henrik.solver@gmail.com>
Date: Sat, 25 Feb 2023 11:08:53 +0100
Subject: [PATCH] Improve error handling

---
 client.go      | 36 ++++++++++++++++++++++++++++--------
 client_test.go | 27 +++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/client.go b/client.go
index 965ce07..55954ed 100644
--- a/client.go
+++ b/client.go
@@ -35,9 +35,18 @@ func (m *Mbclient) closer() {
 
 	<-m.t.C
 	m.conn.Close()
+	m.transactionCounter = 0
 	m.wg.Done()
 }
 
+// closeConn can be called when a tcp communication failure require the current
+// connection to be closed, it should only be used when closer routine is running
+// and the timer is stopped.
+func (m *Mbclient) closeConn() {
+	m.t.Reset(0)
+	m.wg.Wait()
+}
+
 func (m *Mbclient) ReadRegisters(first uint16, numRegs uint16) ([]uint16, error) {
 	var err error
 	// If The timer is expired, conn is closed and needs to be reopened
@@ -52,7 +61,6 @@ func (m *Mbclient) ReadRegisters(first uint16, numRegs uint16) ([]uint16, error)
 		m.wg.Add(1)
 		go m.closer()
 	}
-	defer m.t.Reset(m.keepAliveDuration)
 
 	const requestLength = 12
 	m.transactionCounter++
@@ -70,31 +78,47 @@ func (m *Mbclient) ReadRegisters(first uint16, numRegs uint16) ([]uint16, error)
 	m.conn.SetDeadline(time.Now().Add(5 * time.Second))
 	byteswritten, err := m.conn.Write(req)
 	if err != nil {
+		m.closeConn()
 		return nil, err
 	}
 	if byteswritten != requestLength {
+		m.closeConn()
 		return nil, fmt.Errorf("failed to send request")
 	}
 	m.conn.SetDeadline(time.Now().Add(5 * time.Second))
 	_, err = io.ReadFull(m.conn, m.header[:])
 	if err != nil {
+		m.closeConn()
 		return nil, err
 	}
 	responseHeader.unMarshal(m.header)
 	expectedDataLength := responseHeader.length - 1
 
+	if m.transactionCounter != responseHeader.transactionID {
+		m.t.Reset(0)
+		return nil, fmt.Errorf("modbus transaction mismatch %v != %v", m.transactionCounter, responseHeader.transactionID)
+	}
 	response := make([]byte, expectedDataLength)
-	_, err = m.conn.Read(response)
+	bytesRead, err := io.ReadFull(m.conn, response)
 	if err != nil {
+		m.closeConn()
 		return nil, err
 	}
+	if uint16(bytesRead) != expectedDataLength {
+		m.t.Reset(m.keepAliveDuration)
+		return nil, fmt.Errorf("failed to read complete package")
+	}
+
 	err = mbpayload.unMarshal(response)
 	if mbpayload.functionCode != 3 {
-		return nil, fmt.Errorf("modbus exception %v", mbpayload.functionCode&0x7F)
+		m.t.Reset(m.keepAliveDuration)
+		return nil, fmt.Errorf("modbus exception %v req: %v", mbpayload.functionCode&0x7F, req)
 	}
 	if err != nil {
+		m.t.Reset(m.keepAliveDuration)
 		return nil, err
 	}
+
 	m.t.Reset(m.keepAliveDuration)
 	return mbpayload.registers, nil
 }
@@ -106,10 +130,6 @@ type mbapHeader struct {
 	unitIdentifier     byte
 }
 
-// func (m *mbapHeader) marshalBinary() ([]byte, error) {
-
-// }
-
 func (m *mbapHeader) unMarshal(data [7]byte) error {
 	m.transactionID = binary.BigEndian.Uint16(data[0:2])
 	m.protocolIdentifier = binary.BigEndian.Uint16(data[2:4])
@@ -129,7 +149,7 @@ func (d *mbPDU) unMarshal(data []byte) error {
 	d.functionCode = data[0]
 	d.length = data[1]
 	if d.length+2 != uint8(len(data)) {
-		return fmt.Errorf("lenght mismatch in modbus payload")
+		return fmt.Errorf("length mismatch in modbus payload")
 	}
 	d.registers = make([]uint16, d.length/2)
 	var n uint8
diff --git a/client_test.go b/client_test.go
index 24d8098..fe0253c 100644
--- a/client_test.go
+++ b/client_test.go
@@ -66,3 +66,30 @@ func TestReadOneRegisterShortKeepAlive(t *testing.T) {
 	}
 	time.Sleep(1 * time.Second)
 }
+
+func TestReadALot(t *testing.T) {
+	c, err := New("IAM_248000012514.solver.nu:502", 1, 100*time.Millisecond)
+	t.Log("Connect")
+	assert.NoError(t, err)
+	for n := 0; n < 500; n++ {
+		t.Log(n)
+		_, err := c.ReadRegisters(12401, 2)
+		if err != nil {
+			t.Log(err)
+		}
+		_, err = c.ReadRegisters(12102, 2)
+		if err != nil {
+			t.Log(err)
+		}
+
+		_, err = c.ReadRegisters(12544, 1)
+		if err != nil {
+			t.Log(err)
+		}
+		_, err = c.ReadRegisters(12136, 1)
+		if err != nil {
+			t.Log(err)
+		}
+	}
+	time.Sleep(200 * time.Millisecond)
+}