fix(rotator): one held TCP session for a DCU-1 controller

The DCU-1 client opened a connection per command and closed it again,
"mirroring the gs232/pst/rotgenius idiom". That idiom is right for the
UDP backends beside it and wrong over TCP to an embedded serial server,
which is what an RT-21's Ethernet option is: the heading is polled every
500 ms while the antenna turns, GoTo sends two commands (AP1 then AM1),
Stop sends two more — each its own connect and close. Modules of that
class commonly accept a SINGLE session and need a moment to release it,
so the churn on its own looks like a controller ignoring half of what it
is told.

The socket is now kept between calls, one mutex serialises every
exchange — which also stops the poll and an operator command from
holding two sessions at once — and a write or read error drops it so the
next call redials. One retry after a redial, because a kept socket's
first write succeeds long after the far end has gone. Serial keeps
open-per-call: a COM port has one owner, and holding it would lock out
the controller's own software.

Keeping a session only helps if the client survives the call, and
dcu1Client built a fresh one every time, so it is cached per controller
identity. Two rotors on the same box share one client, which is the
point when one session is all there is. SaveRotators drops the cache:
a client left over from the previous host would hold the very session
its replacement needs.

Three tests against a fake controller that accepts one session at a
time: four commands share one session, a dropped session is redialled
exactly once, and a failed dial leaves nothing behind.

app.go

@@ -17419,6 +17419,10 @@ func (a *App) migrateLegacyRotors() []RotatorDevice {
 // SaveRotators persists the rotor list. Connections are per-call (no socket to
 // (re)open) so no reload step is needed.
 func (a *App) SaveRotators(list []RotatorDevice) error {
+	// Any kept DCU-1 session belongs to the OLD settings. Dropped before the new
+	// ones are stored, so a stale socket cannot hold a single-session controller
+	// against its replacement.
+	defer dropDCU1Clients()
 	if a.settings == nil {
 		return fmt.Errorf("db not initialized")
 	}
@@ -17538,10 +17542,58 @@ func easycommClient(l rotorLink) *easycomm.Client {
 	return easycomm.New(l.Host, l.Port, l.MaxAz)
 }
 
+// dcu1Clients keeps ONE client per controller, because over TCP the client
+// holds its session open between commands.
+//
+// Every other rotator backend here is built fresh per call, which is right for
+// the UDP ones and wrong for a TCP embedded serial server: the heading is
+// polled twice a second while the antenna turns and GoTo sends two commands,
+// so a new client each time meant a new connect and close each time. Those
+// modules commonly accept one session at a time. Cached, the churn is gone.
+//
+// Keyed on the controller's identity, so two rotors on two boxes get one
+// client each and two rotors on the SAME box share it — sharing being the
+// point, when only one session is on offer.
+var (
+	dcu1Mu      sync.Mutex
+	dcu1Clients = map[string]*dcu1.Client{}
+)
+
+func dcu1Key(l rotorLink) string {
+	if l.Transport == "serial" {
+		return fmt.Sprintf("serial|%s|%d", strings.ToUpper(strings.TrimSpace(l.ComPort)), l.Baud)
+	}
+	return fmt.Sprintf("tcp|%s|%d", strings.ToLower(strings.TrimSpace(l.Host)), l.Port)
+}
+
+// dropDCU1Clients closes every kept session. Called when the rotator settings
+// are saved: a client left over from the previous host would hold the very
+// session the new one needs, which is worse than the churn it replaced.
+func dropDCU1Clients() {
+	dcu1Mu.Lock()
+	defer dcu1Mu.Unlock()
+	for k, c := range dcu1Clients {
+		c.Close()
+		delete(dcu1Clients, k)
+	}
+}
+
 // dcu1Client builds the Hy-Gain DCU-1 client for a rotor's transport: the
 // controller's COM port (the usual case — RotorCard DXA, Green Heron, Rotor-EZ)
-// or a serial-over-IP bridge on TCP.
+// or the controller's own Ethernet port / a serial-over-IP bridge on TCP.
 func dcu1Client(l rotorLink) *dcu1.Client {
+	key := dcu1Key(l)
+	dcu1Mu.Lock()
+	defer dcu1Mu.Unlock()
+	if c := dcu1Clients[key]; c != nil {
+		return c
+	}
+	c := newDCU1Client(l)
+	dcu1Clients[key] = c
+	return c
+}
+
+func newDCU1Client(l rotorLink) *dcu1.Client {
 	if l.Transport == "serial" {
 		return dcu1.NewSerial(l.ComPort, l.Baud)
 	}

changelog.json

@@ -14,7 +14,8 @@
       "A correction you make on the transmit VFO is now kept for the whole pass, and remembered for that transponder. Doppler tracking used to undo it a second later.",
       "The Green Heron RT-21 is named in the rotator list. It speaks the DCU-1 command set OpsLog already drives, over its COM port or straight over TCP with the Ethernet option — set the controller to DCU-1 / Rotor-EZ.",
       "Retract and Calibrate on a SteppIR now show the elements moving, and inhibit the transmitter while they do. Neither said anything before — the same was missing on an Ultrabeam retract, where the element lengths counting down hid it.",
-      "The window can be made as small as you like. The 1100x700 floor is gone — only Windows own limit remains."
+      "The window can be made as small as you like. The 1100x700 floor is gone — only Windows own limit remains.",
+      "A DCU-1 controller reached over TCP now gets one connection held open, instead of a new one for every command — twice a second while the antenna turns. Controllers that accept a single session, an RT-21 with the Ethernet option among them, could not keep up."
     ],
     "fr": [
       "Le tracé du ciel et la position passent dans leur propre colonne, à gauche de la carte. Largeur réglable, et la colonne se replie comme celle de droite.",
@@ -29,7 +30,8 @@
       "Une correction faite sur le VFO d’émission est désormais conservée pour toute la passe, et mémorisée pour ce transpondeur. Le suivi Doppler l’effaçait une seconde plus tard.",
       "Le Green Heron RT-21 est nommé dans la liste des rotators. Il parle le jeu de commandes DCU-1 que OpsLog pilote déjà, via son port COM ou directement en TCP avec l’option Ethernet — régler le contrôleur sur DCU-1 / Rotor-EZ.",
       "Rétracter et Calibrer sur une SteppIR montrent désormais les éléments en mouvement, et inhibent l’émission pendant ce temps. Ni l’un ni l’autre ne le signalait — même manque sur la rétraction d’une Ultrabeam, où le défilement des longueurs d’éléments le masquait.",
-      "La fenêtre peut être réduite autant que vous voulez. Le plancher de 1100x700 disparaît — il ne reste que la limite propre à Windows."
+      "La fenêtre peut être réduite autant que vous voulez. Le plancher de 1100x700 disparaît — il ne reste que la limite propre à Windows.",
+      "Un contrôleur DCU-1 joint en TCP reçoit désormais une seule connexion maintenue ouverte, au lieu d’une nouvelle à chaque commande — deux fois par seconde en rotation. Les contrôleurs n’acceptant qu’une session, dont un RT-21 avec l’option Ethernet, ne pouvaient pas suivre."
     ]
   },
   {

internal/rotator/dcu1/dcu1.go

@@ -22,6 +22,21 @@
 //
 // The base DCU-1 set has no dedicated stop; Stop re-commands the current bearing,
 // which halts rotation.
+//
+// ONE TCP session, held open and serialised.
+//
+// This started out opening a connection per command, like the UDP backends
+// beside it. Over TCP to an embedded serial server — which is what the RT-21's
+// Ethernet option is — that is the wrong shape: the heading is polled twice a
+// second while the antenna turns, GoTo sends two commands, and each was its
+// own connect and close. Those modules commonly accept a SINGLE session and
+// need a moment to release it, so the churn alone can look like a controller
+// that ignores half of what it is told.
+//
+// So the socket is kept between calls and one mutex serialises every
+// exchange, which also stops a poll and a command from holding two sessions
+// at once. A write or read error drops the socket; the next call redials.
+// Serial keeps its open-per-call, where a COM port has one owner anyway.
 package dcu1
 
 import (
@@ -31,6 +46,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	"go.bug.st/serial"
@@ -41,8 +57,9 @@ const (
 	ioTimeout   = 2 * time.Second
 )
 
-// Client is a stateless per-call sender, mirroring the gs232/pst/rotgenius idiom.
-// Exactly one of (Host, Port) or ComPort is used.
+// Client talks to one controller. Exactly one of (Host, Port) or ComPort is
+// used. Hold onto it: over TCP it keeps its session open between calls, so a
+// fresh Client per command would give the churn back.
 type Client struct {
 	Host    string
 	Port    int
@@ -50,6 +67,29 @@ type Client struct {
 	// Baud varies by controller (a Hy-Gain DCU-1 is 4800; Green Heron / RotorCard
 	// can differ). Zero keeps 4800.
 	Baud int
+
+	// mu serialises every exchange. Two goroutines are in here in normal use —
+	// the heading poll and the operator's own commands — and on a single-session
+	// controller their overlap is the fault, not just a race on one socket.
+	mu sync.Mutex
+	// conn is the kept TCP session. nil when not connected, or after an error
+	// dropped it. Unused on serial.
+	conn net.Conn
+}
+
+// Close drops the kept session. Safe to call at any time and on any Client.
+func (c *Client) Close() {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	c.dropLocked()
+}
+
+// dropLocked closes the session so the next exchange redials. Caller holds mu.
+func (c *Client) dropLocked() {
+	if c.conn != nil {
+		_ = c.conn.Close()
+		c.conn = nil
+	}
 }
 
 // New returns a TCP Client (a serial-over-IP bridge in front of the controller).
@@ -72,27 +112,56 @@ func NewSerial(comPort string, baud int) *Client {
 // wantReply) reads until a 3-digit bearing is present. cmd must NOT carry the
 // ';'.
 func (c *Client) roundTrip(cmd string, wantReply bool) (string, error) {
-	var conn io.ReadWriteCloser
+	c.mu.Lock()
+	defer c.mu.Unlock()
 	if c.ComPort != "" {
-		baud := c.Baud
-		if baud <= 0 {
-			baud = 4800
-		}
-		sp, err := serial.Open(c.ComPort, &serial.Mode{BaudRate: baud})
-		if err != nil {
-			return "", fmt.Errorf("open rotator %s @ %d baud: %w", c.ComPort, baud, err)
-		}
-		_ = sp.SetReadTimeout(200 * time.Millisecond)
-		conn = sp
-	} else {
-		nc, err := net.DialTimeout("tcp", net.JoinHostPort(c.Host, strconv.Itoa(c.Port)), dialTimeout)
-		if err != nil {
-			return "", fmt.Errorf("connect DCU-1 %s:%d: %w", c.Host, c.Port, err)
-		}
-		_ = nc.SetDeadline(time.Now().Add(ioTimeout))
-		conn = nc
+		return c.exchangeSerial(cmd, wantReply)
 	}
-	defer conn.Close()
+	// A kept socket can be half-dead: the far end went away and the first write
+	// still succeeds because nothing has been acknowledged yet. So one retry on
+	// a FRESH connection, and only when the session was one we had already —
+	// a dial that fails is a dial that fails.
+	for attempt := 0; attempt < 2; attempt++ {
+		reused := c.conn != nil
+		if c.conn == nil {
+			nc, err := net.DialTimeout("tcp", net.JoinHostPort(c.Host, strconv.Itoa(c.Port)), dialTimeout)
+			if err != nil {
+				return "", fmt.Errorf("connect DCU-1 %s:%d: %w", c.Host, c.Port, err)
+			}
+			c.conn = nc
+		}
+		_ = c.conn.SetDeadline(time.Now().Add(ioTimeout))
+		line, err := c.exchange(c.conn, cmd, wantReply)
+		if err == nil {
+			return line, nil
+		}
+		c.dropLocked()
+		if !reused {
+			return "", err
+		}
+	}
+	return "", fmt.Errorf("no reply to %q", cmd)
+}
+
+// exchangeSerial opens the port for one exchange and closes it again. A COM
+// port has one owner, so holding it open would lock out the controller's own
+// software for the whole session.
+func (c *Client) exchangeSerial(cmd string, wantReply bool) (string, error) {
+	baud := c.Baud
+	if baud <= 0 {
+		baud = 4800
+	}
+	sp, err := serial.Open(c.ComPort, &serial.Mode{BaudRate: baud})
+	if err != nil {
+		return "", fmt.Errorf("open rotator %s @ %d baud: %w", c.ComPort, baud, err)
+	}
+	defer sp.Close()
+	_ = sp.SetReadTimeout(200 * time.Millisecond)
+	return c.exchange(sp, cmd, wantReply)
+}
+
+// exchange sends one ';'-terminated command and reads the reply, if any.
+func (c *Client) exchange(conn io.ReadWriter, cmd string, wantReply bool) (string, error) {
 	if _, err := conn.Write([]byte(cmd + ";")); err != nil {
 		return "", fmt.Errorf("send %q: %w", cmd, err)
 	}

internal/rotator/dcu1/session_test.go

@@ -0,0 +1,162 @@
+package dcu1
+
+import (
+	"net"
+	"strconv"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+)
+
+// fakeRT21 is a controller that accepts ONE session at a time and counts how
+// many it was asked for, which is the thing under test.
+type fakeRT21 struct {
+	ln net.Listener
+
+	mu       sync.Mutex
+	sessions int
+	cmds     []string
+}
+
+func newFakeRT21(t *testing.T) *fakeRT21 {
+	t.Helper()
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	f := &fakeRT21{ln: ln}
+	go f.serve()
+	t.Cleanup(func() { _ = ln.Close() })
+	return f
+}
+
+func (f *fakeRT21) serve() {
+	for {
+		conn, err := f.ln.Accept()
+		if err != nil {
+			return
+		}
+		f.mu.Lock()
+		f.sessions++
+		f.mu.Unlock()
+		// Served one at a time, on purpose: a second caller waits in the accept
+		// queue rather than being talked to, which is how these modules behave.
+		f.handle(conn)
+	}
+}
+
+func (f *fakeRT21) handle(conn net.Conn) {
+	defer conn.Close()
+	buf := make([]byte, 64)
+	for {
+		_ = conn.SetReadDeadline(time.Now().Add(2 * time.Second))
+		n, err := conn.Read(buf)
+		if n > 0 {
+			for _, cmd := range strings.Split(string(buf[:n]), ";") {
+				if cmd = strings.TrimSpace(cmd); cmd == "" {
+					continue
+				}
+				f.mu.Lock()
+				f.cmds = append(f.cmds, cmd)
+				f.mu.Unlock()
+				if cmd == "AI1" {
+					_, _ = conn.Write([]byte(";123"))
+				}
+			}
+		}
+		if err != nil {
+			return
+		}
+	}
+}
+
+func (f *fakeRT21) port() int {
+	return f.ln.Addr().(*net.TCPAddr).Port
+}
+
+func (f *fakeRT21) seen() (int, []string) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	return f.sessions, append([]string(nil), f.cmds...)
+}
+
+// One session for the whole conversation.
+//
+// It used to be one per command: the heading is polled twice a second while the
+// antenna turns and GoTo sends two commands (AP1 then AM1), each with its own
+// connect and close. An RT-21's Ethernet option — like most embedded serial
+// servers — commonly accepts a single session and needs a moment to release it,
+// so the churn alone looked like a controller ignoring half of what it was told.
+func TestOneSessionServesEveryCommand(t *testing.T) {
+	f := newFakeRT21(t)
+	c := New("127.0.0.1", f.port())
+	defer c.Close()
+
+	if az, _, err := c.Heading(); err != nil || az != 123 {
+		t.Fatalf("Heading() = %d, %v; want 123", az, err)
+	}
+	if err := c.GoTo(240); err != nil {
+		t.Fatalf("GoTo: %v", err)
+	}
+	if az, _, err := c.Heading(); err != nil || az != 123 {
+		t.Fatalf("second Heading() = %d, %v", az, err)
+	}
+
+	sessions, cmds := f.seen()
+	if sessions != 1 {
+		t.Errorf("the controller was asked for %d sessions; four commands must share one", sessions)
+	}
+	want := []string{"AI1", "AP1240", "AM1", "AI1"}
+	if strings.Join(cmds, ",") != strings.Join(want, ",") {
+		t.Errorf("commands %v, want %v", cmds, want)
+	}
+}
+
+// A session the controller has dropped is redialled, and the command that found
+// it dead is retried rather than reported as a failure — a kept socket's first
+// write succeeds long after the far end has gone.
+func TestADroppedSessionIsRedialled(t *testing.T) {
+	f := newFakeRT21(t)
+	c := New("127.0.0.1", f.port())
+	defer c.Close()
+
+	if _, _, err := c.Heading(); err != nil {
+		t.Fatalf("first Heading: %v", err)
+	}
+	// The controller power-cycles: close our end the way a dropped session
+	// leaves it, then ask again.
+	c.mu.Lock()
+	if c.conn != nil {
+		_ = c.conn.Close() // closed underneath, but still held — a half-dead socket
+	}
+	c.mu.Unlock()
+
+	if az, _, err := c.Heading(); err != nil || az != 123 {
+		t.Fatalf("Heading after the session dropped = %d, %v; want 123", az, err)
+	}
+	if sessions, _ := f.seen(); sessions != 2 {
+		t.Errorf("%d sessions; the dropped one should have been redialled exactly once", sessions)
+	}
+}
+
+// Nothing is kept open for a controller that is not there, and the error names
+// the address so it can be checked.
+func TestADeadControllerReportsWhereItLooked(t *testing.T) {
+	// Port 1 on loopback: nothing listens, and the refusal is immediate.
+	c := New("127.0.0.1", 1)
+	defer c.Close()
+	_, _, err := c.Heading()
+	if err == nil {
+		t.Fatal("no error from a controller that is not there")
+	}
+	if !strings.Contains(err.Error(), "127.0.0.1:"+strconv.Itoa(1)) {
+		t.Errorf("error %q does not name the address it tried", err)
+	}
+	c.mu.Lock()
+	held := c.conn != nil
+	c.mu.Unlock()
+	if held {
+		t.Error("a failed dial left a connection behind")
+	}
+}