Refactored board details to use pluggable discovery (#1332)

* Cache board identification properties

* Board details now shows pluggable discovery identification props

* Use new methods in go-properties-orderedmap to calculate subsets

* Updated i18n

* Added migration docs

* Improved tests

* Update arduino/cores/board.go

Co-authored-by: per1234 <[email protected]>

* Update docs/UPGRADING.md

Co-authored-by: per1234 <[email protected]>

Author: cmaglie

arduino/cores/board.go

@@ -24,9 +24,10 @@ import (
 
 // Board represents a board loaded from an installed platform
 type Board struct {
-	BoardID         string
-	Properties      *properties.Map  `json:"-"`
-	PlatformRelease *PlatformRelease `json:"-"`
+	BoardID                  string
+	Properties               *properties.Map  `json:"-"`
+	PlatformRelease          *PlatformRelease `json:"-"`
+	identificationProperties []*properties.Map
 }
 
 // HasUsbID returns true if the board match the usb vid and pid parameters
@@ -140,14 +141,19 @@ func (b *Board) GeneratePropertiesForConfiguration(config string) (*properties.M
 	return b.GetBuildProperties(fqbn.Configs)
 }
 
+// GetIdentificationProperties calculates and returns a list of properties sets
+// containing the properties required to identify the board. The returned sets
+// must not be changed by the caller.
+func (b *Board) GetIdentificationProperties() []*properties.Map {
+	if b.identificationProperties == nil {
+		b.identificationProperties = b.Properties.ExtractSubIndexSets("upload_port")
+	}
+	return b.identificationProperties
+}
+
 // IsBoardMatchingIDProperties returns true if the board match the given
 // identification properties
 func (b *Board) IsBoardMatchingIDProperties(query *properties.Map) bool {
-	portIDPropsSet := b.Properties.SubTree("upload_port")
-	if portIDPropsSet.Size() == 0 {
-		return false
-	}
-
 	// check checks if the given set of properties p match the "query"
 	check := func(p *properties.Map) bool {
 		for k, v := range p.AsMap() {
@@ -159,26 +165,10 @@ func (b *Board) IsBoardMatchingIDProperties(query *properties.Map) bool {
 	}
 
 	// First check the identification properties with sub index "upload_port.N.xxx"
-	idx := 0
-	haveIndexedProperties := false
-	for {
-		idProps := portIDPropsSet.SubTree(fmt.Sprintf("%d", idx))
-		idx++
-		if idProps.Size() > 0 {
-			haveIndexedProperties = true
-			if check(idProps) {
-				return true
-			}
-		} else if idx > 1 {
-			// Always check sub-id 0 and 1 (https://github.com/arduino/arduino-cli/issues/456)
-			break
+	for _, idProps := range b.GetIdentificationProperties() {
+		if check(idProps) {
+			return true
 		}
 	}
-
-	// if there are no subindexed then check the whole "upload_port.xxx"
-	if !haveIndexedProperties {
-		return check(portIDPropsSet)
-	}
-
 	return false
 }

cli/board/details.go

@@ -130,13 +130,13 @@ func (dr detailsResult) String() string {
 			table.NewCell("✔", color.New(color.FgGreen)))
 	}
 
-	for i, idp := range details.IdentificationPrefs {
-		if i == 0 {
-			t.AddRow() // get some space from above
-			t.AddRow(tr("Identification properties:"), "VID:"+idp.UsbId.Vid+" PID:"+idp.UsbId.Pid)
-			continue
+	for _, idp := range details.GetIdentificationProperties() {
+		t.AddRow() // get some space from above
+		header := tr("Identification properties:")
+		for k, v := range idp.GetProperties() {
+			t.AddRow(header, k+"="+v)
+			header = ""
 		}
-		t.AddRow("", "VID:"+idp.UsbId.Vid+" PID:"+idp.UsbId.Pid)
 	}
 
 	t.AddRow() // get some space from above
@@ -159,14 +159,14 @@ func (dr detailsResult) String() string {
 
 	t.AddRow() // get some space from above
 	for _, tool := range details.ToolsDependencies {
-		t.AddRow(tr("Required tool:"), tool.Packager+":"+tool.Name, "", tool.Version)
+		t.AddRow(tr("Required tool:"), tool.Packager+":"+tool.Name, tool.Version)
 		if showFullDetails {
 			for _, sys := range tool.Systems {
-				t.AddRow("", tr("OS:"), "", sys.Host)
-				t.AddRow("", tr("File:"), "", sys.ArchiveFilename)
-				t.AddRow("", tr("Size (bytes):"), "", fmt.Sprint(sys.Size))
-				t.AddRow("", tr("Checksum:"), "", sys.Checksum)
-				t.AddRow("", "URL:", "", sys.Url)
+				t.AddRow("", tr("OS:"), sys.Host)
+				t.AddRow("", tr("File:"), sys.ArchiveFilename)
+				t.AddRow("", tr("Size (bytes):"), fmt.Sprint(sys.Size))
+				t.AddRow("", tr("Checksum:"), sys.Checksum)
+				t.AddRow("", "URL:", sys.Url)
 				t.AddRow() // get some space from above
 			}
 		}

commands/board/details.go

@@ -50,6 +50,12 @@ func Details(ctx context.Context, req *rpc.BoardDetailsRequest) (*rpc.BoardDetai
 	details.PropertiesId = board.BoardID
 	details.Official = fqbn.Package == "arduino"
 	details.Version = board.PlatformRelease.Version.String()
+	details.IdentificationProperties = []*rpc.BoardIdentificationProperties{}
+	for _, p := range board.GetIdentificationProperties() {
+		details.IdentificationProperties = append(details.IdentificationProperties, &rpc.BoardIdentificationProperties{
+			Properties: p.AsMap(),
+		})
+	}
 
 	details.DebuggingSupported = boardProperties.ContainsKey("debug.executable") ||
 		boardPlatform.Properties.ContainsKey("debug.executable") ||
@@ -80,16 +86,6 @@ func Details(ctx context.Context, req *rpc.BoardDetailsRequest) (*rpc.BoardDetai
 		details.Platform.Size = boardPlatform.Resource.Size
 	}
 
-	details.IdentificationPrefs = []*rpc.IdentificationPref{}
-	vids := board.Properties.SubTree("vid")
-	pids := board.Properties.SubTree("pid")
-	for id, vid := range vids.AsMap() {
-		if pid, ok := pids.GetOk(id); ok {
-			idPref := rpc.IdentificationPref{UsbId: &rpc.USBID{Vid: vid, Pid: pid}}
-			details.IdentificationPrefs = append(details.IdentificationPrefs, &idPref)
-		}
-	}
-
 	details.ConfigOptions = []*rpc.ConfigOption{}
 	options := board.GetConfigOptions()
 	for _, option := range options.Keys() {

docs/UPGRADING.md

@@ -93,6 +93,112 @@ removed, in its place:
 
 `Context.Sketch` types has been changed from `Sketch` to `sketch.Sketch`.
 
+### Change in `board details` response (gRPC and JSON output)
+
+The `board details` output WRT board identification properties has changed, before it was:
+
+```
+$ arduino-cli board details arduino:samd:mkr1000
+Board name:                Arduino MKR1000
+FQBN:                      arduino:samd:mkr1000
+Board version:             1.8.11
+Debugging supported:       ✔
+
+Official Arduino board:    ✔
+
+Identification properties: VID:0x2341 PID:0x824e
+                           VID:0x2341 PID:0x024e
+                           VID:0x2341 PID:0x804e
+                           VID:0x2341 PID:0x004e
+[...]
+
+$ arduino-cli board details arduino:samd:mkr1000 --format json
+[...]
+  "identification_prefs": [
+    {
+      "usb_id": {
+        "vid": "0x2341",
+        "pid": "0x804e"
+      }
+    },
+    {
+      "usb_id": {
+        "vid": "0x2341",
+        "pid": "0x004e"
+      }
+    },
+    {
+      "usb_id": {
+        "vid": "0x2341",
+        "pid": "0x824e"
+      }
+    },
+    {
+      "usb_id": {
+        "vid": "0x2341",
+        "pid": "0x024e"
+      }
+    }
+  ],
+[...]
+```
+
+now the properties have been renamed from `identification_prefs` to `identification_properties` and they are no longer
+specific to USB but they can theoretically be any set of key/values:
+
+```
+$ arduino-cli board details arduino:samd:mkr1000
+Board name:                Arduino MKR1000
+FQBN:                      arduino:samd:mkr1000
+Board version:             1.8.11
+Debugging supported:       ✔
+
+Official Arduino board:    ✔
+
+Identification properties: vid=0x2341
+                           pid=0x804e
+
+Identification properties: vid=0x2341
+                           pid=0x004e
+
+Identification properties: vid=0x2341
+                           pid=0x824e
+
+Identification properties: vid=0x2341
+                           pid=0x024e
+[...]
+
+$ arduino-cli board details arduino:samd:mkr1000 --format json
+[...]
+  "identification_properties": [
+    {
+      "properties": {
+        "pid": "0x804e",
+        "vid": "0x2341"
+      }
+    },
+    {
+      "properties": {
+        "pid": "0x004e",
+        "vid": "0x2341"
+      }
+    },
+    {
+      "properties": {
+        "pid": "0x824e",
+        "vid": "0x2341"
+      }
+    },
+    {
+      "properties": {
+        "pid": "0x024e",
+        "vid": "0x2341"
+      }
+    }
+  ]
+}
+```
+
 ### Change of behaviour of gRPC `Init` function
 
 Previously the `Init` function was used to both create a new `CoreInstance` and initialize it, so that the internal

go.mod

@@ -16,7 +16,6 @@ require (
 	github.com/fluxio/iohelpers v0.0.0-20160419043813-3a4dd67a94d2 // indirect
 	github.com/fluxio/multierror v0.0.0-20160419044231-9c68d39025e5 // indirect
 	github.com/gofrs/uuid v3.2.0+incompatible
-	github.com/golang/protobuf v1.5.2
 	github.com/h2non/filetype v1.0.8 // indirect
 	github.com/juju/loggo v0.0.0-20190526231331-6e530bcce5d8 // indirect
 	github.com/kr/text v0.2.0 // indirect
@@ -38,7 +37,6 @@ require (
 	github.com/spf13/jwalterweatherman v1.0.0
 	github.com/spf13/viper v1.6.2
 	github.com/stretchr/testify v1.6.1
-	github.com/xanzy/ssh-agent v0.2.1 // indirect
 	go.bug.st/cleanup v1.0.0
 	go.bug.st/downloader/v2 v2.1.1
 	go.bug.st/relaxed-semver v0.0.0-20190922224835-391e10178d18
@@ -48,7 +46,7 @@ require (
 	golang.org/x/net v0.0.0-20210505024714-0287a6fb4125
 	golang.org/x/sys v0.0.0-20210503173754-0981d6026fa6 // indirect
 	golang.org/x/text v0.3.6
-	google.golang.org/genproto v0.0.0-20210504143626-3b2ad6ccc450 // indirect
+	google.golang.org/genproto v0.0.0-20210504143626-3b2ad6ccc450
 	google.golang.org/grpc v1.37.0
 	google.golang.org/protobuf v1.26.0
 	gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce // indirect

i18n/data/en.po

@@ -54,7 +54,7 @@ msgstr "Global Flags:"
 msgid "Id"
 msgstr "Id"
 
-#: cli/board/details.go:136
+#: cli/board/details.go:135
 msgid "Identification properties:"
 msgstr "Identification properties:"
 

i18n/rice-box.go

@@ -53,24 +53,17 @@ message BoardDetailsResponse {
   repeated ToolsDependencies tools_dependencies = 10;
   // The board's custom configuration options.
   repeated ConfigOption config_options = 11;
-  // Identifying information for the board (e.g., USB VID/PID).
-  repeated IdentificationPref identification_prefs = 12;
   // List of programmers supported by the board
   repeated Programmer programmers = 13;
   // Set to true if the board supports debugging
   bool debugging_supported = 14;
+  // Identifying information for the board (e.g., USB VID/PID).
+  repeated BoardIdentificationProperties identification_properties = 15;
 }
 
-message IdentificationPref {
-  // Identifying information for USB-connected boards.
-  USBID usb_id = 1;
-}
-
-message USBID {
-  // USB vendor ID.
-  string vid = 1;
-  // USB product ID.
-  string pid = 2;
+message BoardIdentificationProperties {
+  // A set of properties that must all be matched to identify the board
+  map<string, string> properties = 1;
 }
 
 message Package {

rpc/cc/arduino/cli/commands/v1/board.pb.go

@@ -353,15 +353,15 @@
       ]
     }
   ],
-  "identification_prefs": [
+  "identification_properties": [
     {
-      "usb_id": {
+      "properties": {
         "vid": "0x2341",
         "pid": "0x8057"
       }
     },
     {
-      "usb_id": {
+      "properties": {
         "vid": "0x2341",
         "pid": "0x0057"
       }
@@ -483,8 +483,8 @@ def test_board_details(run_command):
     assert result["official"] == gold_board_details["official"]
     assert result["package"] == gold_board_details["package"]
     assert result["platform"] == gold_board_details["platform"]
-    for usb_id in gold_board_details["identification_prefs"]:
-        assert usb_id in result["identification_prefs"]
+    for usb_id in gold_board_details["identification_properties"]:
+        assert usb_id in result["identification_properties"]
     for programmer in gold_board_details["programmers"]:
         assert programmer in result["programmers"]
 
@@ -515,8 +515,8 @@ def test_board_details_old(run_command):
     assert result["official"] == gold_board_details["official"]
     assert result["package"] == gold_board_details["package"]
     assert result["platform"] == gold_board_details["platform"]
-    for usb_id in gold_board_details["identification_prefs"]:
-        assert usb_id in result["identification_prefs"]
+    for usb_id in gold_board_details["identification_properties"]:
+        assert usb_id in result["identification_properties"]
     for programmer in gold_board_details["programmers"]:
         assert programmer in result["programmers"]
 
@@ -537,14 +537,11 @@ def test_board_details_list_programmers_without_flag(run_command):
     run_command("core install arduino:samd@1.8.6")
     result = run_command("board details -b arduino:samd:nano_33_iot")
     assert result.ok
-    lines = [l.strip() for l in result.stdout.splitlines()]
-    assert (
-        "Programmers:               Id                                                                     Name"
-        in lines
-    )
-    assert "edbg                                                                   Atmel EDBG" in lines
-    assert "atmel_ice                                                              Atmel-ICE" in lines
-    assert "sam_ice                                                                Atmel SAM-ICE" in lines
+    lines = [l.strip().split() for l in result.stdout.splitlines()]
+    assert ["Programmers:", "Id", "Name"] in lines
+    assert ["edbg", "Atmel", "EDBG"] in lines
+    assert ["atmel_ice", "Atmel-ICE"] in lines
+    assert ["sam_ice", "Atmel", "SAM-ICE"] in lines
 
 
 def test_board_details_list_programmers_flag(run_command):