-
-
Notifications
You must be signed in to change notification settings - Fork 397
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.
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
feat: simplify board and port handling #2165
Conversation
arduino-ide-extension/src/browser/boards/boards-service-provider.ts
Outdated
Show resolved
Hide resolved
arduino-ide-extension/src/browser/boards/boards-toolbar-item.tsx
Outdated
Show resolved
Hide resolved
arduino-ide-extension/src/browser/dialogs/certificate-uploader/select-board-components.tsx
Outdated
Show resolved
Hide resolved
I have reviewed the code changes in all files. I've not done any UA Testing yet. I'm aware some of my comments are in relation to old code that has been moved around, @kittaakos please feel free to determine what is out of scope for this PR. I've also asked some general questions above just to refresh my understanding, please don't feel these are urgent. Overall I do not see any major issues. Awesome job! It seems to strip out a good amount of complexity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: This has been resolved/superseded by the implementation of a new design where the list of identifications is no longer shown in the "Board Selector" menu. In the case of multiple identifications, the user is now presented with the "Select Other Board and Port" dialog after they select a port from the menu for which there are multiple identifications. The list of boards in that dialog uses the human friendly names, along with differentiator suffixes in case of a name collision, providing a user experience that is even superior to the adjustment I suggested here.
Use human platform name in multiple board identifications list
Multiple board definitions might use the same port identification properties. This PR adds an expandable list under the top level menu item for the port in the Board Selector menu in this case:
The list item text currently uses the machine readable platform identifier with the form <vendor ID>:<architecture ID>
. This identifier is something most Arduino IDE users will not be familiar with.
I think a better user experience would be provided by using the human friendly platform name, (e.g., "Arduino AVR Boards" instead of "arduino:avr"), which the user will already be familiar with from seeing it in other parts of the Arduino IDE UI (Boards Manager and the Tools > Board).
I believe the choice of the machine readable platform identifier was based on the reported use case of having a release version and a development version of a platform installed at the same time, as is done by platform developers (but very rarely by normal users). In this case, the human friendly platform name might not be very helpful since both copies of the platform might have the same name. A solution for this poor user experience has already been implemented for the Tools > Board menu, which is to differentiate the platform names by appending a (<vendor ID>)
suffix to the platform name where there is a name collision:
So the same differentiation approach can be done in the Board Selector menu.
To reproduce
In case it will be helpful to the developers or testers, I'll provide instructions for an easy (assuming you have a Leonardo on hand) way to produce this state of multiple board identifications.
Equipment
- Leonardo board
Steps
- Select File > Preferences... from the Arduino IDE menus.
- Type the following into the "Additional Boards Manager URLs" field:
https://raw.githubusercontent.com/MrBlinky/Arduboy-homemade-package/master/package_arduboy_homemade_index.json
- Click the "OK" button.
- Wait for the index download process to finish.
- Use Boards Manager to install the "Arduboy homemade package platform.
- Connect a Leonardo board to your computer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: Fixed by 58b16b4
Language server not started on application launch
When a sketch is opened via the previous session restoration system, or by file association/command line argument, the board associated with that sketch are automatically selected in Arduino IDE.
🐛 The language server is not started under these conditions.
To reproduce
- Select File > New Sketch from the Arduino IDE menus.
- Select Tools > Board > Arduino AVR Boards > Arduino Uno from the Arduino IDE menus.
- Select File > Save As..." from the Arduino IDE menus.
- Save the sketch to any location and name you like.
- Wait for the language server processing operation to finish, as indicated by the message at the left side of the status bar.
- Hover the mouse pointer over the
setup
in the sketch code.
🙂 An "Editor Hover" appears as expected, indicating the language server is running. - Select Sketch > Verify/Compile from the Arduino IDE menus.
🙂 Compilation finishes without any unexpected notifications. - Select File > Quit from the Arduino IDE menus.
- Start Arduino IDE.
🙂 The sketch you saved during the previous session opens in an IDE window.
🐛 Messages indicating language server processing never appear at the left side of the status bar - Hover the mouse pointer over the
setup
in the sketch code.
🐛 No "Editor Hover" appears. - Select Sketch > Verify/Compile from the Arduino IDE menus.
🐛 A notification appears:⚠ Language server is not running.
- Select Tools > Board > Arduino AVR Boards > Arduino Mega or Mega 2560 from the Arduino IDE menus.
🙂 Messages indicating language server processing appear at the left side of the status bar, as is expected to be triggered when a different board is selected. - Hover the mouse pointer over the
setup
in the sketch code.
🙂 An "Editor Hover" appears as expected, indicating the language server is running. - Select Sketch > Verify/Compile from the Arduino IDE menus.
🙂 Compilation finishes without any unexpected notifications.
Expected behavior
Language server is automatically started for the selected board whenever a sketch is opened, regardless of the mechanism through which it opens.
Arduino IDE version
927aebc (tester build for ef6c455)
Operating system
Windows 11
Additional context
I'm not able to reproduce the fault using the build from the main
branch (e17472e)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: Fixed by 6e5ae7f
Custom board option menus not created on application launch
When a sketch is opened via the previous session restoration system, or by file association/command line argument, the board associated with that sketch are automatically selected in Arduino IDE.
🐛 If the associated board has custom board options, menus are not created for them under the Tools menu.
To reproduce
- Select File > New Sketch from the Arduino IDE menus.
- Select Tools > Board > Arduino AVR Boards > Arduino Mega or Mega 2560 from the Arduino IDE menus.
- Open the Arduino IDE Tools menu.
🙂 A "Processor" custom board option submenu is present, as expected for this board. - Select File > Save As..." from the Arduino IDE menus.
- Save the sketch to any location and name you like.
- Select File > Quit from the Arduino IDE menus.
- Start Arduino IDE.
🙂 The sketch you saved during the previous session opens in an IDE window. - Open the Arduino IDE Tools menu.
🐛 There is no "Processor" custom board option submenu.
Expected behavior
The Tools menu is always populated with custom board menus for the selected board.
Arduino IDE version
927aebc (tester build for ef6c455)
Operating system
Windows 11
Additional context
I'm not able to reproduce the fault using the build from the main
branch (e17472e)
I suspect this is another symptom of the same bug that produced the "Language server not started on application launch" fault I reported in my previous review. I thought I should also report this fault to document it for the benefit of other testers, and because it might provide a lead on identifying the source of the bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: Fixed by 76d46d4
Updated port not selected when Serial Monitor open during upload
The updated port is not selected after an upload under the following conditions:
- The port produced by the board post-upload has a different address than pre-upload.
- Serial Monitor is open.
To reproduce
Equipment
- A board that can produce the conditions of an address change.
The easiest way to produce an address change is using a board that has a persistent bootloader activation, including:
Steps
- Press and release the reset button on the board twice quickly
- Select the board and port from the Arduino IDE menus.
- Select Sketch > Upload from the Arduino IDE menus.
- Wait for the upload to finish.
😕 if you have verbose upload output enabled or are monitoring the Arduino IDE logs, you will see that the IDE remains in the uploading state for some time even after the upload command invocation has returned successfully as it repeatedly attempts to connect the monitor to the port.
🐛 The current port of the board is not selected in Arduino IDE.
Expected behavior
The port selection is updated even when Serial Monitor is open.
Arduino IDE version
d82fa4f (tester build for 1731972)
Operating system
Windows 11
Additional context
Everything works as expected when uploading with the Serial Monitor open if the post-upload port is the same as pre-upload address.
The "Port monitor error: command 'open' failed: Serial port not found. Could not connect to COM33 serial port." error occurs even when using the build from main
(9a6a457) (and back to the introduction of the monitor error notification system in #1965). However, the correct port is selected and the monitor connected at the end of the process despite what the notification says when using that version of Arduino IDE. So this PR does result in a worsening of an already poor user experience.
I see this in the logs from an upload that produced a COM33 -> COM32 address change when the arduino.cli.daemon.debug
advanced setting is set to true
:
2023-08-14 07:58:08 2023-08-14T14:58:08.511Z daemon INFO 51 | RESP: {
51 | "Message": {
51 | "Result": {
51 | "updated_upload_port": {
51 | "address": "COM32",
51 | "label": "COM32",
51 | "protocol": "serial",
51 | "protocol_label": "Serial Port (USB)",
51 | "properties": {
51 | "pid": "0x804F",
51 | "serialNumber": "F0F2036051504C3750202020FF0F2520",
51 | "vid": "0x2341"
51 | },
51 | "hardware_id": "F0F2036051504C3750202020FF0F2520"
51 | }
51 | }
51 | }
51 | }
51 STREAM CLOSED
2023-08-14 07:58:08 2023-08-14T14:58:08.512Z root INFO Received port after upload [arduino+serial://COM33, arduino:samd:mkrzero, sketch_aug14b]. Before port: {"address":"COM33","addressLabel":"COM33","protocol":"serial","protocolLabel":"Serial Port (USB)","properties":{"pid":"0x004F","serialNumber":"","vid":"0x2341"}}, after port: {"address":"COM32","addressLabel":"COM32","protocol":"serial","protocolLabel":"Serial Port (USB)","properties":{"pid":"0x804F","serialNumber":"F0F2036051504C3750202020FF0F2520","vid":"0x2341"},"hardwareId":"F0F2036051504C3750202020FF0F2520"}
So Arduino CLI is correctly identifying the updated port address as COM32, but Arduino IDE is not selecting it.
Thanks for the great review! Please try the latest version. Although there are no new commits after your review, I did not give you a chance to test the latest. Please take a look at the explanation below. I can confirm this. Unfortunately, I forgot to push to remote. Although I could reproduce the steps on macOS neither with MKR1000 nor with Nano 33 BLE, I could on Windows with
I have noticed the issue on macOS with Uno R4 WiFi (using arduino/ArduinoCore-renesas#73) and fixed it but forgot to push. I can confirm the So the latest version has the fix; it's only my MKR1000 on this Windows machine. Otherwise, IDE2 would show the |
e54eece
to
303e4b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: fixed by b3b731a
Potentially misleading board selector label
The "board selector" menu on the Arduino IDE toolbar lists the discovered ports, and in cases where the identification properties of that port are associated with a board definition, the board name. The user can select both a board and port at the same time.
Prior to the changes made in this pull request, the menu was populated with items of two distinct classes:
Identification? | Menu item label | Action triggered by selection |
---|---|---|
yes | <identification name> | Select port and identified board |
no | "Unknown" | Open "Select Other Board and Port" dialog, populated w/ list of all boards |
The system was redesigned in this pull request to provide an enhanced user experience in cases where a port was identified with multiple board definitions, resulting in there now being four distinct classes menu item:
# identifications | Identifications have different names? |
Menu item label | Action triggered by selection |
---|---|---|---|
1 | N/A | <identification name> | Select port and identified board |
>1 | no | <identification name> | Open "Select Other Board and Port" dialog, populated w/ list of identifications |
>1 | yes | "Unconfirmed board" | Open "Select Other Board and Port" dialog, populated w/ list of identifications |
0 | N/A | "Unknown board" | Open "Select Other Board and Port" dialog, populated w/ list of all boards |
There was an unexpected change in the menu label of the last class of menu item from "Unknown" to "Unknown Board"
🐛 The new menu label will be misleading to users in cases where the port is produced by something other than an Arduino board (e.g., the "COM1" port commonly generated by the internal serial port of the PC motherboard). Failed uploads caused by selecting a incidental port that isn't actually the board being targeted is a common source of confusion for new users.
To reproduce
- If your computer doesn't already produce such a port, connect any device that will produce a port that is not identified as an Arduino board.
- Open the board selector menu.
🐛 The port is labeled "Unknown board", even though Arduino IDE can't know whether it is a board:
Expected behavior
The menu label of this class of port is "Unknown".
Arduino IDE version
d1bf84a (tester build for 530cddb)
Additional context
The port has the expected label when using the build from the main
branch (9a6a457):
Signed-off-by: Akos Kitta <[email protected]>
Ref: arduino/arduino-cli@38479dc Signed-off-by: Akos Kitta <[email protected]>
Thank you for the report. It should work now.
The latest version of this PR is using the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is working well for me now.
Thanks Akos!
If it is set before the board+port settings are restored from the `localStorage`, extensions will see no board+port. Ref: #2165 Ref: dankeboy36/esp-exception-decoder#10 Signed-off-by: Akos Kitta <[email protected]>
If it is set before the board+port settings are restored from the `localStorage`, extensions will see no board+port. Ref: #2165 Ref: dankeboy36/esp-exception-decoder#10 Signed-off-by: Akos Kitta <[email protected]>
Ref: #2165 Closes #2230 Signed-off-by: Akos Kitta <[email protected]>
If it is set before the board+port settings are restored from the `localStorage`, extensions will see no board+port. Ref: #2165 Ref: dankeboy36/esp-exception-decoder#10 Signed-off-by: Akos Kitta <[email protected]>
Ref: #2165 Closes #2230 Signed-off-by: Akos Kitta <[email protected]>
🚧 This PR depends on feature: Detect board port change after upload (arduino/arduino-cli#2253).✅See the Beta Testing Guide for the tester builds.
Motivation
The rewrite of the board and port handling in IDE2. The basic idea is to avoid trying to do port change detection on the IDE2 side (frontend) but rely on the CLI's new API: arduino/arduino-cli#2253.
Other information
Closes #43
Closes #82
Closes #1319
Closes #1366
Closes #2143
Closes #2158
This PR is the replacement of #2149. It's 100% percent the same code, but the PR has been submitted from the
arduino
owner. Hence, the application signing will run on the CIs.Reviewer checklist