Various minor refactors (#1501)

* Factored pluggable monitor creation

* Factored out code duplication

* EnumerateMonitorPortSettings does require only protocol

* Monitor.Open does require only address and protocol

* Added GetPortAddressAndProtocol helper

* fix i18n

* Apply suggestions from code review

Co-authored-by: Silvano Cerza <[email protected]>

Author: cmaglie

arduino/monitor/monitor.go

@@ -29,7 +29,6 @@ import (
 	"github.com/arduino/arduino-cli/cli/globals"
 	"github.com/arduino/arduino-cli/executils"
 	"github.com/arduino/arduino-cli/i18n"
-	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/sirupsen/logrus"
 )
 
@@ -264,9 +263,9 @@ func (mon *PluggableMonitor) Configure(param, value string) error {
 }
 
 // Open connects to the given Port. A communication channel is opened
-func (mon *PluggableMonitor) Open(port *rpc.Port) (io.ReadWriter, error) {
-	if port.Protocol != mon.supportedProtocol {
-		return nil, fmt.Errorf("invalid monitor protocol '%s': only '%s' is accepted", port.Protocol, mon.supportedProtocol)
+func (mon *PluggableMonitor) Open(portAddress, portProtocol string) (io.ReadWriter, error) {
+	if portProtocol != mon.supportedProtocol {
+		return nil, fmt.Errorf("invalid monitor protocol '%s': only '%s' is accepted", portProtocol, mon.supportedProtocol)
 	}
 
 	tcpListener, err := net.Listen("tcp", "127.0.0.1:")
@@ -276,7 +275,7 @@ func (mon *PluggableMonitor) Open(port *rpc.Port) (io.ReadWriter, error) {
 	defer tcpListener.Close()
 	tcpListenerPort := tcpListener.Addr().(*net.TCPAddr).Port
 
-	if err := mon.sendCommand(fmt.Sprintf("OPEN 127.0.0.1:%d %s\n", tcpListenerPort, port.Address)); err != nil {
+	if err := mon.sendCommand(fmt.Sprintf("OPEN 127.0.0.1:%d %s\n", tcpListenerPort, portAddress)); err != nil {
 		return nil, err
 	}
 	if _, err := mon.waitMessage(time.Second*10, "open"); err != nil {

arduino/monitor/monitor_test.go

@@ -24,7 +24,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/arduino/arduino-cli/arduino/discovery"
 	"github.com/arduino/arduino-cli/executils"
 	"github.com/arduino/go-paths-helper"
 	"github.com/stretchr/testify/require"
@@ -59,12 +58,11 @@ func TestDummyMonitor(t *testing.T) {
 	err = mon.Configure("speed", "38400")
 	require.NoError(t, err)
 
-	port := &discovery.Port{Address: "/dev/ttyACM0", Protocol: "test"}
-	rw, err := mon.Open(port.ToRPC())
+	rw, err := mon.Open("/dev/ttyACM0", "test")
 	require.NoError(t, err)
 
 	// Double open -> error: port already opened
-	_, err = mon.Open(port.ToRPC())
+	_, err = mon.Open("/dev/ttyACM0", "test")
 	require.Error(t, err)
 
 	// Write "TEST"
@@ -93,7 +91,7 @@ func TestDummyMonitor(t *testing.T) {
 	time.Sleep(100 * time.Millisecond)
 	require.Equal(t, int32(1), atomic.LoadInt32(&completed))
 
-	rw, err = mon.Open(port.ToRPC())
+	rw, err = mon.Open("/dev/ttyACM0", "test")
 	require.NoError(t, err)
 	n, err = rw.Write([]byte("TEST"))
 	require.NoError(t, err)

cli/arguments/port.go

@@ -46,6 +46,21 @@ func (p *Port) AddToCommand(cmd *cobra.Command) {
 	cmd.Flags().DurationVar(&p.timeout, "discovery-timeout", 5*time.Second, tr("Max time to wait for port discovery, e.g.: 30s, 1m"))
 }
 
+// GetPortAddressAndProtocol returns only the port address and the port protocol
+// without any other port metadata obtained from the discoveries. This method allows
+// to bypass the discoveries unless the protocol is not specified: in this
+// case the discoveries are needed to autodetect the protocol.
+func (p *Port) GetPortAddressAndProtocol(instance *rpc.Instance, sk *sketch.Sketch) (string, string, error) {
+	if p.protocol != "" {
+		return p.address, p.protocol, nil
+	}
+	port, err := p.GetPort(instance, sk)
+	if err != nil {
+		return "", "", err
+	}
+	return port.Address, port.Protocol, nil
+}
+
 // GetPort returns the Port obtained by parsing command line arguments.
 // The extra metadata for the ports is obtained using the pluggable discoveries.
 func (p *Port) GetPort(instance *rpc.Instance, sk *sketch.Sketch) (*discovery.Port, error) {

cli/monitor/monitor.go

@@ -70,23 +70,23 @@ func runMonitorCmd(cmd *cobra.Command, args []string) {
 		quiet = true
 	}
 
-	port, err := portArgs.GetPort(instance, nil)
+	portAddress, portProtocol, err := portArgs.GetPortAddressAndProtocol(instance, nil)
 	if err != nil {
 		feedback.Error(err)
 		os.Exit(errorcodes.ErrGeneric)
 	}
 
+	enumerateResp, err := monitor.EnumerateMonitorPortSettings(context.Background(), &rpc.EnumerateMonitorPortSettingsRequest{
+		Instance:     instance,
+		PortProtocol: portProtocol,
+		Fqbn:         "",
+	})
+	if err != nil {
+		feedback.Error(tr("Error getting port settings details: %s", err))
+		os.Exit(errorcodes.ErrGeneric)
+	}
 	if describe {
-		res, err := monitor.EnumerateMonitorPortSettings(context.Background(), &rpc.EnumerateMonitorPortSettingsRequest{
-			Instance: instance,
-			Port:     port.ToRPC(),
-			Fqbn:     "",
-		})
-		if err != nil {
-			feedback.Error(tr("Error getting port settings details: %s"), err)
-			os.Exit(errorcodes.ErrGeneric)
-		}
-		feedback.PrintResult(&detailsResult{Settings: res.Settings})
+		feedback.PrintResult(&detailsResult{Settings: enumerateResp.Settings})
 		return
 	}
 
@@ -99,15 +99,6 @@ func runMonitorCmd(cmd *cobra.Command, args []string) {
 
 	configuration := &rpc.MonitorPortConfiguration{}
 	if len(configs) > 0 {
-		resp, err := monitor.EnumerateMonitorPortSettings(context.Background(), &rpc.EnumerateMonitorPortSettingsRequest{
-			Instance: instance,
-			Port:     port.ToRPC(),
-			Fqbn:     "",
-		})
-		if err != nil {
-			feedback.Error(err)
-			os.Exit(errorcodes.ErrGeneric)
-		}
 		for _, config := range configs {
 			split := strings.SplitN(config, "=", 2)
 			k := ""
@@ -118,7 +109,7 @@ func runMonitorCmd(cmd *cobra.Command, args []string) {
 			}
 
 			var setting *rpc.MonitorPortSettingDescriptor
-			for _, s := range resp.GetSettings() {
+			for _, s := range enumerateResp.GetSettings() {
 				if k == "" {
 					if contains(s.EnumValues, v) {
 						setting = s
@@ -151,7 +142,7 @@ func runMonitorCmd(cmd *cobra.Command, args []string) {
 	}
 	portProxy, _, err := monitor.Monitor(context.Background(), &rpc.MonitorRequest{
 		Instance:          instance,
-		Port:              port.ToRPC(),
+		Port:              &rpc.Port{Address: portAddress, Protocol: portProtocol},
 		Fqbn:              "",
 		PortConfiguration: configuration,
 	})
@@ -178,7 +169,7 @@ func runMonitorCmd(cmd *cobra.Command, args []string) {
 	}()
 
 	if !quiet {
-		feedback.Print(tr("Connected to %s! Press CTRL-C to exit.", port.String()))
+		feedback.Print(tr("Connected to %s! Press CTRL-C to exit.", portAddress))
 	}
 
 	// Wait for port closed

commands/monitor/monitor.go

@@ -59,18 +59,11 @@ func Monitor(ctx context.Context, req *rpc.MonitorRequest) (*PortProxy, *pluggab
 		return nil, nil, &commands.InvalidInstanceError{}
 	}
 
-	monitorRef, err := findMonitorForProtocolAndBoard(pm, req.GetPort(), req.GetFqbn())
+	m, err := findMonitorForProtocolAndBoard(pm, req.GetPort().GetProtocol(), req.GetFqbn())
 	if err != nil {
 		return nil, nil, err
 	}
 
-	tool := pm.FindMonitorDependency(monitorRef)
-	if tool == nil {
-		return nil, nil, &commands.MonitorNotFoundError{Monitor: monitorRef.String()}
-	}
-
-	m := pluggableMonitor.New(monitorRef.Name, tool.InstallDir.Join(monitorRef.Name).String())
-
 	if err := m.Run(); err != nil {
 		return nil, nil, &commands.FailedMonitorError{Cause: err}
 	}
@@ -80,7 +73,7 @@ func Monitor(ctx context.Context, req *rpc.MonitorRequest) (*PortProxy, *pluggab
 		return nil, nil, &commands.FailedMonitorError{Cause: err}
 	}
 
-	monIO, err := m.Open(req.GetPort())
+	monIO, err := m.Open(req.GetPort().GetAddress(), req.GetPort().GetProtocol())
 	if err != nil {
 		return nil, nil, &commands.FailedMonitorError{Cause: err}
 	}
@@ -95,15 +88,13 @@ func Monitor(ctx context.Context, req *rpc.MonitorRequest) (*PortProxy, *pluggab
 	}, descriptor, nil
 }
 
-func findMonitorForProtocolAndBoard(pm *packagemanager.PackageManager, port *rpc.Port, fqbn string) (*cores.MonitorDependency, error) {
-	if port == nil {
-		return nil, &commands.MissingPortError{}
-	}
-	protocol := port.GetProtocol()
+func findMonitorForProtocolAndBoard(pm *packagemanager.PackageManager, protocol, fqbn string) (*pluggableMonitor.PluggableMonitor, error) {
 	if protocol == "" {
 		return nil, &commands.MissingPortProtocolError{}
 	}
 
+	var monitorDepOrRecipe *cores.MonitorDependency
+
 	// If a board is specified search the monitor in the board package first
 	if fqbn != "" {
 		fqbn, err := cores.ParseFQBN(fqbn)
@@ -115,16 +106,33 @@ func findMonitorForProtocolAndBoard(pm *packagemanager.PackageManager, port *rpc
 		if err != nil {
 			return nil, &commands.UnknownFQBNError{Cause: err}
 		}
+
 		if mon, ok := boardPlatform.Monitors[protocol]; ok {
-			return mon, nil
+			monitorDepOrRecipe = mon
 		}
 	}
 
-	// Otherwise look in all package for a suitable monitor
-	for _, platformRel := range pm.InstalledPlatformReleases() {
-		if mon, ok := platformRel.Monitors[protocol]; ok {
-			return mon, nil
+	if monitorDepOrRecipe == nil {
+		// Otherwise look in all package for a suitable monitor
+		for _, platformRel := range pm.InstalledPlatformReleases() {
+			if mon, ok := platformRel.Monitors[protocol]; ok {
+				monitorDepOrRecipe = mon
+				break
+			}
 		}
 	}
-	return nil, &commands.NoMonitorAvailableForProtocolError{Protocol: protocol}
+
+	if monitorDepOrRecipe == nil {
+		return nil, &commands.NoMonitorAvailableForProtocolError{Protocol: protocol}
+	}
+
+	tool := pm.FindMonitorDependency(monitorDepOrRecipe)
+	if tool == nil {
+		return nil, &commands.MonitorNotFoundError{Monitor: monitorDepOrRecipe.String()}
+	}
+
+	return pluggableMonitor.New(
+		monitorDepOrRecipe.Name,
+		tool.InstallDir.Join(monitorDepOrRecipe.Name).String(),
+	), nil
 }

commands/monitor/settings.go

@@ -30,18 +30,11 @@ func EnumerateMonitorPortSettings(ctx context.Context, req *rpc.EnumerateMonitor
 		return nil, &commands.InvalidInstanceError{}
 	}
 
-	monitorRef, err := findMonitorForProtocolAndBoard(pm, req.GetPort(), req.GetFqbn())
+	m, err := findMonitorForProtocolAndBoard(pm, req.GetPortProtocol(), req.GetFqbn())
 	if err != nil {
 		return nil, err
 	}
 
-	tool := pm.FindMonitorDependency(monitorRef)
-	if tool == nil {
-		return nil, &commands.MonitorNotFoundError{Monitor: monitorRef.String()}
-	}
-
-	m := pluggableMonitor.New(monitorRef.Name, tool.InstallDir.Join(monitorRef.Name).String())
-
 	if err := m.Run(); err != nil {
 		return nil, &commands.FailedMonitorError{Cause: err}
 	}

i18n/data/en.po

@@ -427,7 +427,7 @@ msgstr "Configuring platform."
 msgid "Connected"
 msgstr "Connected"
 
-#: cli/monitor/monitor.go:181
+#: cli/monitor/monitor.go:172
 msgid "Connected to %s! Press CTRL-C to exit."
 msgstr "Connected to %s! Press CTRL-C to exit."
 
@@ -504,7 +504,7 @@ msgstr "Debugging not supported for board %s"
 msgid "Debugging supported:"
 msgstr "Debugging supported:"
 
-#: cli/monitor/monitor.go:199
+#: cli/monitor/monitor.go:190
 msgid "Default"
 msgstr "Default"
 
@@ -790,7 +790,7 @@ msgstr "Error getting information for library %s"
 msgid "Error getting libraries info: %v"
 msgstr "Error getting libraries info: %v"
 
-#: cli/monitor/monitor.go:86
+#: cli/monitor/monitor.go:85
 msgid "Error getting port settings details: %s"
 msgstr "Error getting port settings details: %s"
 
@@ -1138,7 +1138,7 @@ msgstr "Global variables use {0} bytes of dynamic memory."
 
 #: cli/core/list.go:84
 #: cli/core/search.go:114
-#: cli/monitor/monitor.go:199
+#: cli/monitor/monitor.go:190
 #: cli/outdated/outdated.go:62
 msgid "ID"
 msgstr "ID"
@@ -1475,7 +1475,7 @@ msgstr "Missing sketch path"
 msgid "Monitor '%s' not found"
 msgstr "Monitor '%s' not found"
 
-#: cli/monitor/monitor.go:147
+#: cli/monitor/monitor.go:138
 msgid "Monitor port settings:"
 msgstr "Monitor port settings:"
 
@@ -1731,8 +1731,8 @@ msgstr "Platform size (bytes):"
 msgid "Port"
 msgstr "Port"
 
-#: cli/monitor/monitor.go:168
-#: cli/monitor/monitor.go:175
+#: cli/monitor/monitor.go:159
+#: cli/monitor/monitor.go:166
 msgid "Port closed:"
 msgstr "Port closed:"
 
@@ -1873,7 +1873,7 @@ msgstr "Sets a setting value."
 msgid "Sets where to save the configuration file."
 msgstr "Sets where to save the configuration file."
 
-#: cli/monitor/monitor.go:199
+#: cli/monitor/monitor.go:190
 msgid "Setting"
 msgstr "Setting"
 
@@ -2348,7 +2348,7 @@ msgstr "VERSION"
 msgid "VERSION_NUMBER"
 msgstr "VERSION_NUMBER"
 
-#: cli/monitor/monitor.go:199
+#: cli/monitor/monitor.go:190
 msgid "Values"
 msgstr "Values"
 
@@ -2503,7 +2503,7 @@ msgstr "cleaning build path"
 msgid "command"
 msgstr "command"
 
-#: arduino/monitor/monitor.go:150
+#: arduino/monitor/monitor.go:149
 msgid "command '%[1]s' failed: %[2]s"
 msgstr "command '%[1]s' failed: %[2]s"
 
@@ -2516,7 +2516,7 @@ msgstr "command '%[1]s' failed: %[2]s"
 msgid "command failed: %s"
 msgstr "command failed: %s"
 
-#: arduino/monitor/monitor.go:147
+#: arduino/monitor/monitor.go:146
 msgid "communication out of sync, expected '%[1]s', received '%[2]s'"
 msgstr "communication out of sync, expected '%[1]s', received '%[2]s'"
 
@@ -2855,11 +2855,11 @@ msgstr "invalid platform archive size: %s"
 msgid "invalid pluggable monitor reference: %s"
 msgstr "invalid pluggable monitor reference: %s"
 
-#: cli/monitor/monitor.go:130
+#: cli/monitor/monitor.go:121
 msgid "invalid port configuration value for %s: %s"
 msgstr "invalid port configuration value for %s: %s"
 
-#: cli/monitor/monitor.go:139
+#: cli/monitor/monitor.go:130
 msgid "invalid port configuration: %s"
 msgstr "invalid port configuration: %s"
 
@@ -3106,11 +3106,11 @@ msgstr "pluggable discovery already added: %s"
 msgid "port"
 msgstr "port"
 
-#: cli/arguments/port.go:122
+#: cli/arguments/port.go:137
 msgid "port not found: %[1]s %[2]s"
 msgstr "port not found: %[1]s %[2]s"
 
-#: arduino/monitor/monitor.go:239
+#: arduino/monitor/monitor.go:238
 msgid "protocol version not supported: requested %[1]d, got %[2]d"
 msgstr "protocol version not supported: requested %[1]d, got %[2]d"
 
@@ -3300,7 +3300,7 @@ msgstr "the platform has no releases"
 msgid "the server responded with status %s"
 msgstr "the server responded with status %s"
 
-#: arduino/monitor/monitor.go:140
+#: arduino/monitor/monitor.go:139
 msgid "timeout waiting for message"
 msgstr "timeout waiting for message"