feature: Detect board port change after upload #2253
Conversation
cmaglie
added
priority: high
- update firmware uploader to `2.3.0`, - new gRPC API Signed-off-by: Akos Kitta <[email protected]>
cmaglie
force-pushed
the
board_port_after_upload
branch
from
6a02899
to
2d5ec7f
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2253 +/- ##
========================================
Coverage 62.94% 62.94%
========================================
Files 220 221 +1
Lines 19540 19724 +184
========================================
+ Hits 12299 12415 +116
- Misses 6151 6207 +56
- Partials 1090 1102 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Arduino CLI should always return the port after an upload, even in the case where no port change is expected. The consumer shouldn't be required to implement "if not updated_upload_port, use original port" logic.
The whole point is that all the logic for determining which port should be selected after an upload should be implemented in Arduino CLI. The consumer should be able to simply select the port Arduino CLI tells it to select in all cases.
Arduino CLI should always return the port after an upload, even in the case where no port change is expected. The consumer shouldn't be required to implement "if not updated_upload_port, use original port" logic. The whole point is that all the logic for determining which port should be selected after an upload should be implemented in Arduino CLI. The consumer should be able to simply select the port Arduino CLI tells it to select in all cases.
cmaglie
force-pushed
the
board_port_after_upload
branch
from
6b80bca
to
bfeefb5
Compare
cmaglie
requested review from
per1234,
alessio-perugini,
umbynos and
MatteoPologruto
cmaglie
force-pushed
the
board_port_after_upload
branch
from
bfeefb5
to
1b60c97
Compare
cmaglie
force-pushed
the
board_port_after_upload
branch
from
1b60c97
to
990d93a
Compare
I've implemented the behavior above. |
|
I received specific steps to force the port change with Uno R4 WiFi. In this case, the
Steps: ./arduino-cli version
arduino-cli Version: git-snapshot Commit: 990d93a5 Date: 2023-08-10T14:51:50Z./arduino-cli board list --format json[
{
"port": {
"address": "/dev/cu.BLTH",
"label": "/dev/cu.BLTH",
"protocol": "serial",
"protocol_label": "Serial Port"
}
},
{
"port": {
"address": "/dev/cu.Bluetooth-Incoming-Port",
"label": "/dev/cu.Bluetooth-Incoming-Port",
"protocol": "serial",
"protocol_label": "Serial Port"
}
},
{
"matching_boards": [
{
"name": "Arduino UNO R4 WiFi",
"fqbn": "arduino:renesas_uno:unor4wifi"
}
],
"port": {
"address": "/dev/cu.usbmodemDC5475C56BE02",
"label": "/dev/cu.usbmodemDC5475C56BE02",
"protocol": "serial",
"protocol_label": "Serial Port (USB)",
"properties": {
"pid": "0x1002",
"serialNumber": "DC5475C56BE0",
"vid": "0x2341"
},
"hardware_id": "DC5475C56BE0"
}
}
]cat ~/Documents/Arduino/r4/r4.ino
#include <HID.h>
void setup() {}
void loop() {}% ./arduino-cli upload -b arduino:renesas_uno:unor4wifi -p /dev/cu.usbmodemDC5475C56BE02 ~/Documents/Arduino/r4 --format json{
"stdout": "Erase flash\n\nDone in 0.001 seconds\nWrite 46784 bytes to flash (12 pages)\n\r[ ] 0% (0/12 pages)\r[== ] 8% (1/12 pages)\r[===== ] 16% (2/12 pages)\r[======= ] 25% (3/12 pages)\r[========== ] 33% (4/12 pages)\r[============ ] 41% (5/12 pages)\r[=============== ] 50% (6/12 pages)\r[================= ] 58% (7/12 pages)\r[==================== ] 66% (8/12 pages)\r[====================== ] 75% (9/12 pages)\r[========================= ] 83% (10/12 pages)\r[=========================== ] 91% (11/12 pages)\r[==============================] 100% (12/12 pages)\nDone in 2.968 seconds\n",
"stderr": "",
"updated_upload_port": {
"address": "/dev/cu.usbmodemDC5475C56BE02",
"label": "/dev/cu.usbmodemDC5475C56BE02",
"protocol": "serial",
"protocol_label": "Serial Port (USB)",
"properties": {
"pid": "0x1002",
"serialNumber": "DC5475C56BE0",
"vid": "0x2341"
},
"hardware_id": "DC5475C56BE0"
}
}The ./arduino-cli board list --format json[
{
"port": {
"address": "/dev/cu.BLTH",
"label": "/dev/cu.BLTH",
"protocol": "serial",
"protocol_label": "Serial Port"
}
},
{
"port": {
"address": "/dev/cu.Bluetooth-Incoming-Port",
"label": "/dev/cu.Bluetooth-Incoming-Port",
"protocol": "serial",
"protocol_label": "Serial Port"
}
},
{
"matching_boards": [
{
"name": "Arduino UNO R4 WiFi",
"fqbn": "arduino:renesas_uno:unor4wifi"
}
],
"port": {
"address": "/dev/cu.usbmodem14201",
"label": "/dev/cu.usbmodem14201",
"protocol": "serial",
"protocol_label": "Serial Port (USB)",
"properties": {
"pid": "0x006D",
"serialNumber": "3507110A35323634DB5233324B572D0A",
"vid": "0x2341"
},
"hardware_id": "3507110A35323634DB5233324B572D0A"
}
}
]CLI version: 990d93a I don't know whether this PR should cover this use case. Can you please help, @per1234? Thank you! |
|
I am using d1869b6, and after manually running ~20 consecutive uploads, I no longer see the bug. ✅
./arduino-cli upload -b arduino:samd:mkr1000 -p /dev/cu.usbmodem14101 ~/Documents/Arduino/monitor3 --format json{
"stdout": "Atmel SMART device 0x10010005 found\nDevice : ATSAMD21G18A\nChip ID : 10010005\nVersion : v2.0 [Arduino:XYZ] Dec 20 2016 15:36:43\nAddress : 8192\nPages : 3968\nPage Size : 64 bytes\nTotal Size : 248KB\nPlanes : 1\nLock Regions : 16\nLocked : none\nSecurity : false\nBoot Flash : true\nBOD : true\nBOR : true\nArduino : FAST_CHIP_ERASE\nArduino : FAST_MULTI_PAGE_WRITE\nArduino : CAN_CHECKSUM_MEMORY_BUFFER\nErase flash\ndone in 0.700 seconds\n\nWrite 11344 bytes to flash (178 pages)\n\r[========== ] 35% (64/178 pages)\r[===================== ] 71% (128/178 pages)\r[==============================] 100% (178/178 pages)\ndone in 0.074 seconds\n\nVerify 11344 bytes of flash with checksum.\nVerify successful\ndone in 0.010 seconds\nCPU reset.\n",
"stderr": "",
"updated_upload_port": {
"address": "/dev/tty.usbmodem14101",
"label": "/dev/cu.usbmodem14101",
"protocol": "serial",
"protocol_label": "Serial Port (USB)",
"properties": {
"pid": "0x804E",
"serialNumber": "94A3397C5150435437202020FF150838",
"vid": "0x2341"
},
"hardware_id": "94A3397C5150435437202020FF150838"
}
}
./arduino-cli board list --format json[
{
"port": {
"address": "/dev/cu.BLTH",
"label": "/dev/cu.BLTH",
"protocol": "serial",
"protocol_label": "Serial Port"
}
},
{
"port": {
"address": "/dev/cu.Bluetooth-Incoming-Port",
"label": "/dev/cu.Bluetooth-Incoming-Port",
"protocol": "serial",
"protocol_label": "Serial Port"
}
},
{
"matching_boards": [
{
"name": "Arduino MKR1000",
"fqbn": "arduino:samd:mkr1000"
}
],
"port": {
"address": "/dev/cu.usbmodem14101",
"label": "/dev/cu.usbmodem14101",
"protocol": "serial",
"protocol_label": "Serial Port (USB)",
"properties": {
"pid": "0x804E",
"serialNumber": "94A3397C5150435437202020FF150838",
"vid": "0x2341"
},
"hardware_id": "94A3397C5150435437202020FF150838"
}
}
]
|
Hi @kittaakos. I apologize for the confusion. I intended to circle back to our Slack convo about this after I had investigated but hadn't found the time. I provided these instructions purely as a response to the request for demonstrations of an upload causing a port change. I hadn't looked into how the system implemented in this PR would work at that time so I didn't realize that there were some unique considerations for this particular demonstration. A port change takes some time for the operating system to de-enumerate the old port and then enumerate the new port. So Arduino CLI's system for detecting a port change requires it to wait some time while watching for the appearance of a new port, timing out and falling back to using the original port if the new port doesn't appear within the timeout duration. Some boards (e.g., classic UNO, Mega, Nano Every) will never experience a port change on upload. Since it would be inefficient for Arduino CLI to wait around for a port change in this case it is possible for the platform author to configure the board definition to skip the wait step by setting the So for boards with this arduino-cli/commands/upload/upload.go Lines 401 to 402 in 990d93a There is an unusual situation with the UNO R4 WiFi where, where the average sketch will not produce a port change due to the board having a dedicated USB "bridge" module, but if the sketch uses USB HID capabilities (which, in a innovative approach, are provided by the "bridge" chip) then the port change does occur. The board definition of the UNO R4 WiFi sets the https://github.com/arduino/ArduinoCore-renesas/blob/1.0.2/boards.txt#L138 This is arguably incorrect, but setting it to arduino/ArduinoCore-renesas#74 (comment) The mitigation measure for the existing poor user experience has been to document the special procedures that are required when using the board's HID capabilities (arduino/docs-content#1246, arduino/help-center-content#258). I'll have to add the requirement of manually selecting the post-upload port to that documentation once we ship arduino/arduino-ide#2165 It is possible to adjust the configuration of the UNO R4 WiFi board definition to simulate a port change on a board that is correctly configured for such a thing:
Unfortunately I find that Arduino CLI is still unable to correctly identify the post-upload port. I'm not sure why. I see the port is seen, but has a different |
I am also able to reproduce this. I see that the |
Previously only the pointer was copied, thus making changes in
`actualPort` to be reflected also to `port`. This lead to some weird
result in the `updatedUploadPort` result:
{
"stdout": "Verify 11344 bytes of flash with checksum.\nVerify successful\ndone in 0.010 seconds\nCPU reset.\n",
"stderr": "",
"updated_upload_port": {
"address": "/dev/tty.usbmodem14101", <------- this address...
"label": "/dev/cu.usbmodem14101", <------- ...is different from the label
"protocol": "serial",
"protocol_label": "Serial Port (USB)",
"properties": {
"pid": "0x804E",
"serialNumber": "94A3397C5150435437202020FF150838",
"vid": "0x2341"
},
"hardware_id": "94A3397C5150435437202020FF150838"
}
}
I've pushed two commits that should fix this problem. |
cmaglie
force-pushed
the
board_port_after_upload
branch
from
1d3e528
to
af873e0
Compare
We must acesss the gRPC API only until we cross the `command` package border. Once we are inside the `command` package we should use the internal API only.
Now the upload detects cases when the upload port is "unstable", i.e. the port changes even if it shouldn't (because the wait_for_upload_port property in boards.txt is set to false). This change should make the upload process more resilient.
|
I've implemented a workaround for the weird port change on the Uno R4, so the detection now work more or less like this:
|
There was a problem hiding this comment.
UPDATE: Fixed by e12a068
Panic when upload causes disappearance of port
On boards with native USB, the USB stack that produces the USB CDC serial port used for uploading is running on the same microcontroller as the sketch program. This means that, when working with these board, it is common for sketch uploads to cause the disappearance of the port, either unexpectedly due to a bug such as a divide by zero or expectedly such as when putting the MCU to sleep.
🐛 If a sketch upload causes the disappearance of the port, Arduino CLI panics.
To reproduce
Equipment
- Board with native USB capability, such as:
Steps
Upload a sketch that causes the loss of the port of your board:
$ ./arduino-cli version
arduino-cli.exe Version: git-snapshot Commit: e9e5fbdd Date: 2023-08-16T03:41:25Z
$ mkdir "/tmp/NoInterruptsSketch"
$ printf "void setup() {noInterrupts();}\nvoid loop() {}\n" > "/tmp/NoInterruptsSketch/NoInterruptsSketch.ino"
$ ./arduino-cli compile --fqbn arduino:avr:leonardo
[...]
$ ./arduino-cli upload --fqbn arduino:avr:leonardo --port COM23 --verbose "/tmp/NoInterruptsSketch"
Performing 1200-bps touch reset on serial port COM23
Waiting for upload port...
Upload port found on COM24
"C:\Users\per\AppData\Local\Arduino15\packages\arduino\tools\avrdude\6.3.0-arduino17/bin/avrdude" "-CC:\Users\per\AppData\Local\Arduino15\packages\arduino\tools\avrdude\6.3.0-arduino17/etc/avrdude.conf" -v -V -patmega32u4 -cavr109 "-PCOM24" -b57600 -D "-Uflash:w:C:\Users\per\AppData\Local\Temp\arduino\sketches\892D5E2A267501D13FBB3246DF62F6BE/NoInterruptsSketch.ino.hex:i"
avrdude: Version 6.3-20190619
Copyright (c) 2000-2005 Brian Dean, http://www.bdmicro.com/
Copyright (c) 2007-2014 Joerg Wunsch
System wide configuration file is "C:\Users\per\AppData\Local\Arduino15\packages\arduino\tools\avrdude\6.3.0-arduino17/etc/avrdude.conf"
Using Port : COM24
Using Programmer : avr109
Overriding Baud Rate : 57600
AVR Part : ATmega32U4
Chip Erase delay : 9000 us
PAGEL : PD7
BS2 : PA0
RESET disposition : dedicated
RETRY pulse : SCK
serial program mode : yes
parallel program mode : yes
Timeout : 200
StabDelay : 100
CmdexeDelay : 25
SyncLoops : 32
ByteDelay : 0
PollIndex : 3
PollValue : 0x53
Memory Detail :
Block Poll Page Polled
Memory Type Mode Delay Size Indx Paged Size Size #Pages MinW MaxW ReadBack
----------- ---- ----- ----- ---- ------ ------ ---- ------ ----- ----- ---------
eeprom 65 20 4 0 no 1024 4 0 9000 9000 0x00 0x00
flash 65 6 128 0 yes 32768 128 256 4500 4500 0x00 0x00
lfuse 0 0 0 0 no 1 0 0 9000 9000 0x00 0x00
hfuse 0 0 0 0 no 1 0 0 9000 9000 0x00 0x00
efuse 0 0 0 0 no 1 0 0 9000 9000 0x00 0x00
lock 0 0 0 0 no 1 0 0 9000 9000 0x00 0x00
calibration 0 0 0 0 no 1 0 0 0 0 0x00 0x00
signature 0 0 0 0 no 3 0 0 0 0 0x00 0x00
Programmer Type : butterfly
Description : Atmel AppNote AVR109 Boot Loader
Connecting to programmer: .
Found programmer: Id = "CATERIN"; type = S
Software Version = 1.0; No Hardware Version given.
Programmer supports auto addr increment.
Programmer supports buffered memory access with buffersize=128 bytes.
Programmer supports the following devices:
Device code: 0x44
avrdude: devcode selected: 0x44
avrdude: AVR device initialized and ready to accept instructions
Reading | ################################################## | 100% 0.00s
avrdude: Device signature = 0x1e9587 (probably m32u4)
avrdude: reading input file "C:\Users\per\AppData\Local\Temp\arduino\sketches\892D5E2A267501D13FBB3246DF62F6BE/NoInterruptsSketch.ino.hex"
avrdude: writing flash (3464 bytes):
Writing | ################################################## | 100% 0.27s
avrdude: 3464 bytes of flash written
avrdude done. Thank you.
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x50 pc=0xe60dea]
goroutine 1 [running]:
github.com/arduino/arduino-cli/arduino/discovery.(*Port).ToRPC(...)
E:/git/arduino-cli/arduino/discovery/discovery.go:108
github.com/arduino/arduino-cli/commands/upload.runProgramAction(0xc000c6a700, 0x65d6de?, {0x0, 0x0}, {0x0, 0x0}, {0xc00011a060, 0x14}, 0x1a321a0?, {0x0, ...}, ...)
E:/git/arduino-cli/commands/upload/upload.go:515 +0x54ca
github.com/arduino/arduino-cli/commands/upload.Upload({0x19d9060?, 0x0?}, 0xc00010c000, {0x13a2a60, 0xc0001151a0}, {0x13a2a60, 0xc0001151b8})
E:/git/arduino-cli/commands/upload/upload.go:146 +0x468
github.com/arduino/arduino-cli/internal/cli/upload.runUploadCommand(0xc00038c280?, {0xc000170cc0, 0x1, 0x6?})
E:/git/arduino-cli/internal/cli/upload/upload.go:171 +0xbba
github.com/spf13/cobra.(*Command).execute(0xc00038c280, {0xc000170c60, 0x6, 0x6})
C:/Users/per/go/pkg/mod/github.com/spf13/[email protected]/command.go:860 +0x663
github.com/spf13/cobra.(*Command).ExecuteC(0xc0002c0500)
C:/Users/per/go/pkg/mod/github.com/spf13/[email protected]/command.go:974 +0x3bd
github.com/spf13/cobra.(*Command).Execute(0x0?)
C:/Users/per/go/pkg/mod/github.com/spf13/[email protected]/command.go:902 +0x19
main.main()
E:/git/arduino-cli/main.go:31 +0xea
🐛 The process panicked.
Expected behavior
Arduino CLI handles the disappearance of the board's port after an upload gracefully.
Arduino CLI version
Operating system
Windows 11
Additional context
I bisected the regression to e9e5fbd (does not occur at the previous commit f8394fb).
Recovering the board from the no port state
If you performed the demonstration I provided above, the board will be in a state where it can't be uploaded to as usual. It can be recovered by performing the following procedure:
- Prepare a sketch that doesn't contain any problematic code:
$ arduino-cli sketch new "/tmp/BareMinimum" - Invoke an
arduino-cli uploadcommand for the sketch without a--portflag.
For example:$ arduino-cli upload --fqbn arduino:avr:leonardo --verbose "/tmp/BareMinimum" - Watch the command output until you see the following line:
Waiting for upload port... - Press and release the reset button on the board twice quickly to activate the bootloader.
The upload should now finish successfully. After that the board will produce a port once more and you can go back to uploading via the standard procedure.
There was a problem hiding this comment.
UPDATE: Fixed by 862bbe2
updated_upload_port may be different than upload target port when uploading to boards that produce multiple ports
Some Arduino boards produce multiple ports. The algorithm for identifying updated_upload_port selects the first of these to appear after an upload.
🐛 This port may be different from the one the user specified via the upload request, which will result in the updated_upload_port value being different than expected.
To reproduce
Equipment
- Board that produces multiple ports, such as:
- Nano ESP32
- Teensy board (when running a sketch that was compiled with the
usbcustom board option set to one of the "serial" options)
Demo
$ ./arduino-cli version
arduino-cli.exe Version: git-snapshot Commit: e9e5fbdd Date: 2023-08-16T03:41:25Z
$ ./arduino-cli sketch new /tmp/BareMinimum
[...]
$ ./arduino-cli board list
Port Protocol Type Board Name FQBN Core
1-7.1.4.1 dfu USB DFU Arduino Nano ESP32 arduino:esp32:nano_nora arduino:esp32
COM11 serial Serial Port (USB) Arduino Nano ESP32 arduino:esp32:nano_nora arduino:esp32
$ ./arduino-cli upload --fqbn arduino:esp32:nano_nora --protocol dfu --port 1-7.1.4.1 PERTILLISCH/sketches/BareMinimum/
dfu-util 0.11-arduino4
Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/
Opening DFU capable USB device...
Device ID 2341:0070
Device DFU version 0101
Claiming USB DFU Interface...
Setting Alternate Interface #0 ...
Determining device status...
DFU state(2) = dfuIDLE, status(0) = No error condition is present
DFU mode device DFU version 0101
Device returned transfer size 4096
Copying data from PC to DFU device
Download [=========================] 100% 286064 bytes
Download done.
DFU state(7) = dfuMANIFEST, status(0) = No error condition is present
DFU state(2) = dfuIDLE, status(0) = No error condition is present
Done!
New upload port: COM11 (serial)
$ ./arduino-cli board list
Port Protocol Type Board Name FQBN Core
1-7.1.4.1 dfu USB DFU Arduino Nano ESP32 arduino:esp32:nano_nora arduino:esp32
COM11 serial Serial Port (USB) Arduino Nano ESP32 arduino:esp32:nano_nora arduino:esp32
🐛 updated_upload_port was COM11 even though 1-7.1.4.1 was specified via the --port flag.
Expected behavior
updated_upload_port is the same (or equivalent) to the target port when such a port is available after upload.
Any additional ports produced by the board are only selected for updated_upload_port if there is no closer port match.
Arduino CLI version
Operating system
- Windows
- Linux
- macOS
Operating system version
- Windows 11
- Ubuntu 22.04
- macOS Ventura
Additional context
Admittedly the demonstration I provided of a 1-7.1.4.1 -> COM11 port change when uploading to Nano ESP32 is not very compelling. The user of this specific board would typically be better off with the serial protocol port selected when available. However, unexpected port selection changes in Arduino IDE when using Nano ESP32 was one of the faults that were specifically targeted for resolution by this work. I don't know how Arduino CLI behaves in this situation, but the direction of the port change fault as produced by the Arduino IDE port selection system was nondeterministic, meaning that some users experienced an unexpected change from the serial to the dfu protocol port, which caused Serial Monitor to no longer function (since there is no pluggable monitor for dfu protocol).
Even if the direction of the change is deterministically to the serial protocol port, for other boards this might be harmful. For example, the teensy protocol port is preferred over the serial port when using a Teensy board and Arduino CLI returns the serial port after uploading to the teensy port.
Co-authored-by: per1234 <[email protected]>
Great catch, I've just pushed a commit to fix this! |
The new algorithm takes into account the case where a single board may expose multiple ports, in this case the selection will increase priority to ports that: 1. have the same HW id as the user specified port for upload 2. have the same protocol as the user specified port for upload 3. have the same address as the user specified port for upload
Ok, I've pushed also a patch for this issue, the detection algorithm now prioritizes ports having (in order of importance):
BTW to allow this selection I needed to introduce a delay to allow the discoveries to detect the set of available ports and report them to the watcher. The initial delay is 5 seconds and it may be cut to 1 sec if a port with matching HW ID is found. From my preliminary tests, it seems to work fine but I'm not 100% sure about this last point (reducing the timeout to 1 sec), so more testing is very welcome. About testing, I'd like to have feedback also:
For example here is the trace I'm interested in: |
per1234
dismissed
their stale review
Previous reviews have been resolved. Thanks!
Please check if the PR fulfills these requirements
See how to contribute
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)configuration.schema.jsonupdated if new parameters are added.What kind of change does this PR introduce?
The CLI is now able to detect the port where a board re-attaches after an upload (this is particularly useful for boards with native USB connections that require disconnection of the communication port to perform the sketch upload).
Previously this task was performed by the Arduino IDE with mixed results, for more info see #2245.
What is the current behavior?
After an upload if the board change port this is not detected.
What is the new behavior?
The same information is available through JSON output too:
Does this PR introduce a breaking change, and is titled accordingly?
Yes
Other information
Fix #2245