[skip changelog] Some PluggableMonitor client improvements (#1475)

* Addde QUIT command in monitor-client

* Use rpc.Port in pluggable monitor Open method

* Slightly improved monitor client error messages

* Fixed resource leaking in PluggableMonitor client

* fixed i18n

* Fixed test... ooops

arduino/monitor/monitor.go

@@ -24,10 +24,10 @@ import (
 	"sync"
 	"time"
 
-	"github.com/arduino/arduino-cli/arduino/discovery"
 	"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/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -45,6 +45,7 @@ const (
 // PluggableMonitor is a tool that communicates with a board through a communication port.
 type PluggableMonitor struct {
 	id                   string
+	processArgs          []string
 	process              *executils.Process
 	outgoingCommandsPipe io.Writer
 	incomingMessagesChan <-chan *monitorMessage
@@ -96,29 +97,12 @@ func (msg monitorMessage) String() string {
 var tr = i18n.Tr
 
 // New create and connect to the given pluggable monitor
-func New(id string, args ...string) (*PluggableMonitor, error) {
-	proc, err := executils.NewProcess(args...)
-	if err != nil {
-		return nil, err
-	}
-	stdout, err := proc.StdoutPipe()
-	if err != nil {
-		return nil, err
-	}
-	stdin, err := proc.StdinPipe()
-	if err != nil {
-		return nil, err
+func New(id string, args ...string) *PluggableMonitor {
+	return &PluggableMonitor{
+		id:          id,
+		processArgs: args,
+		state:       Dead,
 	}
-	messageChan := make(chan *monitorMessage)
-	disc := &PluggableMonitor{
-		id:                   id,
-		process:              proc,
-		incomingMessagesChan: messageChan,
-		outgoingCommandsPipe: stdin,
-		state:                Dead,
-	}
-	go disc.jsonDecodeLoop(stdout, messageChan)
-	return disc, nil
 }
 
 // GetID returns the identifier for this monitor
@@ -195,6 +179,25 @@ func (mon *PluggableMonitor) sendCommand(command string) error {
 
 func (mon *PluggableMonitor) runProcess() error {
 	logrus.Infof("starting monitor %s process", mon.id)
+	proc, err := executils.NewProcess(mon.processArgs...)
+	if err != nil {
+		return err
+	}
+	stdout, err := proc.StdoutPipe()
+	if err != nil {
+		return err
+	}
+	stdin, err := proc.StdinPipe()
+	if err != nil {
+		return err
+	}
+	mon.outgoingCommandsPipe = stdin
+	mon.process = proc
+
+	messageChan := make(chan *monitorMessage)
+	mon.incomingMessagesChan = messageChan
+	go mon.jsonDecodeLoop(stdout, messageChan)
+
 	if err := mon.process.Start(); err != nil {
 		return err
 	}
@@ -210,6 +213,9 @@ func (mon *PluggableMonitor) killProcess() error {
 	if err := mon.process.Kill(); err != nil {
 		return err
 	}
+	if err := mon.process.Wait(); err != nil {
+		return err
+	}
 	mon.statusMutex.Lock()
 	defer mon.statusMutex.Unlock()
 	mon.state = Dead
@@ -284,14 +290,14 @@ func (mon *PluggableMonitor) Configure(param, value string) error {
 	} else if msg.EventType != "configure" {
 		return errors.Errorf(tr("communication out of sync, expected 'configure', received '%s'"), msg.EventType)
 	} else if msg.Message != "OK" || msg.Error {
-		return errors.Errorf(tr("command failed: %s"), msg.Message)
+		return errors.Errorf(tr("configure failed: %s"), msg.Message)
 	} else {
 		return nil
 	}
 }
 
 // Open connects to the given Port. A communication channel is opened
-func (mon *PluggableMonitor) Open(port *discovery.Port) (io.ReadWriter, error) {
+func (mon *PluggableMonitor) Open(port *rpc.Port) (io.ReadWriter, error) {
 	mon.statusMutex.Lock()
 	defer mon.statusMutex.Unlock()
 
@@ -320,7 +326,7 @@ func (mon *PluggableMonitor) Open(port *discovery.Port) (io.ReadWriter, error) {
 	} else if msg.EventType != "open" {
 		return nil, errors.Errorf(tr("communication out of sync, expected 'open', received '%s'"), msg.EventType)
 	} else if msg.Message != "OK" || msg.Error {
-		return nil, errors.Errorf(tr("command failed: %s"), msg.Message)
+		return nil, errors.Errorf(tr("open failed: %s"), msg.Message)
 	}
 
 	conn, err := tcpListener.Accept()
@@ -354,3 +360,19 @@ func (mon *PluggableMonitor) Close() error {
 		return nil
 	}
 }
+
+// Quit terminates the monitor. No more commands can be accepted by the monitor.
+func (mon *PluggableMonitor) Quit() error {
+	if err := mon.sendCommand("QUIT\n"); err != nil {
+		return err
+	}
+	if msg, err := mon.waitMessage(time.Second * 10); err != nil {
+		return fmt.Errorf(tr("calling %[1]s: %[2]w"), "QUIT", err)
+	} else if msg.EventType != "quit" {
+		return errors.Errorf(tr("communication out of sync, expected 'quit', received '%s'"), msg.EventType)
+	} else if msg.Message != "OK" || msg.Error {
+		return errors.Errorf(tr("command failed: %s"), msg.Message)
+	}
+	mon.killProcess()
+	return nil
+}

arduino/monitor/monitor_test.go

@@ -44,8 +44,7 @@ func TestDummyMonitor(t *testing.T) {
 	require.NoError(t, builder.Run())
 
 	// Run dummy-monitor and test if everything is working as expected
-	mon, err := New("dummy", "testdata/dummy-monitor")
-	require.NoError(t, err)
+	mon := New("dummy", "testdata/dummy-monitor")
 	err = mon.Run()
 	require.NoError(t, err)
 
@@ -60,11 +59,12 @@ func TestDummyMonitor(t *testing.T) {
 	err = mon.Configure("speed", "38400")
 	require.NoError(t, err)
 
-	rw, err := mon.Open(&discovery.Port{Address: "/dev/ttyACM0", Protocol: "test"})
+	port := &discovery.Port{Address: "/dev/ttyACM0", Protocol: "test"}
+	rw, err := mon.Open(port.ToRPC())
 	require.NoError(t, err)
 
 	// Double open -> error: port already opened
-	_, err = mon.Open(&discovery.Port{Address: "/dev/ttyACM0", Protocol: "test"})
+	_, err = mon.Open(port.ToRPC())
 	require.Error(t, err)
 
 	// Write "TEST"
@@ -93,7 +93,7 @@ func TestDummyMonitor(t *testing.T) {
 	time.Sleep(100 * time.Millisecond)
 	require.Equal(t, int32(1), atomic.LoadInt32(&completed))
 
-	rw, err = mon.Open(&discovery.Port{Address: "/dev/ttyACM0", Protocol: "test"})
+	rw, err = mon.Open(port.ToRPC())
 	require.NoError(t, err)
 	n, err = rw.Write([]byte("TEST"))
 	require.NoError(t, err)

i18n/data/en.po

@@ -2394,7 +2394,8 @@ msgstr "boardname"
 #: arduino/discovery/discovery.go:375
 #: arduino/discovery/discovery.go:392
 #: arduino/discovery/discovery.go:415
-#: arduino/monitor/monitor.go:246
+#: arduino/monitor/monitor.go:252
+#: arduino/monitor/monitor.go:370
 msgid "calling %[1]s: %[2]w"
 msgstr "calling %[1]s: %[2]w"
 
@@ -2447,28 +2448,27 @@ msgstr "command"
 #: arduino/discovery/discovery.go:379
 #: arduino/discovery/discovery.go:396
 #: arduino/discovery/discovery.go:419
-#: arduino/monitor/monitor.go:250
-#: arduino/monitor/monitor.go:270
-#: arduino/monitor/monitor.go:287
-#: arduino/monitor/monitor.go:323
-#: arduino/monitor/monitor.go:351
+#: arduino/monitor/monitor.go:256
+#: arduino/monitor/monitor.go:276
+#: arduino/monitor/monitor.go:357
+#: arduino/monitor/monitor.go:374
 msgid "command failed: %s"
 msgstr "command failed: %s"
 
-#: arduino/monitor/monitor.go:349
+#: arduino/monitor/monitor.go:355
 msgid "communication out of sync, expected 'close', received '%s'"
 msgstr "communication out of sync, expected 'close', received '%s'"
 
-#: arduino/monitor/monitor.go:285
+#: arduino/monitor/monitor.go:291
 msgid "communication out of sync, expected 'configure', received '%s'"
 msgstr "communication out of sync, expected 'configure', received '%s'"
 
-#: arduino/monitor/monitor.go:268
+#: arduino/monitor/monitor.go:274
 msgid "communication out of sync, expected 'describe', received '%s'"
 msgstr "communication out of sync, expected 'describe', received '%s'"
 
 #: arduino/discovery/discovery.go:313
-#: arduino/monitor/monitor.go:248
+#: arduino/monitor/monitor.go:254
 msgid "communication out of sync, expected 'hello', received '%s'"
 msgstr "communication out of sync, expected 'hello', received '%s'"
 
@@ -2476,11 +2476,12 @@ msgstr "communication out of sync, expected 'hello', received '%s'"
 msgid "communication out of sync, expected 'list', received '%s'"
 msgstr "communication out of sync, expected 'list', received '%s'"
 
-#: arduino/monitor/monitor.go:321
+#: arduino/monitor/monitor.go:327
 msgid "communication out of sync, expected 'open', received '%s'"
 msgstr "communication out of sync, expected 'open', received '%s'"
 
 #: arduino/discovery/discovery.go:377
+#: arduino/monitor/monitor.go:372
 msgid "communication out of sync, expected 'quit', received '%s'"
 msgstr "communication out of sync, expected 'quit', received '%s'"
 
@@ -2500,6 +2501,10 @@ msgstr "communication out of sync, expected 'stop', received '%s'"
 msgid "computing hash: %s"
 msgstr "computing hash: %s"
 
+#: arduino/monitor/monitor.go:293
+msgid "configure failed: %s"
+msgstr "configure failed: %s"
+
 #: commands/upload/upload.go:611
 msgid "could not find a valid build artifact"
 msgstr "could not find a valid build artifact"
@@ -2962,6 +2967,10 @@ msgstr "no valid sketch found in %[1]s: missing %[2]s"
 msgid "no versions available for the current OS"
 msgstr "no versions available for the current OS"
 
+#: arduino/monitor/monitor.go:329
+msgid "open failed: %s"
+msgstr "open failed: %s"
+
 #: arduino/resources/checksums.go:72
 #: arduino/resources/install.go:59
 msgid "opening archive file: %s"
@@ -3051,7 +3060,7 @@ msgid "port not found: %[1]s %[2]s"
 msgstr "port not found: %[1]s %[2]s"
 
 #: arduino/discovery/discovery.go:317
-#: arduino/monitor/monitor.go:252
+#: arduino/monitor/monitor.go:258
 msgid "protocol version not supported: requested 1, got %d"
 msgstr "protocol version not supported: requested 1, got %d"
 
@@ -3241,7 +3250,7 @@ msgstr "the server responded with status %s"
 msgid "timeout waiting for message from %s"
 msgstr "timeout waiting for message from %s"
 
-#: arduino/monitor/monitor.go:177
+#: arduino/monitor/monitor.go:161
 msgid "timeout waiting for message from monitor %s"
 msgstr "timeout waiting for message from monitor %s"