feature: Detect board port change after upload (#2253)

* UploadResponse now has 'oneof' clause for better API design

* Added scaffolding to return updated-port after upload

* Upload port change detection (first draft)

* Simplified port detection using a Future-style abstraction

* Perform watcher-flush higher in the call tree

* Do not infer upload port if 'upload.wait_for_upload_port' is false

* Further simplified port detection subroutine structure

* fixed linter issue

* Always return an updatedUploadPort.

Arduino CLI should always return the port after an upload, even in the case
where no port change is expected. The consumer shouldn't be required to
implement "if not updated_upload_port, use original port" logic.

The whole point is that all the logic for determining which port should be
selected after an upload should be implemented in Arduino CLI. The consumer
should be able to simply select the port Arduino CLI tells it to select in
all cases.

* Updated docs

* Perform a deep-copy of upload ports where needed.

Previously only the pointer was copied, thus making changes in
`actualPort` to be reflected also to `port`. This lead to some weird
result in the `updatedUploadPort` result:

{
  "stdout": "Verify 11344 bytes of flash with checksum.\nVerify successful\ndone in 0.010 seconds\nCPU reset.\n",
  "stderr": "",
  "updated_upload_port": {
    "address": "/dev/tty.usbmodem14101",     <------- this address...
    "label": "/dev/cu.usbmodem14101",        <------- ...is different from the label
    "protocol": "serial",
    "protocol_label": "Serial Port (USB)",
    "properties": {
      "pid": "0x804E",
      "serialNumber": "94A3397C5150435437202020FF150838",
      "vid": "0x2341"
    },
    "hardware_id": "94A3397C5150435437202020FF150838"
  }
}

* When updating `actualPort` address, update also the address label.

* Fixed some potential nil pointer exceptions

* Further simplified board watcher

We must acesss the gRPC API only until we cross the `command` package
border. Once we are inside the `command` package we should use the
internal API only.

* Before returning from upload, check if the port is still alive

Now the upload detects cases when the upload port is "unstable", i.e.
the port changes even if it shouldn't (because the wait_for_upload_port
property in boards.txt is set to false).

This change should make the upload process more resilient.

* Apply suggestions from code review
Co-authored-by: per1234 <accounts@perglass.com>

* Fixed nil exception

* Improved tracking algorithm for upload-port reconnection

The new algorithm takes into account the case where a single board may
expose multiple ports, in this case the selection will increase priority
to ports that:

  1. have the same HW id as the user specified port for upload
  2. have the same protocol as the user specified port for upload
  3. have the same address as the user specified port for upload

---------
Co-authored-by: per1234 <accounts@perglass.com>

arduino/discovery/discovery.go

@@ -97,6 +97,12 @@ type Port struct {
 
 var tr = i18n.Tr
 
+// Equals returns true if the given port has the same address and protocol
+// of the current port.
+func (p *Port) Equals(o *Port) bool {
+	return p.Address == o.Address && p.Protocol == o.Protocol
+}
+
 // ToRPC converts Port into rpc.Port
 func (p *Port) ToRPC() *rpc.Port {
 	props := p.Properties
@@ -113,6 +119,24 @@ func (p *Port) ToRPC() *rpc.Port {
 	}
 }
 
+// PortFromRPCPort converts an *rpc.Port to a *Port
+func PortFromRPCPort(o *rpc.Port) (p *Port) {
+	if o == nil {
+		return nil
+	}
+	res := &Port{
+		Address:       o.Address,
+		AddressLabel:  o.Label,
+		Protocol:      o.Protocol,
+		ProtocolLabel: o.ProtocolLabel,
+		HardwareID:    o.HardwareId,
+	}
+	if o.Properties != nil {
+		res.Properties = properties.NewFromHashmap(o.Properties)
+	}
+	return res
+}
+
 func (p *Port) String() string {
 	if p == nil {
 		return "none"
@@ -120,6 +144,18 @@ func (p *Port) String() string {
 	return p.Address
 }
 
+// Clone creates a copy of this Port
+func (p *Port) Clone() *Port {
+	if p == nil {
+		return nil
+	}
+	var res Port = *p
+	if p.Properties != nil {
+		res.Properties = p.Properties.Clone()
+	}
+	return &res
+}
+
 // Event is a pluggable discovery event
 type Event struct {
 	Type        string

arduino/discovery/discovery_client/main.go

@@ -51,10 +51,12 @@ func main() {
 		fmt.Printf("   Address: %s\n", port.Address)
 		fmt.Printf("  Protocol: %s\n", port.Protocol)
 		if ev.Type == "add" {
-			keys := port.Properties.Keys()
-			sort.Strings(keys)
-			for _, k := range keys {
-				fmt.Printf("            %s=%s\n", k, port.Properties.Get(k))
+			if port.Properties != nil {
+				keys := port.Properties.Keys()
+				sort.Strings(keys)
+				for _, k := range keys {
+					fmt.Printf("            %s=%s\n", k, port.Properties.Get(k))
+				}
 			}
 		}
 		fmt.Println()

commands/board/list.go

@@ -34,6 +34,7 @@ import (
 	"github.com/arduino/arduino-cli/commands"
 	"github.com/arduino/arduino-cli/internal/inventory"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
+	"github.com/arduino/go-properties-orderedmap"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -128,20 +129,22 @@ func apiByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) {
 	}, nil
 }
 
-func identifyViaCloudAPI(port *discovery.Port) ([]*rpc.BoardListItem, error) {
+func identifyViaCloudAPI(props *properties.Map) ([]*rpc.BoardListItem, error) {
 	// If the port is not USB do not try identification via cloud
-	id := port.Properties
-	if !id.ContainsKey("vid") || !id.ContainsKey("pid") {
+	if !props.ContainsKey("vid") || !props.ContainsKey("pid") {
 		return nil, nil
 	}
 
 	logrus.Debug("Querying builder API for board identification...")
-	return cachedAPIByVidPid(id.Get("vid"), id.Get("pid"))
+	return cachedAPIByVidPid(props.Get("vid"), props.Get("pid"))
 }
 
 // identify returns a list of boards checking first the installed platforms or the Cloud API
 func identify(pme *packagemanager.Explorer, port *discovery.Port) ([]*rpc.BoardListItem, error) {
 	boards := []*rpc.BoardListItem{}
+	if port.Properties == nil {
+		return boards, nil
+	}
 
 	// first query installed cores through the Package Manager
 	logrus.Debug("Querying installed cores for board identification...")
@@ -167,7 +170,7 @@ func identify(pme *packagemanager.Explorer, port *discovery.Port) ([]*rpc.BoardL
 	// if installed cores didn't recognize the board, try querying
 	// the builder API if the board is a USB device port
 	if len(boards) == 0 {
-		items, err := identifyViaCloudAPI(port)
+		items, err := identifyViaCloudAPI(port.Properties)
 		if err != nil {
 			// this is bad, but keep going
 			logrus.WithError(err).Debug("Error querying builder API")

commands/board/list_test.go

@@ -103,10 +103,7 @@ func TestGetByVidPidMalformedResponse(t *testing.T) {
 }
 
 func TestBoardDetectionViaAPIWithNonUSBPort(t *testing.T) {
-	port := &discovery.Port{
-		Properties: properties.NewMap(),
-	}
-	items, err := identifyViaCloudAPI(port)
+	items, err := identifyViaCloudAPI(properties.NewMap())
 	require.NoError(t, err)
 	require.Empty(t, items)
 }

commands/daemon/daemon.go

@@ -297,15 +297,27 @@ func (s *ArduinoCoreServerImpl) PlatformList(ctx context.Context, req *rpc.Platf
 // Upload FIXMEDOC
 func (s *ArduinoCoreServerImpl) Upload(req *rpc.UploadRequest, stream rpc.ArduinoCoreService_UploadServer) error {
 	syncSend := NewSynchronizedSend(stream.Send)
-	outStream := feedStreamTo(func(data []byte) { syncSend.Send(&rpc.UploadResponse{OutStream: data}) })
-	errStream := feedStreamTo(func(data []byte) { syncSend.Send(&rpc.UploadResponse{ErrStream: data}) })
-	err := upload.Upload(stream.Context(), req, outStream, errStream)
+	outStream := feedStreamTo(func(data []byte) {
+		syncSend.Send(&rpc.UploadResponse{
+			Message: &rpc.UploadResponse_OutStream{OutStream: data},
+		})
+	})
+	errStream := feedStreamTo(func(data []byte) {
+		syncSend.Send(&rpc.UploadResponse{
+			Message: &rpc.UploadResponse_ErrStream{ErrStream: data},
+		})
+	})
+	res, err := upload.Upload(stream.Context(), req, outStream, errStream)
 	outStream.Close()
 	errStream.Close()
-	if err != nil {
-		return convertErrorToRPCStatus(err)
+	if res != nil {
+		syncSend.Send(&rpc.UploadResponse{
+			Message: &rpc.UploadResponse_Result{
+				Result: res,
+			},
+		})
 	}
-	return nil
+	return convertErrorToRPCStatus(err)
 }
 
 // UploadUsingProgrammer FIXMEDOC

commands/upload/burnbootloader.go

@@ -39,7 +39,7 @@ func BurnBootloader(ctx context.Context, req *rpc.BurnBootloaderRequest, outStre
 	}
 	defer release()
 
-	err := runProgramAction(
+	_, err := runProgramAction(
 		pme,
 		nil, // sketch
 		"",  // importFile

commands/upload/upload_test.go

@@ -184,7 +184,7 @@ func TestUploadPropertiesComposition(t *testing.T) {
 	testRunner := func(t *testing.T, test test, verboseVerify bool) {
 		outStream := &bytes.Buffer{}
 		errStream := &bytes.Buffer{}
-		err := runProgramAction(
+		_, err := runProgramAction(
 			pme,
 			nil,                     // sketch
 			"",                      // importFile

docs/UPGRADING.md

@@ -4,6 +4,58 @@ Here you can find a list of migration guides to handle breaking changes between
 
 ## 0.34.0
 
+### The gRPC `cc.arduino.cli.commands.v1.UploadRepsonse` command response has been changed.
+
+Previously the `UploadResponse` was used only to stream the tool output:
+
+```
+message UploadResponse {
+  // The output of the upload process.
+  bytes out_stream = 1;
+  // The error output of the upload process.
+  bytes err_stream = 2;
+}
+```
+
+Now the API logic has been clarified using the `oneof` clause and another field has been added providing an
+`UploadResult` message that is sent when a successful upload completes.
+
+```
+message UploadResponse {
+  oneof message {
+    // The output of the upload process.
+    bytes out_stream = 1;
+    // The error output of the upload process.
+    bytes err_stream = 2;
+    // The upload result
+    UploadResult result = 3;
+  }
+}
+
+message UploadResult {
+  // When a board requires a port disconnection to perform the upload, this
+  // field returns the port where the board reconnects after the upload.
+  Port updated_upload_port = 1;
+}
+```
+
+### golang API: method `github.com/arduino/arduino-cli/commands/upload.Upload` changed signature
+
+The `Upload` method signature has been changed from:
+
+```go
+func Upload(ctx context.Context, req *rpc.UploadRequest, outStream io.Writer, errStream io.Writer) error { ... }
+```
+
+to:
+
+```go
+func Upload(ctx context.Context, req *rpc.UploadRequest, outStream io.Writer, errStream io.Writer) (*rpc.UploadResult, error) { ... }
+```
+
+Now an `UploadResult` structure is returned together with the error. If you are not interested in the information
+contained in the structure you can safely ignore it.
+
 ### golang package `github.com/arduino/arduino-cli/inventory` removed from public API
 
 The package `inventory` is no more a public golang API.

internal/algorithms/channels.go

+// This file is part of arduino-cli.
+//
+// Copyright 2023 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package f
+
+import "sync"
+
+// DiscardCh consumes all incoming messages from the given channel until it's closed.
+func DiscardCh[T any](ch <-chan T) {
+	for range ch {
+	}
+}
+
+// Future is an object that holds a result value. The value may be read and
+// written asynchronously.
+type Future[T any] interface {
+	Send(T)
+	Await() T
+}
+
+type future[T any] struct {
+	wg    sync.WaitGroup
+	value T
+}
+
+// NewFuture creates a new Future[T]
+func NewFuture[T any]() Future[T] {
+	res := &future[T]{}
+	res.wg.Add(1)
+	return res
+}
+
+// Send a result in the Future. Threads waiting for result will be unlocked.
+func (f *future[T]) Send(value T) {
+	f.value = value
+	f.wg.Done()
+}
+
+// Await for a result from the Future, blocks until a result is available.
+func (f *future[T]) Await() T {
+	f.wg.Wait()
+	return f.value
+}

internal/cli/compile/compile.go

@@ -236,6 +236,8 @@ func runCompileCommand(cmd *cobra.Command, args []string) {
 		DoNotExpandBuildProperties:    showProperties == arguments.ShowPropertiesUnexpanded,
 	}
 	compileRes, compileError := compile.Compile(context.Background(), compileRequest, stdOut, stdErr, nil)
+
+	var uploadRes *rpc.UploadResult
 	if compileError == nil && uploadAfterCompile {
 		userFieldRes, err := upload.SupportedUserFields(context.Background(), &rpc.SupportedUserFieldsRequest{
 			Instance: inst,
@@ -268,8 +270,10 @@ func runCompileCommand(cmd *cobra.Command, args []string) {
 			UserFields: fields,
 		}
 
-		if err := upload.Upload(context.Background(), uploadRequest, stdOut, stdErr); err != nil {
+		if res, err := upload.Upload(context.Background(), uploadRequest, stdOut, stdErr); err != nil {
 			feedback.Fatal(tr("Error during Upload: %v", err), feedback.ErrGeneric)
+		} else {
+			uploadRes = res
 		}
 	}
 
@@ -330,6 +334,7 @@ func runCompileCommand(cmd *cobra.Command, args []string) {
 		CompilerOut:        stdIO.Stdout,
 		CompilerErr:        stdIO.Stderr,
 		BuilderResult:      compileRes,
+		UploadResult:       uploadRes,
 		ProfileOut:         profileOut,
 		Success:            compileError == nil,
 		showPropertiesMode: showProperties,
@@ -375,6 +380,7 @@ type compileResult struct {
 	CompilerOut   string               `json:"compiler_out"`
 	CompilerErr   string               `json:"compiler_err"`
 	BuilderResult *rpc.CompileResponse `json:"builder_result"`
+	UploadResult  *rpc.UploadResult    `json:"upload_result"`
 	Success       bool                 `json:"success"`
 	ProfileOut    string               `json:"profile_out,omitempty"`
 	Error         string               `json:"error,omitempty"`

internal/cli/upload/upload.go

@@ -168,8 +168,31 @@ func runUploadCommand(command *cobra.Command, args []string) {
 		DryRun:     dryRun,
 		UserFields: fields,
 	}
-	if err := upload.Upload(context.Background(), req, stdOut, stdErr); err != nil {
+	if res, err := upload.Upload(context.Background(), req, stdOut, stdErr); err != nil {
 		feedback.FatalError(err, feedback.ErrGeneric)
+	} else {
+		io := stdIOResult()
+		feedback.PrintResult(&uploadResult{
+			Stdout:            io.Stdout,
+			Stderr:            io.Stderr,
+			UpdatedUploadPort: res.UpdatedUploadPort,
+		})
 	}
-	feedback.PrintResult(stdIOResult())
+}
+
+type uploadResult struct {
+	Stdout            string    `json:"stdout"`
+	Stderr            string    `json:"stderr"`
+	UpdatedUploadPort *rpc.Port `json:"updated_upload_port,omitempty"`
+}
+
+func (r *uploadResult) Data() interface{} {
+	return r
+}
+
+func (r *uploadResult) String() string {
+	if r.UpdatedUploadPort == nil {
+		return ""
+	}
+	return tr("New upload port: %[1]s (%[2]s)", r.UpdatedUploadPort.Address, r.UpdatedUploadPort.Protocol)
 }

rpc/cc/arduino/cli/commands/v1/upload.proto

@@ -62,10 +62,20 @@ message UploadRequest {
 }
 
 message UploadResponse {
-  // The output of the upload process.
-  bytes out_stream = 1;
-  // The error output of the upload process.
-  bytes err_stream = 2;
+  oneof message {
+    // The output of the upload process.
+    bytes out_stream = 1;
+    // The error output of the upload process.
+    bytes err_stream = 2;
+    // The upload result
+    UploadResult result = 3;
+  }
+}
+
+message UploadResult {
+  // When a board requires a port disconnection to perform the upload, this
+  // field returns the port where the board reconnects after the upload.
+  Port updated_upload_port = 1;
 }
 
 message ProgrammerIsRequiredForUploadError {}