Skip to content

Pluggable discovery #2

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

Merged
merged 44 commits into from
Jun 21, 2021
Merged

Pluggable discovery #2

Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
3dc657e
Add RFC for pluggable discovery
cmaglie Mar 15, 2021
2f7f65d
Fixed some typos and made RFC compliant to RFC template
cmaglie Mar 16, 2021
3254493
Assign RFC number
cmaglie Mar 31, 2021
78ee413
Added a note about multiple 'add' events
cmaglie Apr 1, 2021
bfcb83d
Added a note about START_SYNC
cmaglie Apr 1, 2021
16a2687
Added discovery priority based on the currently selected board
cmaglie Apr 1, 2021
afac1cc
fix: Address and Protocol must be unique
cmaglie Apr 30, 2021
2a1cc89
Renamed 'prefs' field to 'properties'
cmaglie Apr 30, 2021
2cab0a9
Made matching algorithm more explicit about mixing identification props
cmaglie Apr 30, 2021
e17982c
Removed identificationPrefs
cmaglie Apr 30, 2021
44e8d99
Added some backward compatibility considerations
cmaglie Apr 30, 2021
b10d52d
Added HELLO command
cmaglie Apr 30, 2021
a88bd92
Added possibility to use discovery from other platforms
cmaglie May 4, 2021
4c7f1bf
cleaned up examples for board identification
cmaglie May 4, 2021
972e15a
Update RFCs/0002-pluggable-discovery.md
cmaglie May 13, 2021
8aa8f6e
Made 'plugin' more explicit
cmaglie May 13, 2021
4b9d4b2
Added HELLO negotiation
cmaglie May 13, 2021
0316cb9
Use a single number revision scheme
cmaglie May 13, 2021
e051454
Added 'name' field in hypotetical serial-discovery
cmaglie May 13, 2021
bad688c
Update RFCs/0002-pluggable-discovery.md
cmaglie May 13, 2021
3916891
Update RFCs/0002-pluggable-discovery.md
cmaglie May 13, 2021
26de7ce
Update RFCs/0002-pluggable-discovery.md
cmaglie May 13, 2021
8affbd7
Update RFCs/0002-pluggable-discovery.md
cmaglie May 13, 2021
a4a7e63
fixed link
cmaglie May 13, 2021
ec88d16
Update RFCs/0002-pluggable-discovery.md
cmaglie May 13, 2021
e50a9dc
Removed weird cuts markers
cmaglie May 13, 2021
7f8c194
Update RFCs/0002-pluggable-discovery.md
cmaglie May 13, 2021
69c3f9f
Added a note about LIST timings
cmaglie May 17, 2021
c591ebb
Specify that backward compatibliity rules apply only for VID/PID
cmaglie May 17, 2021
5149d92
Specify that backward compatibliity rules apply even for indexed rules
cmaglie May 17, 2021
cc44f9b
Builtin discoveries are used by default, unless specified
cmaglie May 17, 2021
875ce97
fix typo
cmaglie May 17, 2021
c3242ad
Transfer ALL port metadata into upload preferences
cmaglie May 17, 2021
5bdfe30
Added no-port protocol support
cmaglie May 24, 2021
db340b1
Rename 'user' custom field to 'field'
cmaglie May 24, 2021
bc3963d
Added error handling
cmaglie May 26, 2021
c6cdecc
Moved paragraph and cleared arduino-cli usage scenario
cmaglie May 26, 2021
e756a44
Added more details about message sequencing
cmaglie May 26, 2021
1c082fd
Discovery install via package_index.json
cmaglie May 27, 2021
6ed74a2
Give to 'Board Identification' its own chapter
cmaglie May 27, 2021
2e078d4
Only serial-discovery and network-discovery are automatically added t…
cmaglie May 31, 2021
1885102
Renamed 'no-port' protocol to 'default'
cmaglie May 31, 2021
6830030
Made backward compatibility mode more explicit for default upload
cmaglie May 31, 2021
ef19331
Changed field to integer
cmaglie Jun 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Added possibility to use discovery from other platforms
  • Loading branch information
@cmaglie
cmaglie committed May 4, 2021
commit a88bd92e28183475bc8b663004f323318d908fce
28 changes: 27 additions & 1 deletion RFCs/0002-pluggable-discovery.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,39 @@ discovery.teensy.pattern="{runtime.tools.teensy_ports.path}/hardware/tools/teens

in this case the platform provides a new `teensy` discovery and the command line tool named `teensy_ports` is launched with the `-J2` parameter to start the discovery tool.

#### Using a discovery made by a 3rd party

A platform may opt to depend on a discovery developed by a 3rd party instead of writing and maintaining his own. Since writing a good-quality cross-platform discovery is very hard and time consuming, we expect this option to be used by the majority of developers.

To depend on a 3rd party discovery the platform must add the following directive in the `platform.txt` file:

```
discovery.required=PLATFORM:ARCHITECTURE:DISCOVERY_ID
```

or if the platform needs more discoveries:

```
discovery.required.0=PLATFORM:ARCHITECTURE:DISCOVERY_ID_1
discovery.required.1=PLATFORM:ARCHITECTURE:DISCOVERY_ID_2
...
```

The `PACKAGER:ARCHITECTURE:DISCOVERY_ID` field represents a unique identifier to a 3rd party discovery in particular the `PACKAGER:ARCHITECTURE:...` part is the same as in the FQBN for the boards.

For example if a platform needs the `network` discovery from the Arduino AVR platform it may specify it with:

```
discovery.required=arduino:avr:network
```

#### Duplicate discoveries

It may happen that different 3rd party platforms provides the same discovery or different versions of the same discovery or, worse, different version of the same discovery launched with different parameters.

We can partially handle this if the `DISCOVERY_ID` field in `platform.txt` is well defined: from the CLI we could group together the platforms that requires the same discovery and launch the latest version available just once. How the different 3rd party will agree on the `DISCOVERY_ID` value population is TBD.

In case different discoveries provide conflicting information (for example if two discoveries provide different information for the same port address) we could partially mitigate the issue by giving priority to the discovery that is bundled with the package of the selected board.
In case different discoveries provide conflicting information (for example if two discoveries provide different information for the same port address) we could partially mitigate the issue by giving priority to the discovery that is used by the package of the selected board.
Copy link
Contributor

@matthijskooijman matthijskooijman May 6, 2021

Choose a reason for hiding this comment

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

Should this section on duplicate discoveries be updated now that discovery.required is added? I believe there are two problems here:

  • What if multiple platforms define (different versions of) the same discovery tool, can we somehow match them and run the tool only once? I'm not quite sure about this, this is a hard problem...
  • How to handle duplicate port addresses, and which one to use for the actual upload to a board (but also, for displaying, e.g. should a port list show both, or just one, if the latter, which label to show, etc.). The "which to use for upload" is partly mitigated by discovery.required, which limits to just the explicitly required or defined discoveries. Within that set, it might be good to provide some more explicit guidance over priorities (maybe locally defined over required third-party over (required) builtins)?

Copy link
Member Author

@cmaglie cmaglie May 17, 2021

Choose a reason for hiding this comment

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

What if multiple platforms define (different versions of) the same discovery tool, can we somehow match them and run the tool only once? I'm not quite sure about this, this is a hard problem...

let's suppose we have an hypothetical 3rd party ble discovery, it may happen that one platform defines:

discovery.ble.pattern={runtime.tools.ble-discovery-1.0.path}/ble-disc

and another one:

discovery.ble.pattern={runtime.tools.ble-discovery-1.5.path}/ble-disc-1.5

we may match the discovery.ble. part of the definition to group them together, and look at the (runtime.tools...} definition to identify the latest version of the discovery and run only that one. BTW this is very fragile because it requires the developers to agree on 2 identifiers (the discovery.ble. definition and the tool definition in package_index.json).

Another thing that can be done is to move the discovery definition inside package_index.json at the vendor level, so we will have:

{
  "packages": [
    {
      "name": "arduino",
      "maintainer": "Arduino",
      "websiteURL": "http://www.arduino.cc/",
      "platforms": [

        {
          "name": "Arduino AVR Boards",
          "architecture": "avr",
          "version": "1.6.2",
          ...
          "toolsDependencies": [
            {
              "packager": "arduino",
              "name": "arm-none-eabi-gcc",
              "version": "4.8.3-2014q1"
            },
            {
              "packager": "arduino",
              "name": "CMSIS",
              "version": "4.5.0"
            },
            ...
          ],
+         "discoveriesDependencies": [        <--- discovery that must be installed with the platform
+           { "packager": "arduino",    <--- another vendor may be referenced
+             "name": "ble",
+           }
+         ]
        },

        {
          "name": "Arduino SAMD Boards",
          "architecture": "samd",
          "version": "1.6.18",
          ...
        }

      ],
      "tools": [
        {
          "name": "arm-none-eabi-gcc",
          "version": "4.8.3-2014q1",
          ...
        }
      ],
+     "discoveries": [    <-- discoveries provided by the current vendor (outside a specific platform)
+       { "name": "ble",      <-- discovery ID
+         "tool": {           <-- discovery tool archive files
+           "name": "ble-discovery",
+           "version": "1.0.0",
+           ...
+         }
+       }
+     ]
    }
  }
}

this will de-couple discoveries from the single platforms so, in our example, the ble discovery will be identified by arduino:ble instead of, say, arduino:avr:ble and the platforms.txt may simply be:

discovery.required.0=arduino:ble

in this case the arduino:ble becomes the unique identifier of the tool making it more very easy to group together.
Drawbacks:

  • the discovery must be part of the package_index.json
  • we lose the possibility to provide a recipe via the .pattern=... definition, in this case, the tool must be just a single executable that runs without parameters

Copy link
Member Author

@cmaglie cmaglie May 27, 2021

Choose a reason for hiding this comment

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

@matthijskooijman I've just pushed a proposal on how to integrate discoveries into platforms, based on the package_index.json, that should allow a better "sharing" of the tools.

The old way (directly placing the discovery in the platform and providing a pattern) is still available, but its use should be limited to:

  • developing phase
  • discovery that are not meant to be used by 3rd party

Copy link
Contributor

@matthijskooijman matthijskooijman May 29, 2021

Choose a reason for hiding this comment

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

I read the new RFC before reading the above comments, and was a bit suprised about discoveryDependencies, since it:

  • Adds new complexity while the existing toolDependencies already exists and seemed sufficient
  • Loses flexibility by not allowing comandline parameters and forcing a single executable (without additional files like config files).

However, I had initially not realized that the discovery definitions also moved from the platform level to the vendor level, which is significant because it:

  • Prevents version conflicts between e.g. arduino:samd and arduino:avr (you would still have conflicts/duplication between vendors, but that's probably ok).
  • Allows using a discovery tool from another vendor, without requiring any particular platform of that vendor being actually installed. I now realize that this is actually a very important point, since requiring a full platform to be installed just to use its discovery tool is probably not ideal.

OTOH the last point seems to already be supported by toolDependencies already, in the sense that you can reference another vendor's tools using e.g.:

          "toolsDependencies": [
            {
              "packager": "arduino",
              "name": "arm-none-eabi-gcc",
              "version": "7-2017q4"
            }
          ]

However, that would only allow depending on another vendor's tool to get it installed, it would not allow "importing" the recipe from another (not-installed) platform. In other words, you would have to redefine the recipe in each platform that wants to import a discovery tool, which then leads to duplication and again potential version conflicts...

Summarizing: It's a complex challenge, and I'm not quite sure what the best approach would be. I think the current proposal would work, but also has some limitations and complexities, but I haven't thought of something better here (maybe this needs a more thorough reconsideration of the relation between the JSON and platform.txt, but I guess that's out of scope here...)

Copy link
Member Author

@cmaglie cmaglie May 31, 2021

Choose a reason for hiding this comment

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

I deliberately removed the possibility to provide recipes and to put extra files in the discovery archive.

  • If we allow specifying a recipe, two platforms using the same discovery may choose to run it with different command line parameters, and this fact will make them incompatible and force us to run two instances of the same discovery.
  • Without recipes or config files, we just need the discovery identifier like arduino:ble and we are done.
  • Using toolsDependencies forces you to pin a version: this is a good thing for uploaders/compilers but not for discoveries. We want to always use the latest version available. Adding the discoveryDependencies field looked like the easiest way forward in this case.

I agree that those constraints make the whole system less flexible, but they guarantee that all platforms using the same discovery are compatible, allowing us to run one instance of the discovery for all of them and also allows us to upgrade the discovery tool if needed!

I like the idea that the discovery is not part of a platform and should be able to run independently from it, in this sense, I feel that removing the possibility to pass command line recipes from platform.txt is not a big loss, the to contrary I see it as a way to force developers to design their discoveries with this isolation in mind from the beginning.

Anyway, there is still the possibility to use a recipe exactly as before (in the "not recommended" integration solution), the drawback is that in that case, we will not try to do any optimization and the discovery will be used only on that specific platform (no way to use it from a 3rd party platform).

Copy link
Member Author

@cmaglie cmaglie May 31, 2021

Choose a reason for hiding this comment

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

Anyway, there is still the possibility to use a recipe exactly as before (in the "not recommended" integration solution), the drawback is that in that case, we will not try to do any optimization and the discovery will be used only on that specific platform (no way to use it from a 3rd party platform).

Well, it's still possible to reuse the discovery tool (by toolDepending on it and by adding the recipe), but the CLI will run two instances of the same discovery.

Copy link
Contributor

@matthijskooijman matthijskooijman May 31, 2021

Choose a reason for hiding this comment

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

Yeah, so maybe try this and see how it works, and reiterate later if needed... I see the advantage of enlarging the surface for sharing discoveries between platforms, though that might also result in breakage, one reason to pin versions of tools is to ensure that the platform uses it in a way that will work with that particular version. For discovery tools, there's no chance to get conflicts in commandline arguments, but there could be issues with the properties returned (but maybe that mostly means that discovery tools should be written conservatively, adding new properties rather than changing the meaning of existing ones, etc.).


#### Board identification

Expand Down