Skip to content

add back --retries flag #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 18, 2021
Merged

add back --retries flag #61

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
227e286
add retries flag (just like in #24)
umbynos Jun 17, 2021
ce54141
forgot continue statement
umbynos Jun 18, 2021
f3fb1ff
applied suggestions
umbynos Jun 18, 2021
c11c3eb
the json is printed even in case of error now
umbynos Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 86 additions & 70 deletions cli/firmware/flash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ var (
fqbn string
address string
module string
retries uint8
)

// NewCommand created a new `version` command
Expand All @@ -63,6 +64,7 @@ func NewFlashCommand() *cobra.Command {
command.Flags().StringVarP(&fqbn, "fqbn", "b", "", "Fully Qualified Board Name, e.g.: arduino:samd:mkr1000, arduino:mbed_nano:nanorp2040connect")
command.Flags().StringVarP(&address, "address", "a", "", "Upload port, e.g.: COM10, /dev/ttyACM0")
command.Flags().StringVarP(&module, "module", "m", "", "Firmware module ID, e.g.: WINC1500, NINA")
command.Flags().Uint8Var(&retries, "retries", 9, "Number of retries in case of upload failure (default 9)")
return command
}

Expand Down Expand Up @@ -159,82 +161,96 @@ func run(cmd *cobra.Command, args []string) {
os.Exit(errorcodes.ErrGeneric)
}

// Check if board needs a 1200bps touch for upload
uploadPort := address
if board.UploadTouch {
logrus.Info("Putting board into bootloader mode")
newUploadPort, err := serialutils.Reset(address, board.UploadWait, nil)
for retry := 0; retry <= int(retries); retry++ {

if retry == int(retries) {
logrus.Fatal("Operation failed. :-(")
}

if retry != 0 {
logrus.Infof("Retrying upload (%d of %d)", retry, retries)
}

// Check if board needs a 1200bps touch for upload
uploadPort := address
if board.UploadTouch {
logrus.Info("Putting board into bootloader mode")
newUploadPort, err := serialutils.Reset(address, board.UploadWait, nil)
if err != nil {
feedback.Errorf("Error during firmware flashing: missing board address")
continue
}
if newUploadPort != "" {
logrus.Infof("Found port to upload Loader: %s", newUploadPort)
uploadPort = newUploadPort
}
}

// Flash loader Sketch
programmerOut := new(bytes.Buffer)
programmerErr := new(bytes.Buffer)
if feedback.GetFormat() == feedback.JSON {
err = programmer.Flash(commandLine, programmerOut, programmerErr)
} else {
err = programmer.Flash(commandLine, os.Stdout, os.Stderr)
}
if err != nil {
feedback.Errorf("Error during firmware flashing: missing board address")
feedback.Errorf("Error during firmware flashing: %s", err)
continue
}

// Wait a bit after flashing the loader sketch for the board to become
// available again.
time.Sleep(2 * time.Second)

// Get flasher depending on which module to use
var f flasher.Flasher
switch moduleName {
case "NINA":
f, err = flasher.NewNinaFlasher(uploadPort)
case "SARA":
f, err = flasher.NewSaraFlasher(uploadPort)
case "WINC1500":
f, err = flasher.NewWincFlasher(uploadPort)
default:
err = fmt.Errorf("unknown module: %s", moduleName)
feedback.Errorf("Error during firmware flashing: %s", err)
os.Exit(errorcodes.ErrGeneric)
}
if newUploadPort != "" {
logrus.Infof("Found port to upload Loader: %s", newUploadPort)
uploadPort = newUploadPort
if err != nil {
feedback.Errorf("Error during firmware flashing: %s", err)
continue
}
}

// Flash loader Sketch
programmerOut := new(bytes.Buffer)
programmerErr := new(bytes.Buffer)
if feedback.GetFormat() == feedback.JSON {
err = programmer.Flash(commandLine, programmerOut, programmerErr)
} else {
err = programmer.Flash(commandLine, os.Stdout, os.Stderr)
}
if err != nil {
feedback.Errorf("Error during firmware flashing: %s", err)
os.Exit(errorcodes.ErrGeneric)
}

// Wait a bit after flashing the loader sketch for the board to become
// available again.
time.Sleep(2 * time.Second)

// Get flasher depending on which module to use
var f flasher.Flasher
switch moduleName {
case "NINA":
f, err = flasher.NewNinaFlasher(uploadPort)
case "SARA":
f, err = flasher.NewSaraFlasher(uploadPort)
case "WINC1500":
f, err = flasher.NewWincFlasher(uploadPort)
default:
err = fmt.Errorf("unknown module: %s", moduleName)
}
if err != nil {
feedback.Errorf("Error during firmware flashing: %s", err)
os.Exit(errorcodes.ErrGeneric)
}
defer f.Close()
// now flash the actual firmware
flasherOut := new(bytes.Buffer)
flasherErr := new(bytes.Buffer)
if feedback.GetFormat() == feedback.JSON {
err = f.FlashFirmware(firmwareFile, flasherOut)
} else {
err = f.FlashFirmware(firmwareFile, os.Stdout)
}
if err != nil {
feedback.Errorf("Error during firmware flashing: %s", err)
flasherErr.Write([]byte(fmt.Sprintf("Error during firmware flashing: %s", err)))
}
f.Close()

// now flash the actual firmware
flasherOut := new(bytes.Buffer)
flasherErr := new(bytes.Buffer)
if feedback.GetFormat() == feedback.JSON {
err = f.FlashFirmware(firmwareFile, flasherOut)
} else {
err = f.FlashFirmware(firmwareFile, os.Stdout)
}
if err != nil {
feedback.Errorf("Error during firmware flashing: %s", err)
flasherErr.Write([]byte(fmt.Sprintf("Error during firmware flashing: %s", err)))
}
// Print the results
feedback.PrintResult(&flasher.FlashResult{
Programmer: (&flasher.ExecOutput{
Stdout: programmerOut.String(),
Stderr: programmerErr.String(),
}),
Flasher: (&flasher.ExecOutput{
Stdout: flasherOut.String(),
Stderr: flasherErr.String(),
}),
})

// Print the results
feedback.PrintResult(&flasher.FlashResult{
Programmer: (&flasher.ExecOutput{
Stdout: programmerOut.String(),
Stderr: programmerErr.String(),
}),
Flasher: (&flasher.ExecOutput{
Stdout: flasherOut.String(),
Stderr: flasherErr.String(),
}),
})
// Exit if something went wrong but after printing
if err != nil {
os.Exit(errorcodes.ErrGeneric)
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this here only as a "belt and suspenders" measure in case there was some missed error handling in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this condition is here because it allows breaking from the retry loop. Not sure if this can be handled better...
After every error in the loop there's a continue statement that allows to start from the next iteration of the loop in case something is wrong

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After every error in the loop there's a continue statement that allows to start from the next iteration of the loop in case something is wrong

And for that reason, err should never have any value other than nil if this condition is reached, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈 I forgot the continue statement here https://github.com/arduino/FirmwareUploader/pull/61/files#diff-9bcc47c1909291c8177f70e16def7a94afe0d6c3fb6c937a8f2ccc91432b8aecR233
Thanks a lot @per1234

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'm mostly just trying to make sure I understand the code. Since it should never be other than nil if the code is working correctly, I was wondering if a better "belt and suspenders" approach wouldn't be to panic:

if err !=nil{
  panic(fmt.Sprintf("Unhandled error: %s", err))
}
break

I generally just throw in a panic to handle situations that should never occur unless something is broken in the code. It's not clear to me whether it would be beneficial to make the code resilient to this condition, since it could mask a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that behavior is present here.
At the end of the specified numbers of retries the program exits

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that's a different situation from an unhandled error.

Anyway, I am happy with it as is. I think the retries are a very nice improvement. I always wish AVRDUDE had this option.

logrus.Info("Operation completed: success! :-)")
break
}
}
}