[RSDK-7193] Better state machine and timings for wifi provisioning (#15)

go.mod

@@ -3,31 +3,28 @@ module github.com/viamrobotics/agent-provisioning
 go 1.21.4
 
 require (
-	github.com/Wifx/gonetworkmanager/v2 v2.1.0
-	github.com/google/uuid v1.4.0
+	github.com/Otterverse/gonetworkmanager/v2 v2.2.0
+	github.com/edaniels/golog v0.0.0-20230215213219-28954395e8d0
+	github.com/google/uuid v1.6.0
+	github.com/jessevdk/go-flags v1.5.0
+	github.com/pkg/errors v0.9.1
 	go.uber.org/zap v1.24.0
 	go.viam.com/api v0.1.260
+	google.golang.org/grpc v1.61.0
 )
 
 require (
 	github.com/benbjohnson/clock v1.1.0 // indirect
+	github.com/godbus/dbus/v5 v5.1.0 // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.11.3 // indirect
 	go.uber.org/atomic v1.10.0 // indirect
 	go.uber.org/multierr v1.9.0 // indirect
-	golang.org/x/net v0.18.0 // indirect
-	golang.org/x/sys v0.14.0 // indirect
+	golang.org/x/net v0.24.0 // indirect
+	golang.org/x/sys v0.19.0 // indirect
 	golang.org/x/text v0.14.0 // indirect
 	google.golang.org/genproto v0.0.0-20231106174013-bbf56f31fb17 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20231106174013-bbf56f31fb17 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20231106174013-bbf56f31fb17 // indirect
 	google.golang.org/protobuf v1.33.0 // indirect
 )
-
-require (
-	github.com/edaniels/golog v0.0.0-20230215213219-28954395e8d0
-	github.com/godbus/dbus/v5 v5.1.0 // indirect
-	github.com/jessevdk/go-flags v1.5.0
-	github.com/pkg/errors v0.9.1
-	google.golang.org/grpc v1.61.0
-)

go.sum

@@ -18,9 +18,9 @@ github.com/Djarvur/go-err113 v0.0.0-20210108212216-aea10b59be24/go.mod h1:4UJr5H
 github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y=
 github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU=
 github.com/OpenPeeDeeP/depguard v1.0.1/go.mod h1:xsIw86fROiiwelg+jB2uM9PiKihMMmUx/1V+TNhjQvM=
+github.com/Otterverse/gonetworkmanager/v2 v2.2.0 h1:aYEOjBO2I+OMORNpCBXqvVgzHcGks5MWmKv0JmrT5po=
+github.com/Otterverse/gonetworkmanager/v2 v2.2.0/go.mod h1:Bc8kOugBgzCBC0R8oLa3wHnGet7k2ZpMHUobZtxlwhU=
 github.com/StackExchange/wmi v0.0.0-20190523213315-cbe66965904d/go.mod h1:3eOhrUMpNV+6aFIbp5/iudMxNCF27Vw2OZgy4xEx0Fg=
-github.com/Wifx/gonetworkmanager/v2 v2.1.0 h1:2PNs7P6wgOyc57YK7AKMwNxGCLvWU6zFBXoEILV4at8=
-github.com/Wifx/gonetworkmanager/v2 v2.1.0/go.mod h1:fMDb//SHsKWxyDUAwXvCqurV3npbIyyaQWenGpZ/uXg=
 github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
 github.com/alexkohler/prealloc v1.0.0/go.mod h1:VetnK3dIgFBBKmg0YnD9F9x6Icjd+9cvfHR56wJVlKE=
@@ -140,9 +140,8 @@ github.com/google/pprof v0.0.0-20181206194817-3ea8567a2e57/go.mod h1:zfwlbNMJ+OI
 github.com/google/pprof v0.0.0-20190515194954-54271f7e092f/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
 github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
 github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
-github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
-github.com/google/uuid v1.4.0 h1:MtMxsa51/r9yyhkyLsVeVt0B+BGQZzpQiTQ4eHZ8bc4=
-github.com/google/uuid v1.4.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
+github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
+github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+vpHVxEJEs9eg=
 github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
 github.com/gookit/color v1.3.6/go.mod h1:R3ogXq2B9rTbXoSHJ1HyUVAZ3poOJHpd9nQmyGZsfvQ=
@@ -435,8 +434,8 @@ golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v
 golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
 golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
 golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
-golang.org/x/net v0.18.0 h1:mIYleuAkSbHh0tCv7RvjL3F6ZVbLjq4+R7zbOn3Kokg=
-golang.org/x/net v0.18.0/go.mod h1:/czyP5RqHAH4odGYxBJ1qz0+CE5WZ+2j1YgoEo8F2jQ=
+golang.org/x/net v0.24.0 h1:1PcaxkF854Fu3+lvBIx5SYn9wRlBzzcnHZSiaFFAb0w=
+golang.org/x/net v0.24.0/go.mod h1:2Q7sJY5mzlzWjKtYUEXSlBWCdyaioyXzRB2RtU8KVE8=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
 golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
@@ -487,8 +486,8 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q=
-golang.org/x/sys v0.14.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/sys v0.19.0 h1:q5f1RH2jigJ1MoAWp2KTp3gm5zAGFUTarQZ5U386+4o=
+golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
 golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k=

networkmanager/networkmanager.go

@@ -14,7 +14,7 @@ import (
 	"sync"
 	"time"
 
-	gnm "github.com/Wifx/gonetworkmanager/v2"
+	gnm "github.com/Otterverse/gonetworkmanager/v2"
 	"github.com/google/uuid"
 	errw "github.com/pkg/errors"
 	"go.uber.org/zap"
@@ -35,12 +35,12 @@ const (
 var (
 	BindAddr = "10.42.0.1"
 	// older networkmanager requires unit32 arrays for IP addresses.
-	IPAsUint32                    = binary.LittleEndian.Uint32([]byte{10, 42, 0, 1})
-	ErrCouldNotActivateConnection = errors.New("could not activate connection")
-	ErrConnCheckDisabled          = errors.New("NetworkManager connectivity checking disabled by user, network management will be unavailable")
-	ErrNoActiveConnectionFound    = errors.New("no active connection found")
-	loopDelay                     = time.Second * 15
-	connectTimeout                = time.Second * 30
+	IPAsUint32                 = binary.LittleEndian.Uint32([]byte{10, 42, 0, 1})
+	ErrBadPassword             = errors.New("bad or missing password")
+	ErrConnCheckDisabled       = errors.New("NetworkManager connectivity checking disabled by user, network management will be unavailable")
+	ErrNoActiveConnectionFound = errors.New("no active connection found")
+	loopDelay                  = time.Second * 15
+	connectTimeout             = time.Second * 50 // longer than the 45 second timeout in NetworkManager
 )
 
 type NMWrapper struct {
@@ -506,7 +506,7 @@ func getSettingsHotspot(id, ssid, psk string) gnm.ConnectionSettings {
 }
 
 // StartProvisioning puts the wifi in hotspot mode and starts a captive portal.
-func (w *NMWrapper) StartProvisioning(ctx context.Context) error {
+func (w *NMWrapper) StartProvisioning(ctx context.Context, userInputChan chan struct{}) error {
 	provisioningMode, _ := w.state.getProvisioning()
 	if provisioningMode {
 		return errors.New("provisioning mode already started")
@@ -587,6 +587,9 @@ func (w *NMWrapper) StartProvisioning(ctx context.Context) error {
 					continue
 				}
 			}
+
+			// signal that the user sent stuff so we can break the main loop
+			userInputChan <- struct{}{}
 		}
 	}()
 
@@ -651,7 +654,9 @@ func (w *NMWrapper) activateConnection(ctx context.Context, ssid string) error {
 	}
 
 	nw.lastTried = now
-	w.lastSSID = ssid
+	if ssid != w.hotspotSSID {
+		w.lastSSID = ssid
+	}
 
 	w.logger.Infof("Activating connection for SSID: %s", ssid)
 	activeConnection, err := w.nm.ActivateConnection(nw.conn, w.dev, nil)
@@ -660,7 +665,7 @@ func (w *NMWrapper) activateConnection(ctx context.Context, ssid string) error {
 		return errw.Wrapf(err, "error activating connection for ssid: %s", ssid)
 	}
 
-	if err := waitForConnect(ctx, activeConnection); err != nil {
+	if err := w.waitForConnect(ctx); err != nil {
 		nw.lastError = err
 		return err
 	}
@@ -703,21 +708,37 @@ func (w *NMWrapper) deactivateConnection(ssid string) error {
 	return nil
 }
 
-func waitForConnect(ctx context.Context, conn gnm.ActiveConnection) error {
+func (w *NMWrapper) waitForConnect(ctx context.Context) error {
 	timeoutCtx, cancel := context.WithTimeout(ctx, connectTimeout)
 	defer cancel()
+
+	changeChan := make(chan gnm.DeviceStateChange, 32)
+	exitChan := make(chan struct{})
+	defer close(exitChan)
+
+	if err := w.dev.SubscribeState(changeChan, exitChan); err != nil {
+		return errw.Wrap(err, "monitoring connection activation")
+	}
+
 	for {
-		state, err := conn.GetPropertyState()
-		if err != nil {
-			// dbus errors are useless here, as when the connection fails, the object just goes away
-			// so we report our own instead
-			return ErrCouldNotActivateConnection
-		}
-		if state == gnm.NmActiveConnectionStateActivated {
-			return nil
-		}
-		if !provisioning.HealthySleep(timeoutCtx, time.Second) {
-			return errors.Join(err, ErrCouldNotActivateConnection)
+		select {
+		case update := <-changeChan:
+			w.logger.Debugf("%s->%s (%s)", update.OldState, update.NewState, update.Reason)
+			//nolint:exhaustive
+			switch update.NewState {
+			case gnm.NmDeviceStateActivated:
+				return nil
+			case gnm.NmDeviceStateFailed:
+				if update.Reason == gnm.NmDeviceStateReasonNoSecrets {
+					return ErrBadPassword
+				}
+				// custom error if it's some other reason for failure
+				return errw.Errorf("connection failed: %s", update.Reason)
+			default:
+			}
+
+		case <-timeoutCtx.Done():
+			return errw.Wrap(ctx.Err(), "waiting for network activation")
 		}
 	}
 }
@@ -909,10 +930,19 @@ func (w *NMWrapper) startStateMonitors(ctx context.Context) {
 func (w *NMWrapper) StartMonitoring(ctx context.Context) error {
 	w.startStateMonitors(ctx)
 
+	userInputChan := make(chan struct{}, 1)
+	var userInputReceived bool
+
 	for {
 		select {
 		case <-ctx.Done():
 			return nil
+		case <-userInputChan:
+			userInputReceived = true
+			// wait 3 seconds so responses can be sent to/seen by user
+			if !provisioning.HealthySleep(ctx, time.Second*3) {
+				return nil
+			}
 		case <-time.After(loopDelay):
 		}
 
@@ -928,8 +958,7 @@ func (w *NMWrapper) StartMonitoring(ctx context.Context) error {
 			// complex logic, so wasting some variables for readability
 
 			// portal interaction time is updated when a user loads a page or makes a grpc request
-			// it is reset (time.IsZero()==true) when a user actually submits config data
-			inactivePortal := w.cp.GetLastInteraction().Before(now.Add(time.Minute * -5))
+			inactivePortal := w.cp.GetLastInteraction().Before(now.Add(time.Minute*-5)) || userInputReceived
 
 			// exit/retry to test networks only if there's no recent user interaction AND configuration is present
 			haveCandidates := len(w.getCandidates()) > 0 && inactivePortal && isConfigured
@@ -943,16 +972,16 @@ func (w *NMWrapper) StartMonitoring(ctx context.Context) error {
 				if err := w.StopProvisioning(); err != nil {
 					w.logger.Error(err)
 				} else {
-					pMode, pModeChange = w.state.getProvisioning()
+					pMode, _ = w.state.getProvisioning()
 				}
 			}
 		}
 
-		// not in provisioning mode
 		if allGood || pMode {
 			continue
 		}
 
+		// not in provisioning mode
 		if !isOnline {
 			if w.tryCandidates(ctx) {
 				isOnline, lastOnline = w.state.getOnline()
@@ -963,10 +992,10 @@ func (w *NMWrapper) StartMonitoring(ctx context.Context) error {
 		}
 
 		// not in provisioning mode, so start it if not configured (/etc/viam.json)
-		// OR as long as we've been offline for at least two minutes AND out of provisioning for two minutes
+		// OR as long as we've been offline for at least two minutes
 		twoMinutesAgo := now.Add(time.Minute * -2)
-		if !isConfigured || (lastOnline.Before(twoMinutesAgo) && pModeChange.Before(twoMinutesAgo)) {
-			if err := w.StartProvisioning(ctx); err != nil {
+		if !isConfigured || (lastOnline.Before(twoMinutesAgo)) {
+			if err := w.StartProvisioning(ctx, userInputChan); err != nil {
 				w.logger.Error(err)
 			}
 		}

portal/grpc.go

@@ -3,6 +3,7 @@ package portal
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"time"
 
@@ -72,7 +73,11 @@ func (cp *CaptivePortal) SetNetworkCredentials(ctx context.Context,
 	cp.input.Updated = time.Now()
 	cp.input.SSID = req.GetSsid()
 	cp.input.PSK = req.GetPsk()
-	cp.inputRecieved.Store(true)
+	cp.inputReceived.Store(true)
+
+	if req.GetSsid() == cp.status.lastNetwork.SSID {
+		cp.status.lastNetwork.LastError = ""
+	}
 
 	return &pb.SetNetworkCredentialsResponse{}, nil
 }
@@ -93,7 +98,7 @@ func (cp *CaptivePortal) SetSmartMachineCredentials(ctx context.Context,
 	cp.input.PartID = cloud.GetId()
 	cp.input.Secret = cloud.GetSecret()
 	cp.input.AppAddr = cloud.GetAppAddress()
-	cp.inputRecieved.Store(true)
+	cp.inputReceived.Store(true)
 
 	return &pb.SetSmartMachineCredentialsResponse{}, nil
 }
@@ -115,9 +120,14 @@ func (cp *CaptivePortal) GetNetworkList(ctx context.Context,
 }
 
 func (cp *CaptivePortal) errListAsStrings() []string {
-	errList := make([]string, len(cp.status.errors))
-	for i, err := range cp.status.errors {
-		errList[i] = err.Error()
+	errList := []string{}
+
+	if cp.status.lastNetwork.LastError != "" {
+		errList = append(errList, fmt.Sprintf("SSID: %s: %s", cp.status.lastNetwork.SSID, cp.status.lastNetwork.LastError))
+	}
+
+	for _, err := range cp.status.errors {
+		errList = append(errList, err.Error())
 	}
 	return errList
 }

portal/portal.go

@@ -33,7 +33,7 @@ type CaptivePortal struct {
 	mu              sync.Mutex
 	lastInteraction time.Time
 	input           *provisioning.UserInput
-	inputRecieved   atomic.Bool
+	inputReceived   atomic.Bool
 	status          *deviceStatus
 
 	workers sync.WaitGroup
@@ -132,23 +132,24 @@ func (cp *CaptivePortal) Stop() error {
 	}
 
 	cp.input = &provisioning.UserInput{}
-	cp.inputRecieved.Store(false)
+	cp.inputReceived.Store(false)
 
 	return err
 }
 
 func (cp *CaptivePortal) GetUserInput() *provisioning.UserInput {
-	if cp.inputRecieved.Load() {
+	if cp.inputReceived.Load() {
 		cp.mu.Lock()
 		defer cp.mu.Unlock()
 		input := cp.input
 		// in case both network and device credentials are being updated
 		// only send user data after we've had it for ten seconds or if both are already set
-		if time.Now().After(input.Updated.Add(time.Second*10)) || (input.SSID != "" && input.PartID != "") {
+		if time.Now().After(input.Updated.Add(time.Second*10)) ||
+			(input.SSID != "" && input.PartID != "") ||
+			(input.SSID != "" && cp.status.deviceConfigured) ||
+			(input.PartID != "" && cp.status.online) {
 			cp.input = &provisioning.UserInput{}
-			cp.inputRecieved.Store(false)
-			// reset last interaction time since user seems to be done providing input
-			cp.lastInteraction = time.Time{}
+			cp.inputReceived.Store(false)
 			return input
 		}
 	}
@@ -277,7 +278,10 @@ func (cp *CaptivePortal) saveWifi(w http.ResponseWriter, r *http.Request) {
 			cp.status.banner += "Added credentials for SSID: " + cp.input.SSID
 		}
 
+		if ssid == cp.status.lastNetwork.SSID {
+			cp.status.lastNetwork.LastError = ""
+		}
 		cp.input.Updated = time.Now()
-		cp.inputRecieved.Store(true)
+		cp.inputReceived.Store(true)
 	}
 }