diff --git a/internal/device/snapshot.go b/internal/device/snapshot.go index 392a504..a73d8bc 100644 --- a/internal/device/snapshot.go +++ b/internal/device/snapshot.go @@ -69,7 +69,12 @@ func (manager *Manager) readSnapshot( if ccidErr != nil { ccid, ccidErr = manager.command(ctx, client, "AT+QCCID") } - if ccidErr != nil && strings.EqualFold(strings.TrimSpace(backend), "qmi") && isNativeQMICandidate(candidate) { + if ccidErr != nil && strings.EqualFold(strings.TrimSpace(backend), "qmi") && isNativeQMICandidate(candidate) && + strings.EqualFold(strings.TrimSpace(snapshot.SIMStatus), "READY") { + // Without a READY SIM the QMI UIM ICCID read blocks until its (long) + // timeout, and every refresh holds the device lock while it does so, + // starving the AT terminal. Only fall back to QMI when the AT CPIN + // probe already proved a card is present. qmiContext, cancelQMI := manager.withTimeout(ctx, manager.commandTimeout*5) qmiICCID, qmiErr := manager.readNativeQMIICCID(qmiContext, candidate) cancelQMI() @@ -186,14 +191,18 @@ func (manager *Manager) readSnapshot( snapshot.RegistrationSource = "COPS" } if snapshot.IMEI == "" { - response, ok := optional("AT+CGSN") - if ok { - snapshot.IMEI = parseIdentifier( - response, - []string{"+CGSN:", "+GSN:"}, - 14, - 17, - ) + // AT+CGSN on some MHI modems (the UFI dongle behind the OpenStick 410) + // returns the IMEI line but never a final OK, so it would block until the + // caller's deadline (30s during a periodic refresh) and starve every other + // device operation behind the lock. Give it an independent short timeout + // and let the WWAN transport's drain discard the trailing stale bytes. + cgsnCtx, cancelCGSN := context.WithTimeout(ctx, manager.commandTimeout) + cgsnResponse, cgsnErr := manager.command(cgsnCtx, client, "AT+CGSN") + cancelCGSN() + if cgsnErr == nil { + if imei := parseIdentifier(cgsnResponse, []string{"+CGSN:", "+GSN:"}, 14, 17); imei != "" { + snapshot.IMEI = imei + } } } if snapshot.IMEI == "" && strings.EqualFold(strings.TrimSpace(backend), "qmi") && isNativeQMICandidate(candidate) { @@ -211,7 +220,6 @@ func (manager *Manager) readSnapshot( // Preserve a prior successful read across a transient QMI/AT failure. snapshot.IMEI = previousSnapshot.IMEI } - if response, ok := optional("AT+CFUN?"); ok { if mode, found := parseCFUN(response); found { snapshot.OperatingMode = mode diff --git a/internal/device/snapshot_guard_test.go b/internal/device/snapshot_guard_test.go new file mode 100644 index 0000000..f7623c9 --- /dev/null +++ b/internal/device/snapshot_guard_test.go @@ -0,0 +1,142 @@ +package device + +import ( + "context" + "errors" + "strings" + "sync" + "testing" + "time" + + "vocat/internal/modem" +) + +// lenientATClient answers every command with a bare CommandError and records +// the commands it saw. It lets snapshot tests exercise the full readSnapshot +// sequence without enumerating every step of the transcript. +type lenientATClient struct { + mu sync.Mutex + commands []string + cgsnDelay time.Duration + cgsnIMEI string +} + +func (c *lenientATClient) Execute(ctx context.Context, command string) (modem.Response, error) { + c.mu.Lock() + c.commands = append(c.commands, command) + c.mu.Unlock() + if command == "ATI" { + return okResponse("Qualcomm", "PCIe/MHI WWAN modem", "Revision: native-410"), nil + } + if command == "AT+CGSN" && c.cgsnDelay > 0 { + select { + case <-time.After(c.cgsnDelay): + case <-ctx.Done(): + } + } + if command == "AT+CGSN" && c.cgsnIMEI != "" { + return okResponse("+CGSN: " + c.cgsnIMEI), nil + } + return modem.Response{}, &modem.CommandError{Command: command, Final: "ERROR"} +} + +func (c *lenientATClient) WaitURC(context.Context, func(string) bool) (string, error) { + return "", errors.New("no URC") +} + +func (c *lenientATClient) Close() error { return nil } + +func (c *lenientATClient) saw(command string) bool { + c.mu.Lock() + defer c.mu.Unlock() + for _, seen := range c.commands { + if seen == command { + return true + } + } + return false +} + +// AT+CGSN on some MHI modems returns the IMEI line but never a final OK, so it +// would block until the caller's deadline and hold the device lock for the +// whole periodic refresh. The snapshot must bound CGSN with its own short +// timeout instead of inheriting the refresh deadline. +func TestManagerRefreshBoundsCGSNTimeout(t *testing.T) { + client := &lenientATClient{cgsnDelay: 5 * time.Second} + manager, id := newStartedTestManager(t, client) + + ctx, cancel := context.WithTimeout(context.Background(), 4*time.Second) + defer cancel() + start := time.Now() + snapshot, err := manager.Refresh(ctx, id) + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("Refresh: %v", err) + } + // CGSN times out after CommandTimeout (1s in the test manager); the rest + // of the snapshot is immediate. An un-bounded CGSN would wait for the + // 4s outer deadline (or worse, a real 30s refresh deadline). + if elapsed > 3*time.Second { + t.Fatalf("Refresh took %s; CGSN was not bounded by CommandTimeout", elapsed) + } + if !client.saw("AT+CGSN") { + t.Fatalf("CGSN was never sent; commands = %v", client.commands) + } + if snapshot.IMEI != "" { + t.Fatalf("IMEI = %q, want empty after CGSN timeout", snapshot.IMEI) + } +} + +// A missing SIM must not fall back to the QMI UIM ICCID read: without a READY +// card that call blocks until its long timeout and starves the AT terminal +// behind the device lock. +func TestManagerRefreshSkipsQMIICCIDWithoutReadySIM(t *testing.T) { + // CGSN succeeds so the snapshot does not fall back to the QMI DMS IMEI + // read either; the test focuses on the UIM ICCID fallback being skipped + // without a READY card. + client := &lenientATClient{cgsnIMEI: "866241014372802"} + manager, err := NewManager(Options{ + Discoverer: staticDiscoverer{candidates: []modem.Candidate{{ + ID: "mhi-wwan0", + Product: "PCIe/MHI WWAN modem", + QMIControl: "/dev/wwan0qmi0", + NetworkInterface: "wwan0", + ATPort: modem.Port{Path: "/dev/wwan0at0", Name: "wwan0at0", Role: modem.PortRoleAT}, + }}}, + Opener: &staticOpener{client: client}, + }) + if err != nil { + t.Fatal(err) + } + if err := manager.Start(context.Background()); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = manager.Stop(context.Background()) }) + + qmiCalls := 0 + manager.qmiRadioOpener = func(context.Context, string) (qmiRadioSession, error) { + qmiCalls++ + return nil, errors.New("QMI should not be opened without a SIM") + } + if err := manager.SetBackend("mhi-wwan0", "qmi"); err != nil { + t.Fatal(err) + } + + snapshot, err := manager.Refresh(context.Background(), "mhi-wwan0") + if err != nil { + t.Fatalf("Refresh: %v", err) + } + // Exactly one QMI open is expected: the immutable DMS IMEI read runs + // unconditionally for native QMI candidates (IMEI is hardware identity, + // independent of the card). The UIM ICCID fallback, which would block + // without a READY SIM, must be skipped. + if qmiCalls != 1 { + t.Fatalf("qmiRadioOpener called %d times, want 1 (DMS IMEI only, UIM ICCID must be skipped without a READY SIM)", qmiCalls) + } + for _, warning := range snapshot.Warnings { + if strings.Contains(warning, "QMI UIM") { + t.Fatalf("unexpected QMI ICCID warning: %q", warning) + } + } +} diff --git a/internal/modem/discovery.go b/internal/modem/discovery.go index 7db8b0e..2ef67e4 100644 --- a/internal/modem/discovery.go +++ b/internal/modem/discovery.go @@ -12,11 +12,9 @@ import ( ) const ( - quectelVendorID = "2c7c" - djiVendorID = "2ca3" - dji4GProductID = "4006" + djiVendorID = "2ca3" + dji4GProductID = "4006" ) - type SysFSDiscoverer struct { SysRoot string DevRoot string @@ -44,10 +42,19 @@ func (d *SysFSDiscoverer) Discover(ctx context.Context) ([]Candidate, error) { if os.IsNotExist(err) { entries = nil } else { - return nil, fmt.Errorf("discover Quectel USB devices: %w", err) + return nil, fmt.Errorf("discover USB QMI modems: %w", err) } } + // Candidate modems are identified by kernel driver binding instead of a + // vendor-ID whitelist. qmi_wwan only binds Qualcomm QMI control interfaces, + // so any USB device with a bound interface exposes a live QMI channel. This + // keeps discovery vendor-neutral (SIMCom, Sierra, Telit and other + // Qualcomm-based modems are found automatically) while MBIM-only devices + // stay out, because cdc_mbim binds their control interface instead and the + // project has no MBIM backend. + qmiBound := d.qmiWWANBoundDevices() + aliases := readSerialAliases(filepath.Join(d.DevRoot, "serial", "by-id")) devices := make(map[string]*discoveredUSBDevice) for _, entry := range entries { @@ -75,7 +82,7 @@ func (d *SysFSDiscoverer) Discover(ctx context.Context) ([]Candidate, error) { } vendorID := strings.ToLower(readTrimmed(filepath.Join(resolvedDevice, "idVendor"))) productID := strings.ToLower(readTrimmed(filepath.Join(resolvedDevice, "idProduct"))) - if !isSupportedUSBModem(vendorID, productID) { + if _, bound := qmiBound[deviceName]; !bound && !IsDJI4GUSB(vendorID, productID) { continue } @@ -84,7 +91,7 @@ func (d *SysFSDiscoverer) Discover(ctx context.Context) ([]Candidate, error) { serialNumber := readTrimmed(filepath.Join(resolvedDevice, "serial")) state = &discoveredUSBDevice{ candidate: Candidate{ - ID: candidateID(productID, serialNumber, deviceName), + ID: candidateID(vendorID, productID, serialNumber, deviceName), VendorID: vendorID, ProductID: productID, Manufacturer: readTrimmed(filepath.Join(resolvedDevice, "manufacturer")), @@ -134,6 +141,14 @@ func (d *SysFSDiscoverer) Discover(ctx context.Context) ([]Candidate, error) { }) assignQuectelPortRoles(state.candidate.Ports) state.candidate.ATPort = selectATPort(state.candidate.Ports) + if !state.candidate.HasATPort() { + // A bound QMI interface proves the modem is alive, but the snapshot, + // SMS, USSD and eSIM (AT+CSIM) paths all require an AT port. A missing + // ttyUSB/ttyACM node almost always means the option/qcserial driver + // does not claim the serial interfaces (often a missing PID in its + // device-ID table), not that the module lacks an AT interface. + state.candidate.DiscoveryIssue = "at_port_missing" + } result = append(result, state.candidate) } wwanCandidates, err := d.discoverWWAN(ctx) @@ -145,11 +160,6 @@ func (d *SysFSDiscoverer) Discover(ctx context.Context) ([]Candidate, error) { return result, nil } -func isSupportedUSBModem(vendorID, productID string) bool { - return strings.EqualFold(strings.TrimSpace(vendorID), quectelVendorID) || - IsDJI4GUSB(vendorID, productID) -} - // IsDJI4GUSB reports whether a USB identity belongs to the first-generation // DJI/Baiwang 4G module. It keeps the factory 2ca3:4006 identity usable without // requiring a persistent AT+QCFG USB identity rewrite to Quectel 2c7c:0125. @@ -252,7 +262,7 @@ func (d *SysFSDiscoverer) discoverWWAN(ctx context.Context) ([]Candidate, error) Ports: group.ports, NetworkInterface: selectWWANNetworkInterface(d.SysRoot, group.index), } if len(group.ports) > 0 { - candidate.ATPort = group.ports[0] + candidate.ATPort = selectWWANATPort(group.ports) } if len(group.qmiNames) > 0 { candidate.QMIControl = filepath.Join(d.DevRoot, group.qmiNames[0]) @@ -263,6 +273,20 @@ func (d *SysFSDiscoverer) discoverWWAN(ctx context.Context) ([]Candidate, error) return result, nil } +// selectWWANATPort prefers the secondary AT port (…at1) over the primary +// (…at0) when both exist, falling back to the first AT port otherwise. Some +// Qualcomm MHI modems (notably the UFI dongle behind the OpenStick 410) answer +// on at1 immediately while at0 delays every response by 10-20 seconds, so the +// secondary port is the usable AT channel. +func selectWWANATPort(ports []Port) Port { + for _, port := range ports { + if port.InterfaceNumber == 1 { + return port + } + } + return ports[0] +} + func parseWWANPortName(name string) (index, kind string, portIndex int, ok bool) { if !strings.HasPrefix(name, "wwan") { return "", "", 0, false @@ -402,7 +426,34 @@ func readSerialAliases(root string) map[string]string { return result } -func candidateID(productID, serialNumber, usbName string) string { +// qmiWWANBoundDevices returns the set of USB device paths (for example "1-6" +// or the hub-attached "1-4.3.2") that currently have at least one interface +// bound to the kernel's qmi_wwan driver. Interface entries in the driver +// directory are named ":.", so the part +// before the first colon is the owning USB device. The qmi_wwan driver only +// binds Qualcomm QMI control interfaces, so membership doubles as a vendor- +// neutral "this is a live QMI modem" signal. +func (d *SysFSDiscoverer) qmiWWANBoundDevices() map[string]struct{} { + driverRoot := filepath.Join(d.SysRoot, "bus", "usb", "drivers", "qmi_wwan") + entries, err := os.ReadDir(driverRoot) + if err != nil { + return nil + } + devices := make(map[string]struct{}) + for _, entry := range entries { + // The driver directory also holds control files (bind, unbind, uevent, + // module, new_id, ...); only names containing a colon are interfaces. + deviceName, _, ok := strings.Cut(entry.Name(), ":") + if !ok || deviceName == "" { + continue + } + devices[deviceName] = struct{}{} + } + return devices +} + +func candidateID(vendorID, productID, serialNumber, usbName string) string { + prefix := "usb-" + sanitizeID(vendorID) serialNumber = strings.TrimSpace(serialNumber) if serialNumber != "" && !strings.EqualFold(serialNumber, "android") { // A surprising number of EC20/EC25 carrier boards expose the same @@ -411,9 +462,9 @@ func candidateID(productID, serialNumber, usbName string) string { // to the same hub into one entry. Include the physical USB topology in the // discovery key; configured devices remain stable through ATMapper's // USB-path/IMEI matching even when Linux renumbers ttyUSB nodes. - return "quectel-" + sanitizeID(serialNumber+"-"+usbName) + return prefix + "-" + sanitizeID(serialNumber+"-"+usbName) } - return "quectel-" + sanitizeID(productID+"-"+usbName) + return prefix + "-" + sanitizeID(productID+"-"+usbName) } func sanitizeID(value string) string { diff --git a/internal/modem/discovery_common_test.go b/internal/modem/discovery_common_test.go index 90a7664..b00031e 100644 --- a/internal/modem/discovery_common_test.go +++ b/internal/modem/discovery_common_test.go @@ -2,24 +2,24 @@ package modem import "testing" -func TestSupportedUSBModemIdentity(t *testing.T) { +func TestIsDJI4GUSBIdentity(t *testing.T) { tests := []struct { name string vendorID string productID string want bool }{ - {name: "Quectel", vendorID: "2c7c", productID: "0125", want: true}, {name: "DJI 4G module", vendorID: "2ca3", productID: "4006", want: true}, {name: "DJI 4G module uppercase", vendorID: "2CA3", productID: "4006", want: true}, {name: "unrelated DJI device", vendorID: "2ca3", productID: "001f", want: false}, + {name: "Quectel identity", vendorID: "2c7c", productID: "0125", want: false}, {name: "unrelated USB device", vendorID: "0403", productID: "6001", want: false}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if got := isSupportedUSBModem(test.vendorID, test.productID); got != test.want { - t.Fatalf("isSupportedUSBModem(%q, %q) = %v, want %v", test.vendorID, test.productID, got, test.want) + if got := IsDJI4GUSB(test.vendorID, test.productID); got != test.want { + t.Fatalf("IsDJI4GUSB(%q, %q) = %v, want %v", test.vendorID, test.productID, got, test.want) } }) } diff --git a/internal/modem/discovery_test.go b/internal/modem/discovery_test.go index 69c4ebe..f56dd6d 100644 --- a/internal/modem/discovery_test.go +++ b/internal/modem/discovery_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "testing" ) @@ -47,6 +48,7 @@ func TestSysFSDiscoverySelectsInterface04AndNeverInterface02(t *testing.T) { } mustMkdir(t, filepath.Join(usbRoot, "1-6:1.0", "net", "enx001122334455")) mustMkdir(t, filepath.Join(usbRoot, "1-6:1.4", "usbmisc", "cdc-wdm0")) + mustBindQMIWWAN(t, sysRoot, "1-6:1.4") discoverer := NewSysFSDiscoverer(sysRoot, devRoot) candidates, err := discoverer.Discover(context.Background()) @@ -57,7 +59,7 @@ func TestSysFSDiscoverySelectsInterface04AndNeverInterface02(t *testing.T) { t.Fatalf("got %d candidates, want 1", len(candidates)) } candidate := candidates[0] - if candidate.ID != "quectel-0125-1-6" { + if candidate.ID != "usb-2c7c-0125-1-6" { t.Fatalf("ID = %q", candidate.ID) } if candidate.ATPort.Name != "ttyUSB2" { @@ -101,6 +103,7 @@ func TestSysFSDiscoverySelectsTTYUSB2InQMIInterface00Layout(t *testing.T) { ) mustMkdir(t, filepath.Join(usbRoot, "1-6:1.4", "usbmisc", "cdc-wdm0")) mustMkdir(t, filepath.Join(usbRoot, "1-6:1.4", "net", "wwp0s20f0u6i4")) + mustBindQMIWWAN(t, sysRoot, "1-6:1.4") candidates, err := NewSysFSDiscoverer(sysRoot, devRoot).Discover(context.Background()) if err != nil { @@ -146,6 +149,7 @@ func TestSysFSDiscoverySelectsATPortForSecondQMIUSBModem(t *testing.T) { } mustWrite(t, filepath.Join(usbRoot, modem.usbName+":1.4", "bInterfaceNumber"), "04\n") mustMkdir(t, filepath.Join(usbRoot, modem.usbName+":1.4", "usbmisc", modem.wdm)) + mustBindQMIWWAN(t, sysRoot, modem.usbName+":1.4") } candidates, err := NewSysFSDiscoverer(sysRoot, devRoot).Discover(context.Background()) @@ -194,6 +198,7 @@ func TestSysFSDiscoveryDoesNotCollapseModemsWithSharedFactorySerial(t *testing.T mustMkdir(t, filepath.Join(usbRoot, interfaceName, tty, "tty", tty)) } mustMkdir(t, filepath.Join(usbRoot, item.usbName+":1.4", "usbmisc", fmt.Sprintf("cdc-wdm%d", index))) + mustBindQMIWWAN(t, sysRoot, item.usbName+":1.4") } candidates, err := NewSysFSDiscoverer(sysRoot, devRoot).Discover(context.Background()) @@ -216,9 +221,11 @@ func TestSysFSDiscoveryDoesNotCollapseModemsWithSharedFactorySerial(t *testing.T } } -func TestSysFSDiscoveryIgnoresNonQuectelUSB(t *testing.T) { +func TestSysFSDiscoveryIgnoresUSBWithoutQMIWWANBinding(t *testing.T) { root := t.TempDir() usbRoot := filepath.Join(root, "sys", "bus", "usb", "devices") + // A plain USB serial adapter (FTDI) exposes ttyUSB but no QMI interface and + // is never bound to qmi_wwan, so it must not be treated as a modem. mustWrite(t, filepath.Join(usbRoot, "2-1", "idVendor"), "0403\n") mustWrite(t, filepath.Join(usbRoot, "2-1:1.0", "bInterfaceNumber"), "00\n") mustMkdir(t, filepath.Join(usbRoot, "2-1:1.0", "ttyUSB9")) @@ -235,6 +242,117 @@ func TestSysFSDiscoveryIgnoresNonQuectelUSB(t *testing.T) { } } +func TestSysFSDiscoveryFindsNonQuectelVendorBoundToQMIWWAN(t *testing.T) { + root := t.TempDir() + sysRoot := filepath.Join(root, "sys") + devRoot := filepath.Join(root, "dev") + usbRoot := filepath.Join(sysRoot, "bus", "usb", "devices") + // A Sierra EM7430 flashed to its QMI (rmnet0) composition: non-Quectel + // vendor, but its control interface is bound to qmi_wwan. + mustWrite(t, filepath.Join(usbRoot, "1-3", "idVendor"), "1199\n") + mustWrite(t, filepath.Join(usbRoot, "1-3", "idProduct"), "9077\n") + mustWrite(t, filepath.Join(usbRoot, "1-3", "manufacturer"), "Sierra Wireless, Incorporated\n") + mustWrite(t, filepath.Join(usbRoot, "1-3", "product"), "EM7430\n") + for number, tty := range []string{"ttyUSB0", "ttyUSB1", "ttyUSB2", "ttyUSB3"} { + interfaceName := "1-3:1." + strconv.Itoa(number) + mustWrite(t, filepath.Join(usbRoot, interfaceName, "bInterfaceNumber"), fmt.Sprintf("%02x\n", number)) + mustMkdir(t, filepath.Join(usbRoot, interfaceName, tty, "tty", tty)) + } + mustMkdir(t, filepath.Join(usbRoot, "1-3:1.4", "usbmisc", "cdc-wdm0")) + mustBindQMIWWAN(t, sysRoot, "1-3:1.4") + + candidates, err := NewSysFSDiscoverer(sysRoot, devRoot).Discover(context.Background()) + if err != nil { + t.Fatalf("Discover: %v", err) + } + if len(candidates) != 1 { + t.Fatalf("got %d candidates, want 1", len(candidates)) + } + candidate := candidates[0] + if candidate.VendorID != "1199" || candidate.Product != "EM7430" { + t.Fatalf("candidate = %#v", candidate) + } + if candidate.ID != "usb-1199-9077-1-3" { + t.Fatalf("ID = %q", candidate.ID) + } + if candidate.ATPort.Role != PortRoleAT { + t.Fatalf("AT port = %#v", candidate.ATPort) + } + if candidate.QMIControl != filepath.Join(devRoot, "cdc-wdm0") { + t.Fatalf("QMI control = %q", candidate.QMIControl) + } + if candidate.DiscoveryIssue != "" { + t.Fatalf("discovery issue = %q, want none", candidate.DiscoveryIssue) + } +} + +func TestSysFSDiscoveryMarksQMIModemWithoutATPort(t *testing.T) { + root := t.TempDir() + sysRoot := filepath.Join(root, "sys") + devRoot := filepath.Join(root, "dev") + usbRoot := filepath.Join(sysRoot, "bus", "usb", "devices") + // QMI control interface is bound, but no ttyUSB/ttyACM node exists (for + // example the option/qcserial driver does not claim the serial interfaces). + mustWrite(t, filepath.Join(usbRoot, "1-7", "idVendor"), "2c7c\n") + mustWrite(t, filepath.Join(usbRoot, "1-7", "idProduct"), "0125\n") + mustMkdir(t, filepath.Join(usbRoot, "1-7:1.4", "usbmisc", "cdc-wdm0")) + mustBindQMIWWAN(t, sysRoot, "1-7:1.4") + + candidates, err := NewSysFSDiscoverer(sysRoot, devRoot).Discover(context.Background()) + if err != nil { + t.Fatalf("Discover: %v", err) + } + if len(candidates) != 1 { + t.Fatalf("got %d candidates, want 1", len(candidates)) + } + candidate := candidates[0] + if candidate.DiscoveryIssue != "at_port_missing" { + t.Fatalf("discovery issue = %q, want at_port_missing", candidate.DiscoveryIssue) + } + if candidate.HasATPort() { + t.Fatalf("candidate unexpectedly has an AT port: %#v", candidate.ATPort) + } + if candidate.QMIControl != filepath.Join(devRoot, "cdc-wdm0") { + t.Fatalf("QMI control = %q", candidate.QMIControl) + } +} + +func TestSysFSDiscoveryFindsHubAttachedQMIWWANDevice(t *testing.T) { + root := t.TempDir() + sysRoot := filepath.Join(root, "sys") + devRoot := filepath.Join(root, "dev") + usbRoot := filepath.Join(sysRoot, "bus", "usb", "devices") + // Device behind a hub: the USB path "1-4.3.2" contains extra segments, and + // the qmi_wwan binding uses the same composite path before the colon. + mustWrite(t, filepath.Join(usbRoot, "1-4.3.2", "idVendor"), "2c7c\n") + mustWrite(t, filepath.Join(usbRoot, "1-4.3.2", "idProduct"), "0125\n") + for number, tty := range []string{"ttyUSB0", "ttyUSB1", "ttyUSB2", "ttyUSB3"} { + interfaceName := "1-4.3.2:1." + strconv.Itoa(number) + mustWrite(t, filepath.Join(usbRoot, interfaceName, "bInterfaceNumber"), fmt.Sprintf("%02x\n", number)) + mustMkdir(t, filepath.Join(usbRoot, interfaceName, tty, "tty", tty)) + } + mustMkdir(t, filepath.Join(usbRoot, "1-4.3.2:1.4", "usbmisc", "cdc-wdm0")) + mustBindQMIWWAN(t, sysRoot, "1-4.3.2:1.4") + + candidates, err := NewSysFSDiscoverer(sysRoot, devRoot).Discover(context.Background()) + if err != nil { + t.Fatalf("Discover: %v", err) + } + if len(candidates) != 1 { + t.Fatalf("got %d candidates, want 1", len(candidates)) + } + candidate := candidates[0] + if candidate.ATPort.Name != "ttyUSB2" { + t.Fatalf("AT port = %#v, want ttyUSB2", candidate.ATPort) + } + if candidate.QMIControl != filepath.Join(devRoot, "cdc-wdm0") { + t.Fatalf("QMI control = %q", candidate.QMIControl) + } + if !strings.Contains(candidate.ID, "1-4-3-2") { + t.Fatalf("ID = %q, want hub topology in discovery key", candidate.ID) + } +} + func TestSysFSDiscoveryFindsPCIeMHIWWANWithoutUSBBus(t *testing.T) { root := t.TempDir() sysRoot := filepath.Join(root, "sys") @@ -256,7 +374,7 @@ func TestSysFSDiscoveryFindsPCIeMHIWWANWithoutUSBBus(t *testing.T) { if candidate.ID != "mhi-wwan0" || candidate.HardwareKind != "wwan" { t.Fatalf("identity = %#v", candidate) } - if candidate.ATPort.Path != filepath.Join(devRoot, "wwan0at0") || candidate.ATPort.Role != PortRoleAT { + if candidate.ATPort.Path != filepath.Join(devRoot, "wwan0at1") || candidate.ATPort.Role != PortRoleAT { t.Fatalf("AT port = %#v", candidate.ATPort) } if candidate.QMIControl != filepath.Join(devRoot, "wwan0qmi0") { @@ -290,6 +408,23 @@ func TestSysFSDiscoveryFindsWWANFromDevNodesWithoutClassDirectory(t *testing.T) } } +func TestSelectWWANATPortPrefersSecondaryATPort(t *testing.T) { + ports := []Port{ + {Name: "wwan0at0", InterfaceNumber: 0, Role: PortRoleAT}, + {Name: "wwan0at1", InterfaceNumber: 1, Role: PortRoleAT}, + } + if got := selectWWANATPort(ports); got.Name != "wwan0at1" { + t.Fatalf("selectWWANATPort = %#v, want wwan0at1", got) + } +} + +func TestSelectWWANATPortFallsBackToPrimaryWhenOnlyAT0(t *testing.T) { + ports := []Port{{Name: "wwan0at0", InterfaceNumber: 0, Role: PortRoleAT}} + if got := selectWWANATPort(ports); got.Name != "wwan0at0" { + t.Fatalf("selectWWANATPort = %#v, want wwan0at0", got) + } +} + func TestParseWWANPortName(t *testing.T) { for _, test := range []struct { name, index, kind string @@ -323,3 +458,18 @@ func mustMkdir(t *testing.T, path string) { t.Fatal(err) } } + +// mustBindQMIWWAN mimics the kernel's driver-binding directory entry: it adds +// interfaceName (e.g. "1-6:1.4") under /sys/bus/usb/drivers/qmi_wwan exactly +// like the real qmi_wwan driver directory does for a bound QMI interface. +func mustBindQMIWWAN(t *testing.T, sysRoot, interfaceName string) { + t.Helper() + driverDir := filepath.Join(sysRoot, "bus", "usb", "drivers", "qmi_wwan") + if err := os.MkdirAll(driverDir, 0o700); err != nil { + t.Fatal(err) + } + target := filepath.Join(sysRoot, "bus", "usb", "devices", interfaceName) + if err := os.Symlink(target, filepath.Join(driverDir, interfaceName)); err != nil { + t.Fatal(err) + } +} diff --git a/internal/modem/session.go b/internal/modem/session.go index ea1a1cb..bcc6fc0 100644 --- a/internal/modem/session.go +++ b/internal/modem/session.go @@ -148,14 +148,19 @@ func (session *Session) executeLocked(ctx context.Context, command string) (Resp if err := ctx.Err(); err != nil { return response, err } - if err := writeAll(session.transport, []byte(command+"\r")); err != nil { - session.poisonLocked() - return response, fmt.Errorf("write %s: %w", command, err) - } + // Drain the transport before writing the command. Serial transports wait + // for any pending output here (a no-op after a synchronous command), while + // WWAN transports discard bytes left over from a previous command that + // timed out; without this, a late reply (e.g. a slow CGSN response) would + // be mis-parsed as this command's output. if err := drainTransport(ctx, session.transport); err != nil { session.poisonLocked() return response, fmt.Errorf("drain %s: %w", command, err) } + if err := writeAll(session.transport, []byte(command+"\r")); err != nil { + session.poisonLocked() + return response, fmt.Errorf("write %s: %w", command, err) + } return session.readFinalLocked(ctx, started, command, "", response) } diff --git a/internal/modem/session_test.go b/internal/modem/session_test.go index 480f43d..04b9194 100644 --- a/internal/modem/session_test.go +++ b/internal/modem/session_test.go @@ -453,6 +453,62 @@ func TestSessionExecutePromptRejectsUnsafeInput(t *testing.T) { } } +// drainOrderTransport forwards to an inner Transport while recording +// transport-level events, so a test can assert the exact order of Drain and +// Write calls. +type drainOrderTransport struct { + inner Transport + events chan string +} + +func (transport *drainOrderTransport) Write(payload []byte) (int, error) { + transport.events <- "write:" + string(payload) + return transport.inner.Write(payload) +} + +func (transport *drainOrderTransport) Read(buffer []byte) (int, error) { + return transport.inner.Read(buffer) +} + +func (transport *drainOrderTransport) Drain() error { + transport.events <- "drain" + return transport.inner.Drain() +} + +func (transport *drainOrderTransport) ResetInputBuffer() error { + return transport.inner.ResetInputBuffer() +} + +func (transport *drainOrderTransport) SetReadTimeout(timeout time.Duration) error { + return transport.inner.SetReadTimeout(timeout) +} + +func (transport *drainOrderTransport) Close() error { + return transport.inner.Close() +} + +// WWAN transports discard stale bytes left over from a timed-out command +// inside Drain, so the session must call it before writing the next command; +// otherwise a late reply (e.g. a slow CGSN response) would be mis-parsed as +// the new command's output. +func TestSessionDrainsBeforeWritingCommand(t *testing.T) { + inner := &transcriptTransport{steps: []transportStep{{ + write: "AT+CSQ\r", + chunks: []string{"\r\n+CSQ: 24,99\r\nOK\r\n"}, + }}} + events := make(chan string, 8) + session := newTestSession(t, &drainOrderTransport{inner: inner, events: events}) + if _, err := session.Execute(context.Background(), "AT+CSQ"); err != nil { + t.Fatalf("Execute: %v", err) + } + if first := <-events; first != "drain" { + t.Fatalf("first transport event = %q, want drain before the command write", first) + } + if second := <-events; second != "write:AT+CSQ\r" { + t.Fatalf("second transport event = %q, want the command write", second) + } +} + func newTestSession(t *testing.T, transport Transport) *Session { t.Helper() session, err := NewSession(transport, SessionOptions{ diff --git a/internal/modem/wwan_at_linux.go b/internal/modem/wwan_at_linux.go index 148f7d5..3251b4f 100644 --- a/internal/modem/wwan_at_linux.go +++ b/internal/modem/wwan_at_linux.go @@ -108,8 +108,27 @@ func (transport *nativeWWANATTransport) Drain() error { return io.ErrClosedPipe } // WWAN character-device writes are handed to the modem synchronously and - // have no termios output queue to drain. - return nil + // have no termios output queue to drain. A previous command that timed out + // can leave late bytes in the input buffer (e.g. a slow CGSN reply that + // arrives after the command deadline); discard them here so the next + // command starts from a clean stream instead of mis-parsing stale output. + buffer := make([]byte, 4096) + for { + fds := []unix.PollFd{{Fd: int32(transport.fd), Events: unix.POLLIN}} + ready, err := unix.Poll(fds, 0) + if err != nil { + return err + } + if ready == 0 || fds[0].Revents&unix.POLLIN == 0 { + return nil + } + if _, err := unix.Read(transport.fd, buffer); err != nil { + if errors.Is(err, unix.EINTR) || errors.Is(err, unix.EAGAIN) { + continue + } + return err + } + } } func (transport *nativeWWANATTransport) ResetInputBuffer() error { diff --git a/internal/modem/wwan_at_linux_test.go b/internal/modem/wwan_at_linux_test.go new file mode 100644 index 0000000..19481d1 --- /dev/null +++ b/internal/modem/wwan_at_linux_test.go @@ -0,0 +1,78 @@ +//go:build linux + +package modem + +import ( + "errors" + "io" + "testing" + + "golang.org/x/sys/unix" +) + +// TestNativeWWANATTransportDrainDiscardsPendingBytes verifies Drain discards +// every byte already buffered on the transport. A command that timed out (e.g. +// AT+CGSN on an MHI modem that never answers OK) can leave its late reply in +// the input buffer; the next command's Drain must clear it, however much data +// is pending, before the session writes the new command. +func TestNativeWWANATTransportDrainDiscardsPendingBytes(t *testing.T) { + readFD, writeFD := socketpair(t) + defer unix.Close(writeFD) + + // More than one 4096-byte Drain read: a slow CGSN reply (echo + IMEI + + // trailing CRLF) can exceed a single buffer. + payload := make([]byte, 12000) + for index := range payload { + payload[index] = byte('A' + index%26) + } + payload = append(payload, []byte("\r\n+CGSN: 357091089453326\r\n")...) + if _, err := unix.Write(writeFD, payload); err != nil { + t.Fatalf("seed stale bytes: %v", err) + } + + transport := &nativeWWANATTransport{fd: readFD, readTimeout: -1} + if err := transport.Drain(); err != nil { + t.Fatalf("Drain: %v", err) + } + assertNoPendingBytes(t, readFD, "after Drain") + + // Draining a clean transport is a fast no-op that must not block or error. + if err := transport.Drain(); err != nil { + t.Fatalf("second Drain: %v", err) + } +} + +// TestNativeWWANATTransportDrainRejectsClosedTransport covers the guard that +// keeps a poisoned session from draining a wedged, already-closed fd. +func TestNativeWWANATTransportDrainRejectsClosedTransport(t *testing.T) { + readFD, writeFD := socketpair(t) + defer unix.Close(writeFD) + transport := &nativeWWANATTransport{fd: readFD, readTimeout: -1} + if err := transport.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if err := transport.Drain(); !errors.Is(err, io.ErrClosedPipe) { + t.Fatalf("Drain after Close = %v, want ErrClosedPipe", err) + } +} + +func socketpair(t *testing.T) (int, int) { + t.Helper() + fds, err := unix.Socketpair(unix.AF_UNIX, unix.SOCK_STREAM, 0) + if err != nil { + t.Fatal(err) + } + return fds[0], fds[1] +} + +func assertNoPendingBytes(t *testing.T, fd int, context string) { + t.Helper() + fds := []unix.PollFd{{Fd: int32(fd), Events: unix.POLLIN}} + ready, err := unix.Poll(fds, 0) + if err != nil { + t.Fatalf("poll %s: %v", context, err) + } + if ready != 0 { + t.Fatalf("%s: fd still readable", context) + } +} diff --git a/internal/server/at_guard_test.go b/internal/server/at_guard_test.go index 773c1d4..914826d 100644 --- a/internal/server/at_guard_test.go +++ b/internal/server/at_guard_test.go @@ -1,6 +1,15 @@ package server -import "testing" +import ( + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "vocat/internal/device" + "vocat/internal/modem" +) func TestValidateATCommandBlocksTrafficMessagingAndDialActions(t *testing.T) { t.Parallel() @@ -44,3 +53,62 @@ func TestValidateATCommandAllowsReadOnlyStatusQueries(t *testing.T) { } } } + +// The AT terminal must present ERROR / +CME ERROR as a normal response, not as +// a 502. Before the CommandError branch was restored, every unsupported or +// SIM-less command was folded into "the device operation failed", hiding the +// real reason from the user. +func TestHandleATSurfacesCommandErrorAsResponse(t *testing.T) { + controller := fakeDeviceController{ + entry: device.Device{ID: "dev1"}, + atHandler: func(command string) (modem.Response, error) { + return modem.Response{}, &modem.CommandError{ + Command: command, + Final: "+CME ERROR: 10", + Lines: []string{"+CME ERROR: 10"}, + } + }, + } + server := &Server{devices: controller, logger: regionTestLogger(), maxRequestBodyBytes: 1 << 20} + recorder := httptest.NewRecorder() + request := httptest.NewRequest( + http.MethodPost, + "/api/devices/dev1/actions/at", + strings.NewReader(`{"cmd":"AT+CPIN?","timeout_ms":5000}`), + ) + request.Header.Set("Content-Type", "application/json") + + if !server.handleAT(recorder, request, "dev1") { + t.Fatal("handleAT returned false") + } + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body=%s)", recorder.Code, recorder.Body.String()) + } + data := decodeData(t, recorder) + response, _ := data["response"].(string) + if !strings.Contains(response, "+CME ERROR: 10") { + t.Fatalf("response = %q, want +CME ERROR text", response) + } +} + +func TestHandleATMapsNonCommandErrorTo502(t *testing.T) { + controller := fakeDeviceController{ + entry: device.Device{ID: "dev1"}, + atErr: errors.New("transport wedged"), + } + server := &Server{devices: controller, logger: regionTestLogger(), maxRequestBodyBytes: 1 << 20} + recorder := httptest.NewRecorder() + request := httptest.NewRequest( + http.MethodPost, + "/api/devices/dev1/actions/at", + strings.NewReader(`{"cmd":"AT+CSQ"}`), + ) + request.Header.Set("Content-Type", "application/json") + + if !server.handleAT(recorder, request, "dev1") { + t.Fatal("handleAT returned false") + } + if recorder.Code != http.StatusBadGateway { + t.Fatalf("status = %d, want 502", recorder.Code) + } +} diff --git a/internal/server/device_api.go b/internal/server/device_api.go index 12b7a36..e0ba483 100644 --- a/internal/server/device_api.go +++ b/internal/server/device_api.go @@ -1059,6 +1059,26 @@ func (s *Server) handleAT(w http.ResponseWriter, r *http.Request, id string) boo defer cancel() response, err := s.devices.ExecuteAT(ctx, id, command) if err != nil { + var commandErr *modem.CommandError + if errors.As(err, &commandErr) { + // The modem answered with ERROR / +CME ERROR. An AT terminal must + // surface that text (including the CME detail) as a normal response; + // folding it into a 502 hides the real reason from the user. + text := strings.Join(commandErr.Lines, "\n") + if text != "" { + text += "\n" + } + text += commandErr.Final + writeJSON(w, http.StatusOK, map[string]any{ + "data": map[string]any{ + "response": text, + "final": commandErr.Final, + "duration_ms": 0, + "urcs": []string{}, + }, + }) + return true + } s.writeDeviceError(w, err) return true }