Skip to content
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

fix 180: prevent erroneous "auto-reconnect"(s) in board selector #1328

Merged
merged 12 commits into from
Aug 24, 2022

Conversation

davegarthsimpson
Copy link
Contributor

@davegarthsimpson davegarthsimpson commented Aug 16, 2022

Motivation

Users reported erroneous auto-selection of ports, in relation to uploads, unknown boards and using multiple boards of the same fqbn.

Issue Identified

We were on occasion erroneously auto-reconnecting to the wrong port with the same associated board after upload / on disconnect. We were failing to check if the port we were auto-connecting to actually appeared due to an upload.

Change description

Checks if a new port is created by an upload. Knowing if we have an "upload created port," we can now correctly determine if we should auto-reconnect after an upload.

Other information

This is a draft serving to illustrate where this issue resides and a potential solution, these changes should be unit tested prior to merge, and we may wish to centralise the new logic.

This PR also resolves the issue of "double selections" in the board selector dropdown with an additional check here.

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

prevent auto-select of unattached boards Co-authored-by: Francesco Spissu <francescospissu@users.noreply.github.com>
@davegarthsimpson davegarthsimpson changed the title fix 180: prevent erroneous "auto-reconnect"(s) in board selector fix: 180: prevent erroneous "auto-reconnect"(s) in board selector Aug 16, 2022
@davegarthsimpson davegarthsimpson marked this pull request as draft August 16, 2022 13:02
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Aug 16, 2022
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

This fixes the issues such as #180 and #710 for me.

I did notice that it also results in the loss of a useful capability. I report that below because I think it should be documented and tracked, but I think that the PR still results in a net benefit to the users of the IDE so I am still in favor of this proposal.

Describe the problem

The upload process Arduino boards with native USB capability normally works like this:

  1. User triggers an Upload operation.
  2. Arduino CLI sends a signal to the board via the CDC serial port produced by the sketch application.
  3. Board exits sketch application.
    The CDC serial port produced by the sketch application disappears.
  4. Board starts bootloader application.
    The bootloader application produces a CDC serial port.
  5. Arduino CLI detects the new port.
  6. Arduino CLI starts upload process to the bootloader application's port.
  7. Upload process finishes.
  8. Board exits bootloader application.
    The CDC serial port produced by the bootloader application disappears.
  9. Board starts sketch application.
    The sketch application produces a CDC serial port.

On Windows (I don't know whether it ever happens on other operating systems), the sketch application and bootloader application are assigned different port numbers, meaning the port number changes twice during this process and Arduino CLI/Arduino IDE must follow it through these changes.

In cases where the sketch program does not produce a CDC serial port (which may happen by design or because of a bug in the sketch code), users of Arduino boards with native USB capability must manually activate the bootloader before uploading, then upload to the bootloader application's serial port. This means the port will only change once at the end of the upload process during the switch from the bootloader application's port to the sketch application's port.

🐛 Port selection is lost if the board has a different port after completion of the Upload process.

The user will have to manually select the port again in order to use Serial Monitor/Plotter, or upload to the board.

To reproduce

Equipment

  • Modern native USB board. For example:
    • Any MKR Board
    • Nano 33 IoT
    • Nano 33 BLE
    • Portenta H7
    • Sparkfun Pro Micro

Even though they have the native USB capability, the Leonardo and Micro boards have a more primitive bootloader which does not support the double reset technique employed in the instructions below.

Steps

  1. Select File > New from the Arduino IDE menus.
  2. Connect the Arduino board to your computer.
  3. Press and release the reset button on the board twice quickly.
    You should now see the on board LED pulsing to indicate the bootloader is running.
  4. Select the board and port in the Arduino IDE from the "Board Selector" or menus under Tools.
  5. Select Sketch > Upload from the Arduino IDE menus.
  6. Wait for the upload to finish.
  7. Open the "Board Selector" or Tools > Port menu.

🐛 The board's port is not selected.

Expected behavior

The port of the board is always selected after an upload.

Arduino IDE version

2.0.0-rc9.2.snapshot-6222efb

Operating system

Windows

Operating system version

10

Additional context

Arduino IDE 1.x and 2.0.0-rc9.2.snapshot-de32bdd select the correct port after an upload even under these conditions.


I know that it is possible for the sketch application port number to be different after an upload than it was before (e.g., COM7 -> COM5 -> COM9). This used to happen to me especially often when using the Nano 33 BLE and I saw lots of reports of that from other users. I don't know exactly what the conditions are that occurred under, nor whether a fix was made in the core or IDE since then (I don't get the chance to use hardware as much these days), but I was not able to produce that behavior during my quick tests. I suspect that the IDE would have the same problem following the port under those conditions as it does under those produced by the double reset technique.


My opinion is that the correct place to implement this port following capability is in Arduino CLI, which already has it to follow the port change at the start of the upload process. The IDE would then simply select the port specified by Arduino CLI via the gRPC response at the completion of the upload.

@davegarthsimpson
Copy link
Contributor Author

davegarthsimpson commented Aug 22, 2022

@per1234

I suspect that the IDE would have the same problem following the port under those conditions as it does under those produced by the double reset technique.

Theoretically this should not be an issue with this fix, the code should deal with things as expected provided the port changes were triggered after a user clicks upload.

The specific reason this PR doesn't work when a manual "reset-double-press" occurs is because we are only registering ports that "disappear" during an upload, and subsequently looking to reconcile once said upload is complete.

I'm not sure about the feasibility of identifying when a port "disappears" specifically due to the "reset-double-press", to the IDE the event could also be a user unplugging a board for example. We can also look to identify ports that disappear exactly after the upload finished (bootloader created ports), but I think this adds another layer of complexity that may not be optimum for the codebase.

I would agree that we should also review what can be addressed CLI side. @cmaglie what do you think?

@davegarthsimpson davegarthsimpson marked this pull request as ready for review August 22, 2022 14:08
@davegarthsimpson davegarthsimpson changed the title fix: 180: prevent erroneous "auto-reconnect"(s) in board selector fix 180: prevent erroneous "auto-reconnect"(s) in board selector Aug 22, 2022
@davegarthsimpson davegarthsimpson marked this pull request as draft August 22, 2022 16:43
@davegarthsimpson davegarthsimpson marked this pull request as ready for review August 23, 2022 13:33
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I tried it manually with several boards and sketches, and the UX was much better than the state from the main.

arduino-ide-extension/src/node/core-service-impl.ts Outdated Show resolved Hide resolved
arduino-ide-extension/src/node/board-discovery.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

The issue I reported in my previous review no longer occurs. I verified that this fixes #180 #287 and #710

It is really a great improvement to the experience of uploading.

Thanks for your excellent work Dave!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
3 participants