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.

Sign up for GitHub

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

Jump to bottom

Auto-select boards config with hardwareId if present #1885

Closed
wants to merge 13 commits into from
Closed

Auto-select boards config with hardwareId if present #1885

wants to merge 13 commits into from

Conversation

davegarthsimpson
Copy link
Contributor

@davegarthsimpson davegarthsimpson commented Feb 13, 2023 •
edited
Loading

Motivation

To ensure boards not exposing an fqbn can still be "auto-selected", considering their hardwareId.

Change description

Retrieves hardwareId of a discovered port from the CLI.

Considers hardwareId of a discovered port when determining whether or not to "auto-select" a port.

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)

@davegarthsimpson davegarthsimpson changed the title Auto-select boards config with hardwareId if present Auto-select boards config with hardwareId if present Feb 13, 2023
per1234
per1234 requested changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@per1234 per1234 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

UPDATE: Resolved by cf6f83c

This must be updated to `1.19`:

arduino-ide/.github/workflows/build.yml

Line 32 in 08203d8

GO_VERSION: "1.17"

Reference: arduino/arduino-cli#2026

@per1234
Copy link
Contributor

per1234 commented Feb 22, 2023 •
edited
Loading

UPDATE: Resolved by cf6f83c

And the same change in the other workflows as well:

arduino-ide/.github/workflows/check-i18n-task.yml

Line 5 in 08203d8

GO_VERSION: "1.17"

arduino-ide/.github/workflows/i18n-nightly-push.yml

Line 5 in 08203d8

GO_VERSION: "1.17"

arduino-ide/.github/workflows/i18n-weekly-pull.yml

Line 5 in 08203d8

GO_VERSION: "1.17"

arduino-ide/.github/workflows/themes-weekly-pull.yml

Line 11 in 08203d8

GO_VERSION: "1.17"

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Feb 22, 2023
kittaakos
kittaakos suggested changes Feb 22, 2023
View reviewed changes
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.

IDE2 should not use the HEAD of the master branch.

arduino-ide-extension/package.json Outdated Show resolved Hide resolved
arduino-ide-extension/src/common/protocol/boards-service.ts Outdated Show resolved Hide resolved
arduino-ide-extension/src/common/protocol/boards-service.ts Outdated Show resolved Hide resolved
@per1234 per1234 self-requested a review February 22, 2023 10:44
@kittaakos
Copy link
Contributor

The build is failing due to a breaking change in the CLI. See #1809 (comment) for details.

@davegarthsimpson
Copy link
Contributor Author

The build is failing due to a breaking change in the CLI. See #1809 (comment) for details.

thanks for the heads up! @kittaakos can we resolve this with just a change of this line?

@kittaakos
Copy link
Contributor

can we resolve this with just a change of this line?

Correct, IDE2 needs to adapt there. 👍

@davegarthsimpson davegarthsimpson marked this pull request as ready for review February 22, 2023 15:19
@kittaakos kittaakos self-requested a review February 22, 2023 15:23
@ubidefeo
Copy link

@davegarthsimpson
can you take care of the 2 changes requested?

kittaakos
kittaakos previously approved these changes Feb 22, 2023
View reviewed changes
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.

The changes look good to me. I did not verify it.

@davegarthsimpson
Copy link
Contributor Author

@davegarthsimpson can you take care of the 2 changes requested?

@ubidefeo already done I believe!

@davegarthsimpson davegarthsimpson mentioned this pull request Feb 22, 2023
Merged
4 tasks
@kittaakos kittaakos dismissed their stale review February 23, 2023 07:55

new review remark

@kittaakos kittaakos self-requested a review February 23, 2023 07:55
kittaakos
kittaakos reviewed Feb 23, 2023
View reviewed changes
arduino-ide-extension/src/browser/boards/boards-service-provider.ts Outdated
lastBoardsConfigOnUpload.selectedBoard
);

if (!boardIsSameHardware && !boardIsSameFqbn) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

See here: #1913 (review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

response

@davegarthsimpson davegarthsimpson self-assigned this Feb 23, 2023
@davegarthsimpson
Copy link
Contributor Author

closing in favor of #1913

@per1234 per1234 added the conclusion: duplicate Has already been submitted label Feb 24, 2023
@per1234 per1234 deleted the include-hardwareId-in-auto-selection branch August 1, 2023 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kittaakos kittaakos kittaakos approved these changes

@per1234 per1234 Awaiting requested review from per1234

Assignees

@davegarthsimpson davegarthsimpson

Labels
conclusion: duplicate Has already been submitted topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@davegarthsimpson @per1234 @kittaakos @ubidefeo