fix(rotator): stop rebooting the controller between commands

A K3NG controller answered PuTTY perfectly and told OpsLog 'no reply to C'. The reason is not the protocol: the client opened and CLOSED the serial port for every single command, and an Arduino-based controller resets when its port is opened — DTR pulses the reset pin. OpsLog was rebooting it several times a second, and every command it sent landed in a bootloader. The port is opened once and held, per COM port, at package level: the callers build a fresh Client per poll, so the port has to outlive them, and a serial port is a single-owner resource in any case. A newly opened port is given two seconds to boot before the first command, bytes left from a previous exchange are drained rather than read as this command's answer, and a failed exchange drops the port so the next starts from a clean open instead of repeating the same silence. Reply parsing was already right for both flavours and now has the real strings to prove it, that controller's '+0140' among them.
2026-09-03 19:30:59 +02:00
parent b0bbe3e402
commit ec347e1b7a
3 changed files with 136 additions and 14 deletions
@@ -28,6 +28,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"sync"
 	"time"

 	"go.bug.st/serial"
@@ -71,32 +72,99 @@ func NewSerial(comPort string, baud int) *Client {
 	return &Client{ComPort: comPort, Baud: baud}
 }

-// roundTrip opens a connection (TCP or serial per the client's config), sends
-// one CR-terminated command and (when wantReply) reads one CR/LF-terminated
-// reply line.
+// bootSettle is how long a freshly opened serial port is left alone before the
+// first command.
+//
+// An Arduino-based controller — K3NG's firmware, the ERC family — RESETS when
+// the serial port is opened: the DTR line pulses its reset pin, and the
+// bootloader then holds the processor for a second or more. A command sent into
+// that window is simply lost, which is exactly how a controller that answers
+// PuTTY perfectly reports "no reply" here.
+const bootSettle = 2 * time.Second
+
+// heldPort is an open serial port, kept between calls.
+//
+// The package holds it rather than the Client because the callers build a FRESH
+// Client for every poll (one per heading request), and the port has to outlive
+// them. Reopening per command is what made an Arduino controller reboot several
+// times a second and never answer anything. A serial port is a single-owner
+// resource in any case: two clients for COM5 would be two handles on one cable.
+type heldPort struct {
+	p        serial.Port
+	openedAt time.Time
+}
+
+var (
+	portsMu   sync.Mutex
+	openPorts = map[string]*heldPort{}
+)
+
+// acquire returns the open port for com, opening it if needed.
+func acquire(com string, baud int) (*heldPort, error) {
+	portsMu.Lock()
+	defer portsMu.Unlock()
+	if h, ok := openPorts[com]; ok && h.p != nil {
+		return h, nil
+	}
+	if baud <= 0 {
+		baud = 9600
+	}
+	sp, err := serial.Open(com, &serial.Mode{BaudRate: baud})
+	if err != nil {
+		return nil, fmt.Errorf("open rotator %s @ %d baud: %w", com, baud, err)
+	}
+	_ = sp.SetReadTimeout(200 * time.Millisecond)
+	h := &heldPort{p: sp, openedAt: time.Now()}
+	openPorts[com] = h
+	return h, nil
+}
+
+// drop closes and forgets a port, so the next call opens a fresh one. Called
+// when an exchange fails: a half-spoken conversation is worse than a new one.
+func drop(com string) {
+	portsMu.Lock()
+	defer portsMu.Unlock()
+	if h, ok := openPorts[com]; ok {
+		if h.p != nil {
+			_ = h.p.Close()
+		}
+		delete(openPorts, com)
+	}
+}
+
+// roundTrip sends one CR-terminated command and (when wantReply) reads one
+// CR/LF-terminated reply line. Serial keeps its port open between calls; TCP
+// dials per call, which is what the ARCO's LAN side expects.
 func (c *Client) roundTrip(cmd string, wantReply bool) (string, error) {
 	var conn io.ReadWriteCloser
 	if c.ComPort != "" {
-		baud := c.Baud
-		if baud <= 0 {
-			baud = 9600
-		}
-		sp, err := serial.Open(c.ComPort, &serial.Mode{BaudRate: baud})
+		h, err := acquire(c.ComPort, c.Baud)
 		if err != nil {
-			return "", fmt.Errorf("open rotator %s @ %d baud: %w", c.ComPort, baud, err)
+			return "", err
 		}
-		_ = sp.SetReadTimeout(200 * time.Millisecond)
-		conn = sp
+		// Let a just-reset controller finish booting before speaking to it.
+		if wait := bootSettle - time.Since(h.openedAt); wait > 0 {
+			time.Sleep(wait)
+		}
+		conn = h.p
+		// Whatever is already in the buffer belongs to the last exchange — the
+		// trailing LF of the previous reply, or a line the controller volunteered
+		// while nobody was reading. Read as the answer to THIS command it would
+		// be an answer to the wrong question.
+		drain(h.p)
 	} else {
 		nc, err := net.DialTimeout("tcp", net.JoinHostPort(c.Host, strconv.Itoa(c.Port)), dialTimeout)
 		if err != nil {
 			return "", fmt.Errorf("connect ARCO %s:%d: %w", c.Host, c.Port, err)
 		}
 		_ = nc.SetDeadline(time.Now().Add(ioTimeout))
+		defer nc.Close()
 		conn = nc
 	}
-	defer conn.Close()
 	if _, err := conn.Write([]byte(cmd + "\r")); err != nil {
+		if c.ComPort != "" {
+			drop(c.ComPort)
+		}
 		return "", fmt.Errorf("send %q: %w", cmd, err)
 	}
 	if !wantReply {
@@ -121,11 +189,29 @@ func (c *Client) roundTrip(cmd string, wantReply bool) (string, error) {
 	}
 	line := strings.TrimSpace(sb.String())
 	if line == "" {
+		// Silence may mean the port is fine and the controller is not, or that
+		// the handle is stale (a USB adapter unplugged and replugged). Let go of
+		// it so the next attempt starts from a clean open rather than repeating
+		// the same silence for ever.
+		if c.ComPort != "" {
+			drop(c.ComPort)
+		}
 		return "", fmt.Errorf("no reply to %q", cmd)
 	}
 	return line, nil
 }

+// drain empties whatever is waiting, without blocking for long.
+func drain(sp serial.Port) {
+	buf := make([]byte, 128)
+	for i := 0; i < 4; i++ {
+		n, err := sp.Read(buf)
+		if n == 0 || err != nil {
+			return
+		}
+	}
+}
+
 // GoTo points the antenna at the given azimuth (0-359). GS-232A takes M000-M450
 // (overlap rotators accept >360); we normalise to [0,360).
 func (c *Client) GoTo(az int) error {
@@ -0,0 +1,34 @@
+package gs232
+
+import (
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// Real replies, as the controllers actually send them — a K3NG answering "C"
+// with "+0140" and CR+LF among them (reported from a live controller).
+func TestAzimuthReplies(t *testing.T) {
+	cases := []struct {
+		raw  string
+		want int
+	}{
+		{"+0140\r\n", 140},         // GS-232A, K3NG firmware
+		{"+0000\r", 0},             // due north
+		{"+0359\r\n", 359},         // just short of it
+		{"AZ=140\r\n", 140},        // GS-232B flavour
+		{"AZ=140 EL=000\r\n", 140}, // GS-232B with elevation on the same line
+		{"\r\n+0075\r\n", 75},      // a leftover terminator ahead of the answer
+	}
+	for _, c := range cases {
+		m := azRe.FindStringSubmatch(strings.TrimSpace(c.raw))
+		if m == nil {
+			t.Errorf("no azimuth found in %q", c.raw)
+			continue
+		}
+		got, _ := strconv.Atoi(m[1])
+		if got%360 != c.want {
+			t.Errorf("%q parsed as %d, want %d", c.raw, got%360, c.want)
+		}
+	}
+}