Fix upload command not working as expected when certain flags are used #1429
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
silvanocerza
force-pushed
the
scerza/fix-upload-command
branch
from
54422aa to
6cf5613
Compare
per1234
reviewed
Sep 2, 2021
cli/upload/upload.go
Outdated
| var sketchPath *paths.Path | ||
| var sk *sketch.Sketch = nil | ||
| // If the user explicitly specified a directory that contains binaries to upload | ||
| // use those, we don't really need a valid Sketch to upload in this case |
There was a problem hiding this comment.
Do we care about this use case (which used to be supported), where the sketch argument could be used to provide sketch.json metadata when using a binary?:
$ ./arduino-cli version
arduino-cli.exe alpha Version: 0.18.3 Commit: d710b642 Date: 2021-05-14T12:36:58Z
$ ./arduino-cli sketch new /tmp/FooSketch
Sketch created in: C:\Users\per\AppData\Local\Temp\FooSketch
$ ./arduino-cli board attach arduino:avr:uno /tmp/FooSketch
Selected fqbn: arduino:avr:uno
$ ./arduino-cli compile --output-dir /tmp/FooSketchBin /tmp/FooSketch
Sketch uses 444 bytes (1%) of program storage space. Maximum is 32256 bytes.
Global variables use 9 bytes (0%) of dynamic memory, leaving 2039 bytes for local variables. Maximum is 2048 bytes.
$ ./arduino-cli upload -v -p COM10 --input-dir /tmp/FooSketchBin /tmp/FooSketch
"C:\program-files\arduino\cli\arduino-cli_nightly\directories-data\packages\arduino\tools\avrdude\6.3.0-arduino17/bin/avrdude" "-CC:\program-files\arduino\cli\arduino-cli_nightly\directories-data\packages\arduino\tools\avrdude\6.3.0-arduino17/etc/avrdude.conf" -v -V -patmega328p -carduino "-PCOM10" -b115200 -D "-Uflash:w:C:/Users/per/AppData/Local/Temp/FooSketchBin/FooSketch.ino.hex:i"
avrdude: Version 6.3-20190619
Copyright (c) 2000-2005 Brian Dean, http://www.bdmicro.com/
[...]
There was a problem hiding this comment.
Oh right, didn't think about that case. 🤔
I'll try to fix it.
There was a problem hiding this comment.
Fixed with 51b1be6.
silvanocerza
force-pushed
the
scerza/fix-upload-command
branch
from
02539da to
51b1be6
Compare
per1234
approved these changes
Sep 2, 2021
silvanocerza
added a commit
that referenced
this pull request
Sep 2, 2021
#1429) * Fix upload command not working as expected when certain flags are used * Fix absurd upload corner case
rsora
added
type: imperfection
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)Fixes two bugs with a CLI command.
uploadfails if the--input-diror--input-fileare used and Sketch path is not specified.uploadfails if--fqbnis not set even if specified in asketch.jsonfile.uploaddoesn't fail--input-diror--input-fileare set and no Sketch path is specified.uploaddoesn't fail if--fqbnis not specified andsketch.jsoncontains a valid FQBN.titled accordingly?
Nope.
To verify that this PR fixes the aforementioned issue the integration tests must be run locally with a board connected.
Arduino Uno and Arduino MKR WiFi 1010 are recommended.
See how to contribute