Refactored upload procedure to correctly handle bootloader.tool property

commands/upload/testdata/hardware/alice/avr/boards.txt

@@ -0,0 +1,23 @@
+board1.name=board1
+board1.conf.board=conf-board1
+board1.upload.tool=one
+board1.upload.protocol=protocol
+board1.upload.speed=speed
+board1.bootloader.tool=one
+board1.bootloader.fuses=0xFF
+board1.bootloader.file=niceboot/niceboot.hex
+
+
+board2.name=board2
+board2.conf.board=conf-board1
+board2.upload.tool=one-noport
+board2.upload.protocol=protocol
+board2.upload.speed=speed
+
+board2.bootloader.tool=one
+board2.bootloader.low_fuses=0xFF
+board2.bootloader.high_fuses=0xDE
+board2.bootloader.extended_fuses=0xFD
+board2.bootloader.unlock_bits=0x3F
+board2.bootloader.lock_bits=0x0F
+board2.bootloader.file=optiboot/optiboot_atmega328.hex

commands/upload/testdata/hardware/alice/avr/platform.txt

@@ -0,0 +1,79 @@
+name=Upload Test Platform (Alice)
+version=1.0.0
+
+# Upload test 1
+tools.one.cmd.path=echo
+tools.one.conf.general=conf-general
+tools.one.upload.conf=conf-upload
+tools.one.upload.params.verbose=verbose
+tools.one.upload.params.quiet=quiet
+tools.one.upload.params.verify=verify
+tools.one.upload.params.noverify=noverify
+tools.one.upload.pattern={cmd.path} {conf.board} {conf.general} {upload.conf} {upload.verbose} {upload.verify} {upload.protocol} "{serial.port}" -b{upload.speed} "{build.path}/{build.project_name}.hex"
+
+tools.one.program.conf=conf-program
+tools.one.program.params.verbose=verbose
+tools.one.program.params.quiet=quiet
+tools.one.program.params.verify=verify
+tools.one.program.params.noverify=noverify
+tools.one.program.pattern={cmd.path} {conf.board} {conf.general} {program.conf} {program.verbose} {program.verify} {program.protocol} "{serial.port}" -b{upload.speed} "{build.path}/{build.project_name}.hex"
+
+tools.one.erase.conf=conf-erase
+tools.one.erase.params.verbose=verbose
+tools.one.erase.params.quiet=quiet
+tools.one.erase.params.verify=verify
+tools.one.erase.params.noverify=noverify
+tools.one.erase.pattern={cmd.path} ERASE {conf.board} {conf.general} {erase.conf} {erase.verbose} {erase.verify} {protocol} "{serial.port}" -b{upload.speed}
+
+tools.one.bootloader.conf=conf-bootloader
+tools.one.bootloader.params.verbose=verbose
+tools.one.bootloader.params.quiet=quiet
+tools.one.bootloader.params.verify=verify
+tools.one.bootloader.params.noverify=noverify
+tools.one.bootloader.pattern={cmd.path} BURN {conf.board} {conf.general} {bootloader.conf} {bootloader.verbose} {bootloader.verify} {protocol} "{serial.port}" -b{upload.speed} -F{bootloader.fuses} "{runtime.platform.path}/bootloaders/{bootloader.file}"
+
+# Upload test 2
+tools.one-noport.cmd.path=echo
+tools.one-noport.conf.general=conf-general
+tools.one-noport.upload.conf=conf-upload
+tools.one-noport.upload.params.verbose=verbose
+tools.one-noport.upload.params.quiet=quiet
+tools.one-noport.upload.params.verify=verify
+tools.one-noport.upload.params.noverify=noverify
+tools.one-noport.upload.pattern={cmd.path} {conf.board} {conf.general} {upload.conf} {upload.verbose} {upload.verify} {upload.protocol} -b{upload.speed} "{build.path}/{build.project_name}.hex"
+
+tools.one-noport.program.conf=conf-program
+tools.one-noport.program.params.verbose=verbose
+tools.one-noport.program.params.quiet=quiet
+tools.one-noport.program.params.verify=verify
+tools.one-noport.program.params.noverify=noverify
+tools.one-noport.program.pattern={cmd.path} {conf.board} {conf.general} {program.conf} {program.verbose} {program.verify} {program.protocol} -b{upload.speed} "{build.path}/{build.project_name}.hex"
+
+# Recipes for "tool.one-extra-params"
+tools.one-extra-params.cmd.path=echo
+tools.one-extra-params.conf.general=conf-general
+
+tools.one-extra-params.program.conf=conf-program
+tools.one-extra-params.program.params.verbose=verbose
+tools.one-extra-params.program.params.quiet=quiet
+tools.one-extra-params.program.params.verify=verify
+tools.one-extra-params.program.params.noverify=noverify
+tools.one-extra-params.program.pattern={cmd.path} {conf.board} {conf.general} {program.conf} {program.verbose} {program.verify} {program.protocol} {program.extra_params} -b{upload.speed} "{build.path}/{build.project_name}.hex"
+
+# Upload with programmer test 1
+tools.two.cmd.path=echo
+tools.two.conf.general=conf-two-general
+
+tools.two.erase.conf=conf-two-erase
+tools.two.erase.params.verbose=verbose
+tools.two.erase.params.quiet=quiet
+tools.two.erase.params.verify=verify
+tools.two.erase.params.noverify=noverify
+tools.two.erase.pattern={cmd.path} ERASE {conf.board} {conf.general} {erase.conf} {erase.verbose} {erase.verify} {bootloader.protocol} "{serial.port}" -b{upload.speed}
+
+tools.two.bootloader.conf=conf-two-bootloader
+tools.two.bootloader.params.verbose=verbose
+tools.two.bootloader.params.quiet=quiet
+tools.two.bootloader.params.verify=verify
+tools.two.bootloader.params.noverify=noverify
+tools.two.bootloader.pattern={cmd.path} BURN {conf.board} {conf.general} {bootloader.conf} {bootloader.verbose} {bootloader.verify} {bootloader.protocol} "{serial.port}" -b{upload.speed} -F{bootloader.fuses} "{runtime.platform.path}/bootloaders/{bootloader.file}"

commands/upload/testdata/hardware/alice/avr/programmers.txt

@@ -0,0 +1,19 @@
+progr1.name=Programmer 1
+progr1.program.protocol=progprotocol
+progr1.program.tool=one
+progr1.protocol=genprog1protocol
+
+progr2.name=Programmer 2
+progr2.program.protocol=prog2protocol
+progr2.program.tool=one-noport
+
+progr3.name=Programmer 3
+progr3.program.protocol=prog3protocol
+progr3.program.tool=one-extra-params
+progr3.program.extra_params={serial.port}
+
+progr4.name=Programmer 4
+progr4.program.protocol=prog4protocol-upload
+progr4.program.tool=one
+progr4.bootloader.protocol=prog4protocol-bootloader
+progr4.bootloader.tool=two

commands/upload/upload.go

@@ -117,23 +117,8 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		WithField("buildPlatform", buildPlatform).
 		Tracef("Upload data")
 
-	// Load upload tool definitions
-	var uploadToolName string
-	var uploadToolPlatform *cores.PlatformRelease
+	// Extract programmer properties (when specified)
 	var programmer *cores.Programmer
-
-	if burnBootloader {
-		uploadToolName = boardProperties.Get("bootloader.tool")
-		uploadToolPlatform = boardPlatform
-		if uploadToolName == "" {
-			return fmt.Errorf("cannot get programmer tool: undefined 'bootloader.tool' in boards.txt")
-		}
-		logrus.
-			WithField("uploadToolName", uploadToolName).
-			WithField("uploadToolPlatform", uploadToolPlatform).
-			Trace("Upload tool from 'bootloader.tool' property")
-	}
-
 	if programmerID != "" {
 		programmer = boardPlatform.Programmers[programmerID]
 		if programmer == nil {
@@ -143,36 +128,54 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		if programmer == nil {
 			return fmt.Errorf("programmer '%s' not available", programmerID)
 		}
-		uploadToolName = programmer.Properties.Get("program.tool")
-		uploadToolPlatform = programmer.PlatformRelease
-		if uploadToolName == "" {
-			return fmt.Errorf("cannot get programmer tool: undefined 'program.tool' property")
+	}
+
+	// Determine upload tool
+	var uploadToolID string
+	{
+		toolProperty := "upload.tool"
+		if burnBootloader {
+			toolProperty = "bootloader.tool"
+		} else if programmer != nil {
+			toolProperty = "program.tool"
+		}
+
+		// create a temporary configuration only for the selection of upload tool
+		props := properties.NewMap()
+		props.Merge(boardPlatform.Properties)
+		props.Merge(boardPlatform.RuntimeProperties())
+		props.Merge(boardProperties)
+		if programmer != nil {
+			props.Merge(programmer.Properties)
+		}
+		if t, ok := props.GetOk(toolProperty); ok {
+			uploadToolID = t
+		} else {
+			return fmt.Errorf("cannot get programmer tool: undefined '%s' property", toolProperty)
 		}
-		logrus.
-			WithField("uploadToolName", uploadToolName).
-			WithField("uploadToolPlatform", uploadToolPlatform).
-			Trace("Upload tool from --programmer parameter")
+	}
+
+	var uploadToolPlatform *cores.PlatformRelease
+	if programmer != nil {
+		uploadToolPlatform = programmer.PlatformRelease
 	} else {
-		uploadToolName = boardProperties.Get("upload.tool")
 		uploadToolPlatform = boardPlatform
-		if uploadToolName == "" {
-			return fmt.Errorf("cannot get upload tool: undefined 'upload.tool' property")
-		}
-		if split := strings.Split(uploadToolName, ":"); len(split) > 2 {
-			return fmt.Errorf("invalid 'upload.tool' property: %s", uploadToolName)
-		} else if len(split) == 2 {
-			uploadToolName = split[1]
-			uploadToolPlatform = pm.GetInstalledPlatformRelease(
-				pm.FindPlatform(&packagemanager.PlatformReference{
-					Package:              split[0],
-					PlatformArchitecture: boardPlatform.Platform.Architecture,
-				}),
-			)
-		}
-		logrus.
-			WithField("uploadToolName", uploadToolName).
-			WithField("uploadToolPlatform", uploadToolPlatform).
-			Trace("Upload tool")
+	}
+	logrus.
+		WithField("uploadToolID", uploadToolID).
+		WithField("uploadToolPlatform", uploadToolPlatform).
+		Trace("Upload tool")
+
+	if split := strings.Split(uploadToolID, ":"); len(split) > 2 {
+		return fmt.Errorf("invalid 'upload.tool' property: %s", uploadToolID)
+	} else if len(split) == 2 {
+		uploadToolID = split[1]
+		uploadToolPlatform = pm.GetInstalledPlatformRelease(
+			pm.FindPlatform(&packagemanager.PlatformReference{
+				Package:              split[0],
+				PlatformArchitecture: boardPlatform.Platform.Architecture,
+			}),
+		)
 	}
 
 	// Build configuration for upload
@@ -183,9 +186,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	uploadProperties.Merge(boardPlatform.Properties)
 	uploadProperties.Merge(boardPlatform.RuntimeProperties())
 	uploadProperties.Merge(boardProperties)
-
-	uploadToolProperties := uploadProperties.SubTree("tools." + uploadToolName)
-	uploadProperties.Merge(uploadToolProperties)
+	uploadProperties.Merge(uploadProperties.SubTree("tools." + uploadToolID))
 	if programmer != nil {
 		uploadProperties.Merge(programmer.Properties)
 	}
@@ -234,9 +235,13 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	if verify {
 		uploadProperties.Set("upload.verify", uploadProperties.Get("upload.params.verify"))
 		uploadProperties.Set("program.verify", uploadProperties.Get("program.params.verify"))
+		uploadProperties.Set("erase.verify", uploadProperties.Get("erase.params.verify"))
+		uploadProperties.Set("bootloader.verify", uploadProperties.Get("bootloader.params.verify"))
 	} else {
 		uploadProperties.Set("upload.verify", uploadProperties.Get("upload.params.noverify"))
 		uploadProperties.Set("program.verify", uploadProperties.Get("program.params.noverify"))
+		uploadProperties.Set("erase.verify", uploadProperties.Get("erase.params.noverify"))
+		uploadProperties.Set("bootloader.verify", uploadProperties.Get("bootloader.params.noverify"))
 	}
 
 	if !burnBootloader {
@@ -307,7 +312,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		}
 	}
 
-	// Build recipe for upload
+	// Run recipes for upload
 	if burnBootloader {
 		if err := runTool("erase.pattern", uploadProperties, outStream, errStream, verbose); err != nil {
 			return fmt.Errorf("chip erase error: %s", err)

commands/upload/upload_test.go

@@ -16,12 +16,16 @@
 package upload
 
 import (
+	"bytes"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/arduino/arduino-cli/arduino/cores"
+	"github.com/arduino/arduino-cli/arduino/cores/packagemanager"
 	"github.com/arduino/arduino-cli/arduino/sketches"
 	paths "github.com/arduino/go-paths-helper"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 )
 
@@ -119,3 +123,95 @@ func TestDetermineBuildPathAndSketchName(t *testing.T) {
 		})
 	}
 }
+
+func TestUploadPropertiesComposition(t *testing.T) {
+	pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
+	err := pm.LoadHardwareFromDirectory(paths.New("testdata", "hardware"))
+	require.NoError(t, err)
+	buildPath1 := paths.New("testdata", "build_path_1")
+	logrus.SetLevel(logrus.TraceLevel)
+	type test struct {
+		importDir       *paths.Path
+		fqbn            string
+		port            string
+		programmer      string
+		burnBootloader  bool
+		expectedOutput  string
+		expectedOutput2 string
+	}
+
+	cwdPath, err := paths.Getwd()
+	require.NoError(t, err)
+	cwd := strings.ReplaceAll(cwdPath.String(), "\\", "/")
+
+	tests := []test{
+		// 0: classic upload, requires port
+		{buildPath1, "alice:avr:board1", "port", "", false, "conf-board1 conf-general conf-upload $$VERBOSE-VERIFY$$ protocol port -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
+		{buildPath1, "alice:avr:board1", "", "", false, "FAIL", ""},
+		// 2: classic upload, no port
+		{buildPath1, "alice:avr:board2", "port", "", false, "conf-board1 conf-general conf-upload $$VERBOSE-VERIFY$$ protocol -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
+		{buildPath1, "alice:avr:board2", "", "", false, "conf-board1 conf-general conf-upload $$VERBOSE-VERIFY$$ protocol -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
+
+		// 4: upload with programmer, requires port
+		{buildPath1, "alice:avr:board1", "port", "progr1", false, "conf-board1 conf-general conf-program $$VERBOSE-VERIFY$$ progprotocol port -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
+		{buildPath1, "alice:avr:board1", "", "progr1", false, "FAIL", ""},
+		// 6: upload with programmer, no port
+		{buildPath1, "alice:avr:board1", "port", "progr2", false, "conf-board1 conf-general conf-program $$VERBOSE-VERIFY$$ prog2protocol -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
+		{buildPath1, "alice:avr:board1", "", "progr2", false, "conf-board1 conf-general conf-program $$VERBOSE-VERIFY$$ prog2protocol -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
+		// 8: upload with programmer, require port through extra params
+		{buildPath1, "alice:avr:board1", "port", "progr3", false, "conf-board1 conf-general conf-program $$VERBOSE-VERIFY$$ prog3protocol port -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
+		{buildPath1, "alice:avr:board1", "", "progr3", false, "FAIL", ""},
+
+		// 10: burn bootloader, require port
+		{buildPath1, "alice:avr:board1", "port", "", true, "FAIL", ""}, // requires programmer
+		{buildPath1, "alice:avr:board1", "port", "progr1", true,
+			"ERASE conf-board1 conf-general conf-erase $$VERBOSE-VERIFY$$ genprog1protocol port -bspeed\n",
+			"BURN conf-board1 conf-general conf-bootloader $$VERBOSE-VERIFY$$ genprog1protocol port -bspeed -F0xFF " + cwd + "/testdata/hardware/alice/avr/bootloaders/niceboot/niceboot.hex\n"},
+
+		// 12: burn bootloader, preferences override from programmers.txt
+		{buildPath1, "alice:avr:board1", "port", "progr4", true,
+			"ERASE conf-board1 conf-two-general conf-two-erase $$VERBOSE-VERIFY$$ prog4protocol-bootloader port -bspeed\n",
+			"BURN conf-board1 conf-two-general conf-two-bootloader $$VERBOSE-VERIFY$$ prog4protocol-bootloader port -bspeed -F0xFF " + cwd + "/testdata/hardware/alice/avr/bootloaders/niceboot/niceboot.hex\n"},
+	}
+
+	testRunner := func(t *testing.T, test test, verboseVerify bool) {
+		outStream := &bytes.Buffer{}
+		errStream := &bytes.Buffer{}
+		err := runProgramAction(
+			pm,
+			nil,                     // sketch
+			"",                      // importFile
+			test.importDir.String(), // importDir
+			test.fqbn,               // FQBN
+			test.port,               // port
+			test.programmer,         // programmer
+			verboseVerify,           // verbose
+			verboseVerify,           // verify
+			test.burnBootloader,     // burnBootloader
+			outStream,
+			errStream,
+		)
+		verboseVerifyOutput := "verbose verify"
+		if !verboseVerify {
+			verboseVerifyOutput = "quiet noverify"
+		}
+		if test.expectedOutput == "FAIL" {
+			require.Error(t, err)
+		} else {
+			require.NoError(t, err)
+			outFiltered := strings.ReplaceAll(outStream.String(), "\r", "")
+			outFiltered = strings.ReplaceAll(outFiltered, "\\", "/")
+			require.Contains(t, outFiltered, strings.ReplaceAll(test.expectedOutput, "$$VERBOSE-VERIFY$$", verboseVerifyOutput))
+			require.Contains(t, outFiltered, strings.ReplaceAll(test.expectedOutput2, "$$VERBOSE-VERIFY$$", verboseVerifyOutput))
+		}
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("SubTest%02d", i), func(t *testing.T) {
+			testRunner(t, test, false)
+		})
+		t.Run(fmt.Sprintf("SubTest%02d-WithVerifyAndVerbose", i), func(t *testing.T) {
+			testRunner(t, test, true)
+		})
+	}
+}