Skip to content

Commit 4777e1d

Browse files
authored
Refactored upload procedure to correctly handle bootloader.tool property (#952)
* Added more tests for upload * Added test for upload without port * Added test for upload without programmer (with and w/o port) * Added test for burn bootloader * Added test for burn bootloader with bootloader.tool options override * Increased verbosity * Refactored upload procedure to correctly handle bootloader.tool property * Handle windows quirks on command output * Simplified tests
1 parent 03b7738 commit 4777e1d

File tree

5 files changed

+269
-47
lines changed

5 files changed

+269
-47
lines changed
+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
board1.name=board1
2+
board1.conf.board=conf-board1
3+
board1.upload.tool=one
4+
board1.upload.protocol=protocol
5+
board1.upload.speed=speed
6+
board1.bootloader.tool=one
7+
board1.bootloader.fuses=0xFF
8+
board1.bootloader.file=niceboot/niceboot.hex
9+
10+
11+
board2.name=board2
12+
board2.conf.board=conf-board1
13+
board2.upload.tool=one-noport
14+
board2.upload.protocol=protocol
15+
board2.upload.speed=speed
16+
17+
board2.bootloader.tool=one
18+
board2.bootloader.low_fuses=0xFF
19+
board2.bootloader.high_fuses=0xDE
20+
board2.bootloader.extended_fuses=0xFD
21+
board2.bootloader.unlock_bits=0x3F
22+
board2.bootloader.lock_bits=0x0F
23+
board2.bootloader.file=optiboot/optiboot_atmega328.hex
+79
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
name=Upload Test Platform (Alice)
2+
version=1.0.0
3+
4+
# Upload test 1
5+
tools.one.cmd.path=echo
6+
tools.one.conf.general=conf-general
7+
tools.one.upload.conf=conf-upload
8+
tools.one.upload.params.verbose=verbose
9+
tools.one.upload.params.quiet=quiet
10+
tools.one.upload.params.verify=verify
11+
tools.one.upload.params.noverify=noverify
12+
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"
13+
14+
tools.one.program.conf=conf-program
15+
tools.one.program.params.verbose=verbose
16+
tools.one.program.params.quiet=quiet
17+
tools.one.program.params.verify=verify
18+
tools.one.program.params.noverify=noverify
19+
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"
20+
21+
tools.one.erase.conf=conf-erase
22+
tools.one.erase.params.verbose=verbose
23+
tools.one.erase.params.quiet=quiet
24+
tools.one.erase.params.verify=verify
25+
tools.one.erase.params.noverify=noverify
26+
tools.one.erase.pattern={cmd.path} ERASE {conf.board} {conf.general} {erase.conf} {erase.verbose} {erase.verify} {protocol} "{serial.port}" -b{upload.speed}
27+
28+
tools.one.bootloader.conf=conf-bootloader
29+
tools.one.bootloader.params.verbose=verbose
30+
tools.one.bootloader.params.quiet=quiet
31+
tools.one.bootloader.params.verify=verify
32+
tools.one.bootloader.params.noverify=noverify
33+
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}"
34+
35+
# Upload test 2
36+
tools.one-noport.cmd.path=echo
37+
tools.one-noport.conf.general=conf-general
38+
tools.one-noport.upload.conf=conf-upload
39+
tools.one-noport.upload.params.verbose=verbose
40+
tools.one-noport.upload.params.quiet=quiet
41+
tools.one-noport.upload.params.verify=verify
42+
tools.one-noport.upload.params.noverify=noverify
43+
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"
44+
45+
tools.one-noport.program.conf=conf-program
46+
tools.one-noport.program.params.verbose=verbose
47+
tools.one-noport.program.params.quiet=quiet
48+
tools.one-noport.program.params.verify=verify
49+
tools.one-noport.program.params.noverify=noverify
50+
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"
51+
52+
# Recipes for "tool.one-extra-params"
53+
tools.one-extra-params.cmd.path=echo
54+
tools.one-extra-params.conf.general=conf-general
55+
56+
tools.one-extra-params.program.conf=conf-program
57+
tools.one-extra-params.program.params.verbose=verbose
58+
tools.one-extra-params.program.params.quiet=quiet
59+
tools.one-extra-params.program.params.verify=verify
60+
tools.one-extra-params.program.params.noverify=noverify
61+
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"
62+
63+
# Upload with programmer test 1
64+
tools.two.cmd.path=echo
65+
tools.two.conf.general=conf-two-general
66+
67+
tools.two.erase.conf=conf-two-erase
68+
tools.two.erase.params.verbose=verbose
69+
tools.two.erase.params.quiet=quiet
70+
tools.two.erase.params.verify=verify
71+
tools.two.erase.params.noverify=noverify
72+
tools.two.erase.pattern={cmd.path} ERASE {conf.board} {conf.general} {erase.conf} {erase.verbose} {erase.verify} {bootloader.protocol} "{serial.port}" -b{upload.speed}
73+
74+
tools.two.bootloader.conf=conf-two-bootloader
75+
tools.two.bootloader.params.verbose=verbose
76+
tools.two.bootloader.params.quiet=quiet
77+
tools.two.bootloader.params.verify=verify
78+
tools.two.bootloader.params.noverify=noverify
79+
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}"
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
progr1.name=Programmer 1
2+
progr1.program.protocol=progprotocol
3+
progr1.program.tool=one
4+
progr1.protocol=genprog1protocol
5+
6+
progr2.name=Programmer 2
7+
progr2.program.protocol=prog2protocol
8+
progr2.program.tool=one-noport
9+
10+
progr3.name=Programmer 3
11+
progr3.program.protocol=prog3protocol
12+
progr3.program.tool=one-extra-params
13+
progr3.program.extra_params={serial.port}
14+
15+
progr4.name=Programmer 4
16+
progr4.program.protocol=prog4protocol-upload
17+
progr4.program.tool=one
18+
progr4.bootloader.protocol=prog4protocol-bootloader
19+
progr4.bootloader.tool=two

Diff for: commands/upload/upload.go

+52-47
Original file line numberDiff line numberDiff line change
@@ -117,23 +117,8 @@ func runProgramAction(pm *packagemanager.PackageManager,
117117
WithField("buildPlatform", buildPlatform).
118118
Tracef("Upload data")
119119

120-
// Load upload tool definitions
121-
var uploadToolName string
122-
var uploadToolPlatform *cores.PlatformRelease
120+
// Extract programmer properties (when specified)
123121
var programmer *cores.Programmer
124-
125-
if burnBootloader {
126-
uploadToolName = boardProperties.Get("bootloader.tool")
127-
uploadToolPlatform = boardPlatform
128-
if uploadToolName == "" {
129-
return fmt.Errorf("cannot get programmer tool: undefined 'bootloader.tool' in boards.txt")
130-
}
131-
logrus.
132-
WithField("uploadToolName", uploadToolName).
133-
WithField("uploadToolPlatform", uploadToolPlatform).
134-
Trace("Upload tool from 'bootloader.tool' property")
135-
}
136-
137122
if programmerID != "" {
138123
programmer = boardPlatform.Programmers[programmerID]
139124
if programmer == nil {
@@ -143,36 +128,54 @@ func runProgramAction(pm *packagemanager.PackageManager,
143128
if programmer == nil {
144129
return fmt.Errorf("programmer '%s' not available", programmerID)
145130
}
146-
uploadToolName = programmer.Properties.Get("program.tool")
147-
uploadToolPlatform = programmer.PlatformRelease
148-
if uploadToolName == "" {
149-
return fmt.Errorf("cannot get programmer tool: undefined 'program.tool' property")
131+
}
132+
133+
// Determine upload tool
134+
var uploadToolID string
135+
{
136+
toolProperty := "upload.tool"
137+
if burnBootloader {
138+
toolProperty = "bootloader.tool"
139+
} else if programmer != nil {
140+
toolProperty = "program.tool"
141+
}
142+
143+
// create a temporary configuration only for the selection of upload tool
144+
props := properties.NewMap()
145+
props.Merge(boardPlatform.Properties)
146+
props.Merge(boardPlatform.RuntimeProperties())
147+
props.Merge(boardProperties)
148+
if programmer != nil {
149+
props.Merge(programmer.Properties)
150+
}
151+
if t, ok := props.GetOk(toolProperty); ok {
152+
uploadToolID = t
153+
} else {
154+
return fmt.Errorf("cannot get programmer tool: undefined '%s' property", toolProperty)
150155
}
151-
logrus.
152-
WithField("uploadToolName", uploadToolName).
153-
WithField("uploadToolPlatform", uploadToolPlatform).
154-
Trace("Upload tool from --programmer parameter")
156+
}
157+
158+
var uploadToolPlatform *cores.PlatformRelease
159+
if programmer != nil {
160+
uploadToolPlatform = programmer.PlatformRelease
155161
} else {
156-
uploadToolName = boardProperties.Get("upload.tool")
157162
uploadToolPlatform = boardPlatform
158-
if uploadToolName == "" {
159-
return fmt.Errorf("cannot get upload tool: undefined 'upload.tool' property")
160-
}
161-
if split := strings.Split(uploadToolName, ":"); len(split) > 2 {
162-
return fmt.Errorf("invalid 'upload.tool' property: %s", uploadToolName)
163-
} else if len(split) == 2 {
164-
uploadToolName = split[1]
165-
uploadToolPlatform = pm.GetInstalledPlatformRelease(
166-
pm.FindPlatform(&packagemanager.PlatformReference{
167-
Package: split[0],
168-
PlatformArchitecture: boardPlatform.Platform.Architecture,
169-
}),
170-
)
171-
}
172-
logrus.
173-
WithField("uploadToolName", uploadToolName).
174-
WithField("uploadToolPlatform", uploadToolPlatform).
175-
Trace("Upload tool")
163+
}
164+
logrus.
165+
WithField("uploadToolID", uploadToolID).
166+
WithField("uploadToolPlatform", uploadToolPlatform).
167+
Trace("Upload tool")
168+
169+
if split := strings.Split(uploadToolID, ":"); len(split) > 2 {
170+
return fmt.Errorf("invalid 'upload.tool' property: %s", uploadToolID)
171+
} else if len(split) == 2 {
172+
uploadToolID = split[1]
173+
uploadToolPlatform = pm.GetInstalledPlatformRelease(
174+
pm.FindPlatform(&packagemanager.PlatformReference{
175+
Package: split[0],
176+
PlatformArchitecture: boardPlatform.Platform.Architecture,
177+
}),
178+
)
176179
}
177180

178181
// Build configuration for upload
@@ -183,9 +186,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
183186
uploadProperties.Merge(boardPlatform.Properties)
184187
uploadProperties.Merge(boardPlatform.RuntimeProperties())
185188
uploadProperties.Merge(boardProperties)
186-
187-
uploadToolProperties := uploadProperties.SubTree("tools." + uploadToolName)
188-
uploadProperties.Merge(uploadToolProperties)
189+
uploadProperties.Merge(uploadProperties.SubTree("tools." + uploadToolID))
189190
if programmer != nil {
190191
uploadProperties.Merge(programmer.Properties)
191192
}
@@ -234,9 +235,13 @@ func runProgramAction(pm *packagemanager.PackageManager,
234235
if verify {
235236
uploadProperties.Set("upload.verify", uploadProperties.Get("upload.params.verify"))
236237
uploadProperties.Set("program.verify", uploadProperties.Get("program.params.verify"))
238+
uploadProperties.Set("erase.verify", uploadProperties.Get("erase.params.verify"))
239+
uploadProperties.Set("bootloader.verify", uploadProperties.Get("bootloader.params.verify"))
237240
} else {
238241
uploadProperties.Set("upload.verify", uploadProperties.Get("upload.params.noverify"))
239242
uploadProperties.Set("program.verify", uploadProperties.Get("program.params.noverify"))
243+
uploadProperties.Set("erase.verify", uploadProperties.Get("erase.params.noverify"))
244+
uploadProperties.Set("bootloader.verify", uploadProperties.Get("bootloader.params.noverify"))
240245
}
241246

242247
if !burnBootloader {
@@ -307,7 +312,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
307312
}
308313
}
309314

310-
// Build recipe for upload
315+
// Run recipes for upload
311316
if burnBootloader {
312317
if err := runTool("erase.pattern", uploadProperties, outStream, errStream, verbose); err != nil {
313318
return fmt.Errorf("chip erase error: %s", err)

Diff for: commands/upload/upload_test.go

+96
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,16 @@
1616
package upload
1717

1818
import (
19+
"bytes"
1920
"fmt"
21+
"strings"
2022
"testing"
2123

2224
"github.com/arduino/arduino-cli/arduino/cores"
25+
"github.com/arduino/arduino-cli/arduino/cores/packagemanager"
2326
"github.com/arduino/arduino-cli/arduino/sketches"
2427
paths "github.com/arduino/go-paths-helper"
28+
"github.com/sirupsen/logrus"
2529
"github.com/stretchr/testify/require"
2630
)
2731

@@ -121,3 +125,95 @@ func TestDetermineBuildPathAndSketchName(t *testing.T) {
121125
})
122126
}
123127
}
128+
129+
func TestUploadPropertiesComposition(t *testing.T) {
130+
pm := packagemanager.NewPackageManager(nil, nil, nil, nil)
131+
err := pm.LoadHardwareFromDirectory(paths.New("testdata", "hardware"))
132+
require.NoError(t, err)
133+
buildPath1 := paths.New("testdata", "build_path_1")
134+
logrus.SetLevel(logrus.TraceLevel)
135+
type test struct {
136+
importDir *paths.Path
137+
fqbn string
138+
port string
139+
programmer string
140+
burnBootloader bool
141+
expectedOutput string
142+
expectedOutput2 string
143+
}
144+
145+
cwdPath, err := paths.Getwd()
146+
require.NoError(t, err)
147+
cwd := strings.ReplaceAll(cwdPath.String(), "\\", "/")
148+
149+
tests := []test{
150+
// 0: classic upload, requires port
151+
{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", ""},
152+
{buildPath1, "alice:avr:board1", "", "", false, "FAIL", ""},
153+
// 2: classic upload, no port
154+
{buildPath1, "alice:avr:board2", "port", "", false, "conf-board1 conf-general conf-upload $$VERBOSE-VERIFY$$ protocol -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
155+
{buildPath1, "alice:avr:board2", "", "", false, "conf-board1 conf-general conf-upload $$VERBOSE-VERIFY$$ protocol -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
156+
157+
// 4: upload with programmer, requires port
158+
{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", ""},
159+
{buildPath1, "alice:avr:board1", "", "progr1", false, "FAIL", ""},
160+
// 6: upload with programmer, no port
161+
{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", ""},
162+
{buildPath1, "alice:avr:board1", "", "progr2", false, "conf-board1 conf-general conf-program $$VERBOSE-VERIFY$$ prog2protocol -bspeed testdata/build_path_1/sketch.ino.hex\n", ""},
163+
// 8: upload with programmer, require port through extra params
164+
{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", ""},
165+
{buildPath1, "alice:avr:board1", "", "progr3", false, "FAIL", ""},
166+
167+
// 10: burn bootloader, require port
168+
{buildPath1, "alice:avr:board1", "port", "", true, "FAIL", ""}, // requires programmer
169+
{buildPath1, "alice:avr:board1", "port", "progr1", true,
170+
"ERASE conf-board1 conf-general conf-erase $$VERBOSE-VERIFY$$ genprog1protocol port -bspeed\n",
171+
"BURN conf-board1 conf-general conf-bootloader $$VERBOSE-VERIFY$$ genprog1protocol port -bspeed -F0xFF " + cwd + "/testdata/hardware/alice/avr/bootloaders/niceboot/niceboot.hex\n"},
172+
173+
// 12: burn bootloader, preferences override from programmers.txt
174+
{buildPath1, "alice:avr:board1", "port", "progr4", true,
175+
"ERASE conf-board1 conf-two-general conf-two-erase $$VERBOSE-VERIFY$$ prog4protocol-bootloader port -bspeed\n",
176+
"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"},
177+
}
178+
179+
testRunner := func(t *testing.T, test test, verboseVerify bool) {
180+
outStream := &bytes.Buffer{}
181+
errStream := &bytes.Buffer{}
182+
err := runProgramAction(
183+
pm,
184+
nil, // sketch
185+
"", // importFile
186+
test.importDir.String(), // importDir
187+
test.fqbn, // FQBN
188+
test.port, // port
189+
test.programmer, // programmer
190+
verboseVerify, // verbose
191+
verboseVerify, // verify
192+
test.burnBootloader, // burnBootloader
193+
outStream,
194+
errStream,
195+
)
196+
verboseVerifyOutput := "verbose verify"
197+
if !verboseVerify {
198+
verboseVerifyOutput = "quiet noverify"
199+
}
200+
if test.expectedOutput == "FAIL" {
201+
require.Error(t, err)
202+
} else {
203+
require.NoError(t, err)
204+
outFiltered := strings.ReplaceAll(outStream.String(), "\r", "")
205+
outFiltered = strings.ReplaceAll(outFiltered, "\\", "/")
206+
require.Contains(t, outFiltered, strings.ReplaceAll(test.expectedOutput, "$$VERBOSE-VERIFY$$", verboseVerifyOutput))
207+
require.Contains(t, outFiltered, strings.ReplaceAll(test.expectedOutput2, "$$VERBOSE-VERIFY$$", verboseVerifyOutput))
208+
}
209+
}
210+
211+
for i, test := range tests {
212+
t.Run(fmt.Sprintf("SubTest%02d", i), func(t *testing.T) {
213+
testRunner(t, test, false)
214+
})
215+
t.Run(fmt.Sprintf("SubTest%02d-WithVerifyAndVerbose", i), func(t *testing.T) {
216+
testRunner(t, test, true)
217+
})
218+
}
219+
}

0 commit comments

Comments
 (0)