[skip-changelog] [breaking] Refactoring of board autodetection (#1717)

* Changes in errors.MultipleBoardsDetectedError and errors.NoBoardsDetectedError

- The message formatting has been fixed
- The errors now accepts a *rpc.Port instead of a *discovery.Port
- The new error NoBoardsDetectedError has been added

* Renamed some variable in compile/compile.go to improve readability

fqbn -> fqbnArg
port -> portArgs
detectedFqbn -> fqbn
discoveryPort -> port

* Renamed some variable in debug/debug.go to improve readability

fqbn -> fqbnArg
port -> portArgs
discoveryPort -> port

* Fixed some error formatting

* Renamed some variable in compile/compile.go to improve readability

fqbn -> fqbnArg
port -> portArgs
discoveryPort -> port

* Refactored board autodetection in upload/compile

- use `board.List()` instead of implementing a duplicate of it
- factored the logic that calculates FQBN, Port and possibly
  autodetects FQBN, so we have a single implementation for all
  commands of the cli.

* Removed SupportedUserFieldsRequest.Address gRPC parameter

Because it's no more needed

* Added notes about technical debt

* Factored --discovery-timeout flag and added timeout to board autodetection

* Added docs

* Fixed error message and integration tests

* Update cli/arguments/port.go

Co-authored-by: Umberto Baldi <34278123+umbynos@users.noreply.github.com>

Co-authored-by: Umberto Baldi <34278123+umbynos@users.noreply.github.com>

Author: cmaglie

arduino/errors.go

@@ -19,7 +19,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/arduino/arduino-cli/arduino/discovery"
 	"github.com/arduino/arduino-cli/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"google.golang.org/grpc/codes"
@@ -125,16 +124,35 @@ func (e *InvalidVersionError) Unwrap() error {
 	return e.Cause
 }
 
+// NoBoardsDetectedError is returned when detecting the FQBN of a board
+// does not produce any result.
+type NoBoardsDetectedError struct {
+	Port *rpc.Port
+}
+
+func (e *NoBoardsDetectedError) Error() string {
+	return tr(
+		"Please specify an FQBN. The board on port %[1]s with protocol %[2]s can't be identified",
+		e.Port.Address,
+		e.Port.Protocol,
+	)
+}
+
+// ToRPCStatus converts the error into a *status.Status
+func (e *NoBoardsDetectedError) ToRPCStatus() *status.Status {
+	return status.New(codes.InvalidArgument, e.Error())
+}
+
 // MultipleBoardsDetectedError is returned when trying to detect
 // the FQBN of a board connected to a port fails because that
 // are multiple possible boards detected.
 type MultipleBoardsDetectedError struct {
-	Port *discovery.Port
+	Port *rpc.Port
 }
 
 func (e *MultipleBoardsDetectedError) Error() string {
 	return tr(
-		"Please specify an FQBN. Multiple possible ports detected on port %s with protocol %s",
+		"Please specify an FQBN. Multiple possible boards detected on port %[1]s with protocol %[2]s",
 		e.Port.Address,
 		e.Port.Protocol,
 	)

cli/arguments/discovery_timeout.go

@@ -0,0 +1,37 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2022 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 arguments
+
+import (
+	"time"
+
+	"github.com/spf13/cobra"
+)
+
+// DiscoveryTimeout is the timeout given to discoveries to detect ports.
+type DiscoveryTimeout struct {
+	timeout time.Duration
+}
+
+// AddToCommand adds the flags used to set fqbn to the specified Command
+func (d *DiscoveryTimeout) AddToCommand(cmd *cobra.Command) {
+	cmd.Flags().DurationVar(&d.timeout, "discovery-timeout", time.Second, tr("Max time to wait for port discovery, e.g.: 30s, 1m"))
+}
+
+// Get returns the timeout
+func (d *DiscoveryTimeout) Get() time.Duration {
+	return d.timeout
+}

cli/arguments/fqbn.go

@@ -16,8 +16,14 @@
 package arguments
 
 import (
+	"os"
 	"strings"
 
+	"github.com/arduino/arduino-cli/arduino"
+	"github.com/arduino/arduino-cli/arduino/sketch"
+	"github.com/arduino/arduino-cli/cli/errorcodes"
+	"github.com/arduino/arduino-cli/cli/feedback"
+	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/spf13/cobra"
 )
 
@@ -54,3 +60,44 @@ func (f *Fqbn) String() string {
 func (f *Fqbn) Set(fqbn string) {
 	f.fqbn = fqbn
 }
+
+// CalculateFQBNAndPort calculate the FQBN and Port metadata based on
+// parameters provided by the user.
+// This determine the FQBN based on:
+// - the value of the FQBN flag if explicitly specified, otherwise
+// - the FQBN value in sketch.json if available, otherwise
+// - it tries to autodetect the board connected to the given port flags
+// If all above methods fails, it returns the empty string.
+// The Port metadata are always returned except if:
+// - the port is not found, in this case nil is returned
+// - the FQBN autodetection fail, in this case the function prints an error and
+//   terminates the execution
+func CalculateFQBNAndPort(portArgs *Port, fqbnArg *Fqbn, instance *rpc.Instance, sk *sketch.Sketch) (string, *rpc.Port) {
+	// TODO: REMOVE sketch.Sketch from here
+
+	fqbn := fqbnArg.String()
+	if fqbn == "" && sk != nil && sk.Metadata != nil {
+		// If the user didn't specify an FQBN and a sketch.json file is present
+		// read it from there.
+		fqbn = sk.Metadata.CPU.Fqbn
+	}
+	if fqbn == "" {
+		if portArgs == nil || portArgs.address == "" {
+			feedback.Error(&arduino.MissingFQBNError{})
+			os.Exit(errorcodes.ErrGeneric)
+		}
+		fqbn, port := portArgs.DetectFQBN(instance)
+		if fqbn == "" {
+			feedback.Error(&arduino.MissingFQBNError{})
+			os.Exit(errorcodes.ErrGeneric)
+		}
+		return fqbn, port
+	}
+
+	port, err := portArgs.GetPort(instance, sk)
+	if err != nil {
+		feedback.Errorf(tr("Error getting port metadata: %v", err))
+		os.Exit(errorcodes.ErrGeneric)
+	}
+	return fqbn, port.ToRPC()
+}

cli/arguments/port.go

@@ -21,11 +21,13 @@ import (
 	"os"
 	"time"
 
+	"github.com/arduino/arduino-cli/arduino"
 	"github.com/arduino/arduino-cli/arduino/discovery"
 	"github.com/arduino/arduino-cli/arduino/sketch"
 	"github.com/arduino/arduino-cli/cli/errorcodes"
 	"github.com/arduino/arduino-cli/cli/feedback"
 	"github.com/arduino/arduino-cli/commands"
+	"github.com/arduino/arduino-cli/commands/board"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -38,7 +40,7 @@ import (
 type Port struct {
 	address  string
 	protocol string
-	timeout  time.Duration
+	timeout  DiscoveryTimeout
 }
 
 // AddToCommand adds the flags used to set port and protocol to the specified Command
@@ -51,7 +53,7 @@ func (p *Port) AddToCommand(cmd *cobra.Command) {
 	cmd.RegisterFlagCompletionFunc("protocol", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
 		return GetInstalledProtocols(), cobra.ShellCompDirectiveDefault
 	})
-	cmd.Flags().DurationVar(&p.timeout, "discovery-timeout", 5*time.Second, tr("Max time to wait for port discovery, e.g.: 30s, 1m"))
+	p.timeout.AddToCommand(cmd)
 }
 
 // GetPortAddressAndProtocol returns only the port address and the port protocol
@@ -72,6 +74,9 @@ func (p *Port) GetPortAddressAndProtocol(instance *rpc.Instance, sk *sketch.Sket
 // 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) {
+	// TODO: REMOVE sketch.Sketch from here
+	// TODO: REMOVE discovery from here (use board.List instead)
+
 	address := p.address
 	protocol := p.protocol
 
@@ -122,7 +127,7 @@ func (p *Port) GetPort(instance *rpc.Instance, sk *sketch.Sketch) (*discovery.Po
 		}
 	}()
 
-	deadline := time.After(p.timeout)
+	deadline := time.After(p.timeout.Get())
 	for {
 		select {
 		case portEvent := <-eventChan:
@@ -149,15 +154,38 @@ func (p *Port) GetPort(instance *rpc.Instance, sk *sketch.Sketch) (*discovery.Po
 
 // GetSearchTimeout returns the timeout
 func (p *Port) GetSearchTimeout() time.Duration {
-	return p.timeout
+	return p.timeout.Get()
 }
 
-// GetDiscoveryPort is a helper function useful to get the port and handle possible errors
-func (p *Port) GetDiscoveryPort(instance *rpc.Instance, sk *sketch.Sketch) *discovery.Port {
-	discoveryPort, err := p.GetPort(instance, sk)
+// DetectFQBN tries to identify the board connected to the port and returns the
+// discovered Port object together with the FQBN. If the port does not match
+// exactly 1 board,
+func (p *Port) DetectFQBN(inst *rpc.Instance) (string, *rpc.Port) {
+	detectedPorts, err := board.List(&rpc.BoardListRequest{
+		Instance: inst,
+		Timeout:  p.timeout.Get().Milliseconds(),
+	})
 	if err != nil {
-		feedback.Errorf(tr("Error discovering port: %v"), err)
+		feedback.Errorf(tr("Error during FQBN detection: %v", err))
 		os.Exit(errorcodes.ErrGeneric)
 	}
-	return discoveryPort
+	for _, detectedPort := range detectedPorts {
+		port := detectedPort.GetPort()
+		if p.address != port.GetAddress() {
+			continue
+		}
+		if p.protocol != "" && p.protocol != port.GetProtocol() {
+			continue
+		}
+		if len(detectedPort.MatchingBoards) > 1 {
+			feedback.Error(&arduino.MultipleBoardsDetectedError{Port: port})
+			os.Exit(errorcodes.ErrBadArgument)
+		}
+		if len(detectedPort.MatchingBoards) == 0 {
+			feedback.Error(&arduino.NoBoardsDetectedError{Port: port})
+			os.Exit(errorcodes.ErrBadArgument)
+		}
+		return detectedPort.MatchingBoards[0].Fqbn, port
+	}
+	return "", nil
 }

cli/arguments/sketch.go

@@ -48,7 +48,7 @@ func InitSketchPath(path string) (sketchPath *paths.Path) {
 func NewSketch(sketchPath *paths.Path) *sketch.Sketch {
 	sketch, err := sketch.New(sketchPath)
 	if err != nil {
-		feedback.Errorf(tr("Error creating sketch: %v"), err)
+		feedback.Errorf(tr("Error opening sketch: %v"), err)
 		os.Exit(errorcodes.ErrGeneric)
 	}
 	return sketch

cli/board/list.go

@@ -19,9 +19,9 @@ import (
 	"fmt"
 	"os"
 	"sort"
-	"time"
 
 	"github.com/arduino/arduino-cli/arduino/cores"
+	"github.com/arduino/arduino-cli/cli/arguments"
 	"github.com/arduino/arduino-cli/cli/errorcodes"
 	"github.com/arduino/arduino-cli/cli/feedback"
 	"github.com/arduino/arduino-cli/cli/instance"
@@ -33,21 +33,21 @@ import (
 )
 
 var (
-	timeout time.Duration
-	watch   bool
+	timeoutArg arguments.DiscoveryTimeout
+	watch      bool
 )
 
 func initListCommand() *cobra.Command {
 	listCommand := &cobra.Command{
 		Use:     "list",
 		Short:   tr("List connected boards."),
 		Long:    tr("Detects and displays a list of boards connected to the current computer."),
-		Example: "  " + os.Args[0] + " board list --timeout 10s",
+		Example: "  " + os.Args[0] + " board list --discovery-timeout 10s",
 		Args:    cobra.NoArgs,
 		Run:     runListCommand,
 	}
 
-	listCommand.Flags().DurationVar(&timeout, "discovery-timeout", time.Second, tr("Max time to wait for port discovery, e.g.: 30s, 1m"))
+	timeoutArg.AddToCommand(listCommand)
 	listCommand.Flags().BoolVarP(&watch, "watch", "w", false, tr("Command keeps running and prints list of connected boards whenever there is a change."))
 
 	return listCommand
@@ -66,7 +66,7 @@ func runListCommand(cmd *cobra.Command, args []string) {
 
 	ports, err := board.List(&rpc.BoardListRequest{
 		Instance: inst,
-		Timeout:  timeout.Milliseconds(),
+		Timeout:  timeoutArg.Get().Milliseconds(),
 	})
 	if err != nil {
 		feedback.Errorf(tr("Error detecting boards: %v"), err)