Skip to content

Added 'hardwareId' support to pluggable discovery #2065

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Feb 6, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?

Add hardwareId support to the Arduino CLI. The documentation has been updated accordingly.
See #2024 for details and rationale about the change in this PR.

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change, and is titled accordingly?

No, it adds new features in a backward compatible way.

Other information

Close #2024

@matthijskooijman
Copy link
Collaborator

I had a quick look over the PR, looks ok to me (did not look very closely, though). I do wonder: does this really need to be its own field? Would it not also make sense to put this into "properties"? I guess like this it allows defining additional semantics (i.e. same hardwareId -> same board instance), which would at best be part of a written convention if stored in properties?

@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Base: 36.67% // Head: 36.76% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (208fc42) compared to base (cc24197).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head 208fc42 differs from pull request most recent head e34fffb. Consider uploading reports for the commit e34fffb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2065      +/-   ##
==========================================
+ Coverage   36.67%   36.76%   +0.08%     
==========================================
  Files         228      228              
  Lines       19383    19385       +2     
==========================================
+ Hits         7108     7126      +18     
+ Misses      11442    11431      -11     
+ Partials      833      828       -5     
Flag Coverage Δ
unit 36.76% <0.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arduino/discovery/discovery.go 19.56% <0.00%> (-0.08%) ⬇️
internal/cli/board/list.go 0.00% <0.00%> (ø)
arduino/cores/packagemanager/package_manager.go 68.19% <0.00%> (+0.72%) ⬆️
commands/lib/search.go 92.10% <0.00%> (+3.94%) ⬆️
arduino/monitor/monitor.go 47.36% <0.00%> (+6.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cmaglie
Copy link
Member Author

cmaglie commented Feb 6, 2023

Would it not also make sense to put this into "properties"?

Because properties is by definition a set of properties without a defined structure, it may contain anything, even another hardwareId that is not the hardwareId we are defining here. If we reserve a key properties.hardwareId this may potentially create a non-backward compatible change (even if the odds are very low admittedly). Anyway I'd like to leave properties as the territory of discovery developers.

cmaglie added a commit to arduino/pluggable-discovery-protocol-handler that referenced this pull request Feb 6, 2023
@umbynos umbynos added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Feb 7, 2023
Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

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

Mainly cosmetic stuff

@cmaglie cmaglie merged commit 396718f into arduino:master Feb 9, 2023
@cmaglie cmaglie deleted the add_hardwareid branch February 9, 2023 14:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow identification of a specific instance of a board from a Pluggable Discovery
5 participants