Fix unneeded "Select port on upload" message #8194
Conversation
|
✅ Build completed. Please test this code using one of the following: ⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8194-BUILD-804-linux32.tar.xz ℹ️ The |
|
I have tested it under Win7 64bit and with boards without serial port it is working. But if a board with serial port is selected like Arduino Uno and no serial port is available then there is an error: |
|
thanks for testing! yes there is this small unhandled case: if (portMenu.getItemCount() == 0) statusError(e); <---
else if (serialPrompt()) run();
else statusNotice(tr("Upload canceled."));I'm changing it to: if (portMenu.getItemCount() == 0) {
statusError(tr("Serial port not selected."));
} else {
if (serialPrompt()) {
run();
} else {
statusNotice(tr("Upload canceled."));
}
}so in case no ports are available "Serial port not selected." is printed. |
|
✅ Build completed. Please test this code using one of the following: ⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8194-BUILD-805-linux32.tar.xz ℹ️ The |
|
Yes, that is better and now everything is working. |
cmaglie
added
Type: Bug
Component: IDE user interface
There was a problem hiding this comment.
Seems I'm a bit late to the party. I meant to review this PR, but hadn't found the time. I did a review now anyway.
Great to see this refactoring, that improves a lot. Yay for killing the UploadAppHandler stuff that was not relevantly named anymore. Next up, renaming exportApplet?
I noticed a noUploadPort argument in UploaderUtils:
public Uploader getUploaderByPreferences(boolean noUploadPort) {
I wonder if that is something that should be set? Or perhaps it is no longer relevant? AFAICS, the argument is only set to true when passing --nouploadport on the commandline, but I haven't traced what it does exactly.
I haven't fully understood the core commit of this PR yet (haven't dug into the code deeply), but I did review all others. I left some remarks inline, of things that might be good to fix in the future (but nothing that actually breaks behaviour, I believe).
| src = res; | ||
| } | ||
|
|
||
| // If the resulting string contains the tag, then the key is required |
There was a problem hiding this comment.
This is not guaranteed: For example, of the string is "{foo}", a tag of "bar" is chosen (which is not in "{foo}"), but the "foo" variable contains "some string with bar", then this code will conclude that whatever key is passed is required, while it might not be.
Did you consider adding some instrumentation to replaceFromMapping to either check if a key is used, or more generically, let it return a list of keys it expanded?
There was a problem hiding this comment.
This can't happen because tag is guaranteed to be different from any key/value contained in the map and either from any part of the pattern recipe, see how it is choosen in the while loop just before this.
There was a problem hiding this comment.
Did you consider adding some instrumentation to replaceFromMapping to either check if a key is used, or more generically, let it return a list of keys it expanded?
This is trickier than it looks :-)
- returning the keys it expanded, won't help in detecting if a key is required, it could be that, for example,
{serial.port}is required but it's not expanded because the keyserial.portit's not present in the preferences map. - we could "detect" a list of keys that needs to be replaced using a regexp that looks at matching braces in the recipes, but there are some recipes where the braces are actually needed! see this
openocdcommand for instance:
tools.openocd.program.pattern="{path}/{cmd}" {program.verbose} -s "{path}/share/openocd/scripts/" -f "{runtime.platform.path}/variants/{build.variant}/{build.openocdscript}" -c "telnet_port disabled; program {{{build.path}/{build.project_name}.elf}} verify reset; shutdown"
the part{{{build.path}/{build.project_name}.elf}}will translate to{{my/sketch.elf}}but this doesn't mean thatmy/sketch.elfis a required field.
| else if (serialPrompt()) run(); | ||
| else statusNotice(tr("Upload canceled.")); | ||
| if (portMenu.getItemCount() == 0) { | ||
| statusError(tr("Serial port not selected.")); |
There was a problem hiding this comment.
Shouldn't this be "no serial ports available" or something?
| private boolean runCommand(String patternKey, PreferencesMap prefs) throws Exception, RunnerException { | ||
| try { | ||
| String pattern = prefs.getOrExcept(patternKey); | ||
| StringReplacer.checkIfRequiredKeyIsMissingOrExcept("serial.port", pattern, prefs); |
There was a problem hiding this comment.
Is this the only line that can throw a PreferencesMapException? If so, wouldn't it be better to have a smaller try block for that (or perhaps even let checkIfRequiredKeyIsMissingOrExcept return a boolean, if all it does is except or not except)?
Reading this, I wonder: Shouldn't StringReplacer.formatAndSplit (etc.) just throw when they find any unset variable? Or does that break for optional variables in some cases?
|
404 Page not found |
|
@rscarlat - The fix is in git master, but not any released version yet. I believe it should be available in the hourly build version at https://www.arduino.cc/en/Main/Software (or perhaps in the beta build - I keep getting confused by the existance of both...). |
|
Since this PR has been merged in master it has been automatically built and deployed as Hourly (and temporary PR urls have been deleted). |
This turned out to be really tricky: to reliably understand if the serial port is actually needed the only way is to check if the recipe contains the variable
{serial.port}considering also the fact that the tag{serial.port}may be hidden inside another variable likeextra_paramsor similar.Some other cleanups in this PR:
DefaultExportAppHandlerandDefaultExportHandlerthat are basically 99% the same are now merged and renamed toUploadHandler.SerialUploader, now it should be much more consistent when running upload (w/ or w/out programmer) and burn bootloader.