From 9ae0da667b4c9866d417357d237c84ca27db0765 Mon Sep 17 00:00:00 2001
From: Michal Sieron <msieron@antmicro.com>
Date: Thu, 10 Nov 2022 15:46:24 +0100
Subject: [PATCH] remote/comm_udp: Fix Etherbone timeout errors

This commit fixes a bug where, retry after timeout would send another
request, but wouldn't discard a response that arrived after the timeout.

Retries worked, but only for dropped packets. If a response arrived, but
`socker.recvfrom` timed out, response would still be put in receive
queue. Later after sending another request, client would try to read
from the socket and would find a response. But this response would be
for the old request.
This way request/response pairs would get misaligned and stop working
properly.

This commit adds read numbering (writes do not have responses).
Numbering is achieved by utilizing the fact that responses to Etherbone
reads are actually writes to an address specified in a request.
This way, we don't need to extend Etherbone protocol, in fact we use it
as it is intended.

This numbering is then used to discard responses that don't match
current request.

I also cleaned setting of the timeout, as it was being set in multiple
places, sometimes to values so small that retry was bound to happen.

Signed-off-by: Michal Sieron <msieron@antmicro.com>
---
 litex/tools/remote/comm_udp.py | 60 +++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/litex/tools/remote/comm_udp.py b/litex/tools/remote/comm_udp.py
index 390d0052d..e282a6607 100644
--- a/litex/tools/remote/comm_udp.py
+++ b/litex/tools/remote/comm_udp.py
@@ -22,13 +22,14 @@ class CommUDP(CSRBuilder):
         self.port   = port
         self.debug  = debug
         self.timeout= timeout
+        self.read_counter = 0
 
     def open(self, probe=True):
         if hasattr(self, "socket"):
             return
         self.socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
         self.socket.bind(("", self.port))
-        self.socket.settimeout(2)
+        self.socket.settimeout(self.timeout)
         if probe:
             self.probe(self.server, self.port)
 
@@ -46,7 +47,6 @@ class CommUDP(CSRBuilder):
         packet.bytes += bytes([0x00, 0x00, 0x00, 0x00]) # Add Padding as payload.
 
         retries = 10
-        self.socket.settimeout(self.timeout / retries)
 
         # Send probe request to server and get server's response.
         for r in range(retries):
@@ -76,44 +76,60 @@ class CommUDP(CSRBuilder):
     def scan(self, ip="192.168.1.x"):
         print(f"Etherbone scan on {ip} network:")
         ip = ip.replace("x", "{}")
-        self.socket.settimeout(0.01)
         for i in range(1, 255):
             if self.probe(ip=ip.format(str(i)), port=self.port, loose=True):
                 print("- {}".format(ip.format(i)))
-        self.socket.settimeout(1)
 
     def read(self, addr, length=None, burst="incr"):
         assert burst == "incr"
         length_int = 1 if length is None else length
-        record = EtherboneRecord()
-        record.reads = EtherboneReads(addrs=[addr+4*j for j in range(length_int)])
-        record.rcount = len(record.reads)
-
-        packet = EtherbonePacket()
-        packet.records = [record]
-        packet.encode()
 
         retries = 10
 
-        self.socket.settimeout(self.timeout / retries)
         for r in range(retries):
+            self.read_counter += 1
+
+            record = EtherboneRecord()
+            record.reads = EtherboneReads(addrs=[addr+4*j for j in range(length_int)])
+            record.rcount = len(record.reads)
+            record.reads.base_ret_addr = self.read_counter
+
+            packet = EtherbonePacket()
+            packet.records = [record]
+            packet.encode()
+
             self.socket.sendto(packet.bytes, (self.server, self.port))
-            try:
-                datas, dummy = self.socket.recvfrom(8192)
-            except socket.timeout:
-                if self.debug:
-                    print("socket timeout, retrying ({}/{})".format(r+1, retries))
-                continue
-            break
+
+            timed_out = False
+            while True:
+                try:
+                    datas, dummy = self.socket.recvfrom(8192)
+                except socket.timeout:
+                    if self.debug:
+                        print("socket timeout, retrying ({}/{})".format(r+1, retries))
+                    timed_out = True
+                    break
+
+                packet = EtherbonePacket(datas)
+                packet.decode()
+                record = packet.records.pop()
+                datas = record.writes.get_datas()
+                if record.writes.base_addr == self.read_counter:
+                    break
+                else:
+                    if self.debug:
+                        print(f"WARNING: request/response id mismatch: 0x{self.read_counter:08x} != 0x{record.writes.base_addr:08x}")
+
+            if not timed_out:
+                break
+
         else:
             raise socket.timeout
 
-        packet = EtherbonePacket(datas)
-        packet.decode()
-        datas = packet.records.pop().writes.get_datas()
         if self.debug:
             for i, value in enumerate(datas):
                 print("read 0x{:08x} @ 0x{:08x}".format(value, addr + 4*i))
+
         return datas[0] if length is None else datas
 
     def write(self, addr, datas):