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

Pluggable discovery #2

Merged
merged 44 commits into from
Jun 21, 2021
Merged

Pluggable discovery #2

merged 44 commits into from
Jun 21, 2021

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Mar 31, 2021

This document describes how the Pluggable Discovery works and how it should integrate with monitors and uploaders.

When a sketch is uploaded to an Arduino board the only way for transferring the binary executable to the microcontroller is through the serial port. Currently:

  • the subroutines to enumerate the serial ports and to possibly identify a board are “hardcoded” in the IDE/CLI
  • the only way to identify a board is via USB VID/PID identifiers
  • the "serial monitor", to communicate with the board after the upload, is “hardcoded” in the IDE

The current structure does not allow to use different kind of “ports” to communicate with the microcontroller. For example it would be interesting to have the possibility to:

  • discover a board (MDNS) and upload via network (OTA)

  • discover a board and upload via a communication port that is not the serial port (for example via USB-HID as implemented by Teensy or via any other kind of port)

  • allow a third party to implement his own upload protocol / port

The pluggable discovery aims to provide a solution to these problems.

Please check section https://github.com/arduino/tooling-rfcs#5-review-and-revision the RFC is officially open for review so comments are welcome!

@PaulStoffregen
Copy link

A few random comments...

In event mode, discovery may detect changes after the port is initially added. As implemented in 1.8.13, discovery may send another "add" message with the same unique address. The Java IDE treats this repeated add message as replacing the previous discovery info for that address. The meaning of repeated "add" should be documented, or if transmitting another "add" message isn't allowed, that should be specified. Maybe some other message like "replace" chould be defined in that case?

The Java IDE allows discovery to respond with an error to START_SYNC, indicating it does not support event mode. As this draft is worded, event mode seems to be mandatory. Either way is fine as far as I am concerned. As a specification, if any portion of the protocol is optional, which features a discovery utility must implement versus may implement should be clear.

In the case of duplicate addresses, labels or other conflicts, perhaps consider the selected board and give preference to info from the discovery its package provided.

A recipe and maybe discovery metadata need to be defined for discovered hardware to communicate with the serial monitor.

For hardware supporting debugging, maybe recipes and metadata would also be needed to set up a debug session?

@cmaglie
Copy link
Member Author

cmaglie commented Apr 1, 2021

Hi @PaulStoffregen, thanks for the feedback!

In event mode, discovery may detect changes after the port is initially added. As implemented in 1.8.13, discovery may send another "add" message with the same unique address. The Java IDE treats this repeated add message as replacing the previous discovery info for that address. The meaning of repeated "add" should be documented, or if transmitting another "add" message isn't allowed, that should be specified. Maybe some other message like "replace" should be defined in that case?

👍🏼 I'll add a note about this particular use case (multiple add for the same address). I don't think adding another replace message is worth it.

The Java IDE allows discovery to respond with an error to START_SYNC, indicating it does not support event mode. As this draft is worded, event mode seems to be mandatory. Either way is fine as far as I am concerned. As a specification, if any portion of the protocol is optional, which features a discovery utility must implement versus may implement should be clear.

The discovery Is supposed to implement the START_SYNC, I'll add a note about that. Even in the worst case that an event-mode is not doable, I prefer to be the discovery to "emulate" a START_SYNC with an internal polling loop.

In the case of duplicate addresses, labels or other conflicts, perhaps consider the selected board and give preference to info from the discovery its package provided.

👍🏼 added a note

A recipe and maybe discovery metadata need to be defined for discovered hardware to communicate with the serial monitor.
For hardware supporting debugging, maybe recipes and metadata would also be needed to set up a debug session?

Are you referring to pluggable monitors, I guess? I'm going to create a separate RFC for pluggable monitors, I wanted to leave them out from this one to not overcomplicate it.

To answer your question: yes, the idea is to pass this information not only to upload.* recipes but also to monitor.* (TBD on another RFC) and to the debugger, even if it's not explicitly mentioned in this specification it doesn't mean we will not do it later.

About debugging, we are iterating through various options (we already tried to encode the tooling setup into a recitpe.debug=... in platform.txt but it turned out to be quite a nightmare, so we ended up with a configuration-based approach in the current version of the cli). BTW the information from the discovery will be surely made available to the debbuger in a way or another.

@PaulStoffregen
Copy link

@cmaglie - Is it too soon to talk of Pluggable Serial Monitor? Is there any chance both Pluggable Discovery and Pluggable Serial Monitor can happen before the official 2.0 IDE release? Both are needed for Teensy to work. My patches to the Java code add a very rudimentary Pluggable Serial Monitor using stdin / stdout and status updates via stderr. If 2.0 will use localhost sockets or some other IPC method, I'd really like to start moving my code in that direction. Ideally, I'd love to see a 2.0 beta with these features, so I can offer users a way to test Teensy with your beta versions before the 2.0 release.

@ubidefeo
Copy link

@PaulStoffregen
Pluggable Monitor is definitely in the cards, we have started talking about it but we don't have an RFC yet.
Once we're done with Discovery I guess the next logical step is the Monitor, because it can be used to begin implementing other tools such as Serial Plotter.
I'll let @cmaglie keep control over these implementations, I'm just happy if we can make this new IDE more flexible over time.
What I can tell you is that definitely these two features and a couple more need to be brought to life before a final 2.0 is out ;)

@matthijskooijman
Copy link
Contributor

I just gave this proposal an initial read-through, and I'm quite thrilled about it. There has been quite some limitations for boards that deviated from the default serial-port-uploader scheme (i.e. DFU, SWD, etc.) that I think can be neatly solved by this (though I haven't fully thought these through yet).

While reading this proposal, I noted down some questions, suggestions and confusions, see below. It's quite the list and a bit of a braindump here and there, but I hope it will help fuel further thoughts and discussion even though there's mostly questions and not much proposals for improvements in there :-)

I'll try to let this proposal sink in a bit further and see how it holds up for some specific usecases, so hopefully (if I don't get swamped by other work and projects :-p) I can follow up with some more later.

Anyway, here's my wall of text, enjoy!

Regarding the stdin/stdout protocol of discovery tools:

  1. It's not immediately obvious how commands are formatted. Reading closely, they are just the command strings themselves (presumably terminated by a newline? Or CRLF?). This does mean that commands cannot have parameters without messy parsing, so maybe commands should just be JSON strings as well? Even if we do not need parameters now, sending e.g. {"command": "START"} might be more extensible.
  2. Why are commands uppercase and the corresponding eventtypes lowercase? Wouldn't it make sense to make them the same?
  3. For START_SYNC it says "After calling START_SYNC an initial burst of add events may be generated in sequence to report all the ports available at the moment of the start.". This makes sense, since it allows getting a complete picture of the available ports without a race condition (if you would need to call LIST first and then START_SYNC). However, this says "may be generated", shouldn't this be mandatory (to prevent having a limbo situation where you need to call LIST for some discoverers but not for others)? Also, should these intitial add events maybe be marked as such so callers can, if the need to, distinguish between real new ports and pre-existing ports (without relying on some heuristic such as "within x ms of sending the START_SYNC command)? Additionally, would it be useful for callers to know when the initial burst of ports is complete and when the waiting for changes starts? I can imagine that a tool wants to know this to e.g. get wait for an initial complete list first and display that, rather than starting with an empty list that quickly populates (which might not be ideal in UI terms). I guess I'm wondering if START_SYNC should maybe start with a list reply rather than a burst of initial add events?

Then, some thoughts on matching port to boards:

  1. Which ports and boards are considered and which prefs are available to match? I suspect that for ports returned by a given platform's discovery tools, only that platform's boards are considered. For builtin discovery tools, I guess all boards are checked for matching prefs? Or would it make sense to have a platform explicitly reference any builtin discovery tools that it needs (maybe without specifying a recipe, but just referencing them by name)? Going further, it might even be useful for a platform to explicitly include discovery tools from another platform (i.e. by id, not by repeating the recipe), similar how a platform can reference another core, or (implicitly) reuses programmers from a referenced platform? In any case, this does require that builtin discoveries have a very well-defined set of identificationPrefs, which cannot be lightly changed later. For platform-specific discoveries, this is not so strict, since the set of prefs reported by the tool and matched by the platform is just a detail internal to the platform.

  2. Must all preferences listed in identificationPrefs be matched? Or just the ones common to the port and board?

  3. There is an example of identificationPrefs in the proposal:

     myboard.pears.0=20
     myboard.apples.0=30
     myboard.pears.1=30
     myboard.apples.1=40
    

    Which is stated to match pears=20, apples=30 and pears=30, apples=40. But will this also match pears=20, apples=40 (i.e. mixing index 0 and 1), or are these indexes required to match? If the latter, how about when some prefs are indexed multiples and some are not?

  4. Would it make sense to make the prefs used to match against identificationPrefs explicit in boards.txt, e.g. using arduino_zero_edbg.port_match.vid.0=0x03eb or so (probably better name than port_match)? This reduces the chances of accidental false positive or false negative matches on prefs that are not actually intended for matching but just happen to have the same name (especially when new identificationPrefs are added to builtin discoveries later). Such a structure could also be used to disambguate the previous two points, e.g. by having something like:

    myboard.port_match.0.vid=0x1234 # Match just one particular vidpid
    myboard.port_match.0.pid=0x5678
    myboard.port_match.1.vid=0x1234 # Match *all* boards with this vid
    myboard.port_match.2.vid=0x0000 # And all boars with this vid that also have foo=bar
    myboard.port_match.2.foo=bar
    
  5. When matching ports to boards, must available upload protocols for a board be considered? Or is a port matched to a board regardless of available upload protocols and are those only used for subsequently filtering ports available for upload?

Then some more misc thoughts, mostly about the way ports are described and used:

  1. Can a port implement multiple protocols? I can imagine that some USB devices have multiple interfaces that could implement different protocols. If such different protocols can result in different addresses, these could of course be treated as different ports as well (and I guess the address could also be made unique, even if the upload tools for both protocols require the same port name, which can then just be added to prefs and use that in the recipe in place of the uniquefied address). Reading on, it also seems that when a user selects a port for upload that has multiple protocols, it might become ambiguous which upload tool to use, so a single protocol per port probably makes most sense.
  2. In the port properties, reading the LIST command output documentation, I initially thought that prefs and identificationPrefs seemed to generic and did not seem to reflect what I initially understood as their meaning (i.e. info about one particular instance of a board and info about a type of board). However, reading on, I've found that these two properties have different meanings, for which their name actually makes more sense. AFAICT prefs is just an arbitrary list of preferences that is usable in the upload tools recipes (and, AFAICT will not be used by the tooling in any way?). Maybe uploadPrefs might slightly better reflect this? Then identificationPrefs contains info on the board model that this port belongs to, intended to be matched against board preferences to match board definitions against this port (i.e. to identify the board belonging to this port, so I guess the name is actually ok).
  3. In the constraints section, it says "Each port may provide metadata to identify a specific instance of the board (serial number / MAC address)", which I think refers to the prefs attribute? This initially made me thought that the prefs attribute is actually exclusively info on the board instance (implying that two different ports belonging to the same board instance would have identical prefs), but it seems that prefs is really metatadata on a specific instance of the port (which is somewhat equivalent to a specific instance of a board for single-port boards). So maybe this should be rephrased somehow to clarify this.
  4. Maybe some explicit mention should be made of what constitutes the identity of a port. In particular, I think that address now constitutes the identifier of each port and should be unique (re-reading, this is already mentioned in the constraints section). Or is it protocol + port (this ties into the multiple protocols per port question, I guess)? This is also relevant for correlating add and remove events. And should such identify/address be globally unique, or only within a given discovery tool? How to deal with two discoveries returning the same address?
  5. Do we need a way to correlate multiple ports belonging to the same board instance? The constraints section talks about "specific instance[s] of the board", which lead me to believe that a board instance is somehow an explicit concept in this proposal, but reading more carefully, this concept does not seem to exist at all. For the purposes of this proposal, a board instance that has two ports is effectively the same as two separate board instances (that maybe have same serial or so). Would it be valuable to add this concept somehow, e.g. by adding one ore more "board instance identifiers" to a port, that can be used to group multiple ports belonging to the same board instance? This might also need to be specified to work across different discovery tools, e.g. a board might have a serial port, but also an USB DFU endpoint, which I think would be discovered by different tools, but should probably be correlated to belong to the same board instance (this helps for the serial monitor extension later, but correlating the serial port with the USB endpoint can also maybe help the user distinguish multiple similar boards).
  6. Platforms can define custom discovery tools, but when should these run? When any board from such platform is selected? Or should tools be connected to specific boards? When doing a generic port list (e.g. using arduino-cli or so), I guess all tools from all platforms should be ran?
  7. Should we make more details available for each port? I.e. for a USB-serial port, in addition to the vid-pid, you could more specifically trace the port to a USB configuration index, interface index, maybe even endpoint index. I don't think such info belongs in identificationPrefs (since this is really additional info on the port's location or identity, not on the board it is connected to). I'm also not sure if this is at all relevant for serial ports, but e.g. for DFU uploads, different tools can accept different ways to identify a particular port/device. For example dfu-util allows selecting a device based on its serial number, USB devnum or USB path (i.e. 1-1.5.4.1), openocd seems to use just the USB path, while STM32 cube programmer uses its own set of identifiers (but also seems to support serial numbers). Some of these might be specific on just one platform or have different meanings on different platforms (e.g. the sysfs/udev path of a (USB) device is also used to identify devices on Linux sometimes). Now I better understand how prefs is used, I guess all of these identifiers are just additional info about a particular port and could be put into prefs, which also allows using them in recipes. It would be good if we could have some guidance about such properties, so different discovery tools would use the same names and same formats for these things were possible.
  8. Historically, there was a serial.port=ttyACM0 and serial.port.file=/dev/ttyACM0, but the examples suggest that now only /dev/ttyACM0 is returned by the discovery tool. Should the short form also be returned by the discovery tool for upload tools that need it (and do not want to rely on backward compatibility variables)? I guess this is just a very specific instance of the previous point.
  9. In the examples a protocol network is mentioned, but I wonder if that is not way too generic? To really return a meaningful list of network devices, some selection based on e.g. available services is needed? Or maybe just listing all devices discovered by e.g. mdns and let the user sort out the right ones could work (or maybe any advertised service names or so could be added to identifcationPrefs, or using macprefixes seems to be a strategy from the examples?)
  10. I think it would be helpful if some examples of discovery output and recipes would be done for a few more protocol/port types (I'm thinking of DFU and EDBG in particular). This helps understand things, and also helps exposing flaws in the proposal where it fails to express things we'll be needing.
  11. How does all this relate to upload using programmer? In a way, I guess a programmer could be treated as a board by itself,
    with its own prefs (e.g. vid/pid) matching against available ports, etc. but I haven't thought this through.

Finally, responding to an earlier comment:

Pluggable Monitor is definitely in the cards, we have started talking about it but we don't have an RFC yet.

It would be good to at least think about this a bit, since I guess that serial monitor would typically also have ports connected to boards, and some of these ports might overlap with the upload ports (e.g. with most standard serial-based bootloader uploading). So maybe serial monitor could simply be one of the protocols supported by a port.

@matthijskooijman
Copy link
Contributor

I already gave this some more thought over dinner. First off, here's a few usecases that I think would be worthwhile to consider:

  1. The Arduino Leonard or similar. These are boards that normally expose a serial monitor port, which is given a 1200bps touch, and then resets into the bootloader (which involves re-enumerating the USB device). The bootloader then offers a new serial port, through which the upload happens.
  2. The Arduino zero, which offers two ways to upload: One is through the embedded EDBG chip, which is essentially just an integrated programmer. The other is through the native USB connection with bossac, though I have to admit I'm not entirely sure how this one works (it does seem to use 1200bps touch and a serial port).
  3. STM32duino-based boards using DFU upload. These use 1200bps touch to reset into the ROM bootloader, which re-enumerates as a DFU device. The upload tool (STM32CubeProgrammer currently, but it would be nice to (also) support openocd) then handles the DFU upload. This currently just uses the first available DFU device, which might not be the right one.
  4. STM32duino-based boards using DFU and DFU auto-reset. I've been using this in my fork and hope to get this merged as part of [Beta] Jump to system memory boot from user application stm32duino/Arduino_Core_STM32#710. When the sketch is running, it exposes another USB interface (in addition to ACM serial) for DFU that supports a reset command. This command resets into the bootloader, which then accepts the firmware over the full DFU protocol. Both the reset and upload can be handled by dfu-util.
  5. STM32duino-based boards using mass storage upload. This just copies a file into a (to be) mounted directory. The current script for this ignores the serial port completely and instead just looks at the filesystem label, which contains the particular board name.
  6. STM32duino-based boards using HID bootloader. These are reset into the bootloader using some twiddling of the serial port settings, I believe, and after that re-enumerate as a HID device for the upload. Both the reset and upload is handled by the upload tool, so from the perspective of the Arduino tooling, this is a simple upload (just pass it a serial port and it will upload). There's one exception, though: When the main sketch breaks and the board gets stuck in bootloader mode, I'm not exactly sure how that would work with this tool.

There's a lot of STM32duino in there, mostly because I have been working with it a lot, and because it uses a lot of different upload methods (there's a bunch more that I haven't even mentioned).

Matching ports after bootloader reset
A lot of these boards use the 1200bps touch to reset into the bootloader. What happens then varies:

  • For the Leonardo, it reenumerates as a serial port (but it might not have the same address, so I think currently arduino-cli has some semi-hardcoded heuristics to find the bootloader serial port after re-enumeration, but I think this is currently still just looks for any port that appears at the right time (though there was some work on improving this, haven't checked what it changes exactly).
  • For the DFU versions, something similar happens, but since there is no serial port after reset, arduino-cli does not find a post-reset port but just calls the tools, which just upload to the first available DFU device (possibly limited by USB vidpid), which is imperfect.
  • I'm not quite sure for bossac and the Arduino zero.

In all these cases, it would be nice if the tooling could deterministically the post-reset upload port.

One complication is that in some cases, the type of port changes (e.g. from serial to DFU), which could mean we need to match ports between different discovery tools, or that e.g. the DFU discovery tool must also return serial ports so it can stay in control of both. Neither seem ideal, though.

The other question is how to match the ports before and after. In these cases, matching based on the USB path (i.e. the chain of ports and hubs, such as 1-1.5.4.1) seems an obvious choice, though it might not be available (or easy to figure out) on all platforms. Ideally, though, this would of course not be some fixed property, but different discoverers could offer different identifiers to correlate ports or even multiple identifiers (I can imagine the builtin serial discovery would return serial number, USB path, usb devnum, and maybe other properties that can be used to match).

Then should we somewhere declare the protocols of the port before reset and after? OTOH, the port before is necessarily a serial port (to allow 1200bps touching), so maybe that can be hardcoded (just like how 1200bps touch works). Then the boards.txt can just declare the protocol after reset, maybe? And some other property to declare which pref should be used to match the before-reset-serial port to the after-reset-other-protocol-port? Or, another approach could be to turn the upload in a two-step process, involving two upload tools: One that just does the 1200bps touch, declares how to find the post-upload port and which upload tool to use after the reset? That seems like it could turn out elegantly (and maybe even be generalized to other reset-and-then-upload procedures, where the reset might be done by one tool and the upload by another).

Another thought could be to not match ports against each other, but the assign ports to a board instance (maybe through the same one or more identifiers mentioned above), and then just use another port with the appropriate protocol belonging to the same board instance.

Recovering when stuck in bootloader
Another thing to consider, is how to recover from a failed upload, so when a board is stuck in the bootloader (or was forcibly put into bootloader mode, using e.g. the reset button on a Leonardo or the BOOT0 pin on an STM32). For the Leonardo, the upload happens using serial, so you can just upload normally, but on other cases (DFU, HID) there is no serial port to select and apply the 1200bps touch on. Currently, you can usually start an upload on any other serial port, on which the 1200bps touch fizzles, but since the actual upload then just uses the first available e.g. DFU device, it still works, but it's not quite nice.

However, the above suggestion of making these a two-step upload process, with a separate tool operating on a separate port (e.g. with protocol=dfu) would cause these post-reset bootloader ports to also show up in the normal list of ports to select, so the user can just select a device stuck in bootloader mode and start an upload normally.

Ok, so there's another wall of text... I was hoping to also write down some of my thoughts in example JSON port descriptions and maybe boards.txt/platform.txt snippets, but I ran out of time for now, maybe more later. In the meantime, love to hear some thoughts on all this....

@PaulStoffregen
Copy link

The common use case is for platform.txt recipes and board-package specific utilities those recipes run to be the "consumers" of most of the JSON identification fields.

Yes, it is good to think of how many specific cases would be handled. But I do not believe every question needs to be fully answered. In fact, most questions about how specific boards would work don't need to be answered.

Just knowing that Pluggable Discovery provides a flexible mechanism for the designer of a board-package to craft their own discoverer utility and have it work seemlessly with their corresponding upload and "serial" communication utility (for use with forthcoming Pluggable Serial Monitor) should be enough.

@matthijskooijman
Copy link
Contributor

You make a good point that the primary goal of this RFC is probably to define the way discovery tools are defined and communicated with, and they are just a building block to build upon. In that sense, a lot of my remarks are not relevant in this stage, since they are more about the content of these port descriptions and how they are handled by the tooling and/or platform.txt, than about the actual discovery protocol itself.

However, since this proposal does define some things about the content of these port descriptions (the basic structure and some of the content through examples), it does make sense to think about this aspect too.

I do suspect that for what you call the "common use case", cases where the discovery tool and upload tool are platform-specific and matched to each other, the current proposal is likely sufficiently defined (and a big step forward over the current hardcoded approaches). But IMHO it would be a pity if we move forward with this proposal, only to find out that it is insufficiently expressive to handle the somewhat more complex cases. I don't think it's needed to solve every case right now, but exploring some cases in more depth is, in my experience, a good way to discover if a tool is sufficiently flexible and expressive.

Looking back at my previous comments, I think the questions that are most pertinent to the specification of the discovery tool mechanism itself (ignoring the inner content of the protocol, i.e. which prefs are returned and how they are used), are:

  • The first couple of questions about the stdin/stdout protocol
  • The question of whether a "board instance" should be a explicit concept or not (since this could influence the JSON format)
  • The naming of prefs and identificationPrefs (see also below for some additional thoughts).

In a sense, it might be good to further decouple the discovery of ports from the way they are used. A discovery tool should just discover ports and describe them in as much detail as possible, leaving it to the platform/platform.txt to define how to actually handle/match/filter the resulting ports and their info (this is mostly true for builtin discoveries, for platform-specific discoveries, it would be fine to put more logic in the discoveries if that helps).

In this light, it might be good to rename prefs to something like attributes. The name "prefs" refers to "preferences", which makes some sense in the internal Arduino tooling vocabulary that refers to the stuff in e.g. platform.txt/boards.txt as prefs, but in this context, these really are not "prefs" except that they will be made available as prefs to the upload recipe. So in a sense, the name "prefs" comes from the way these attributes are used, not from what they actually are. So, in that sense, I would suggest just calling them attributes, since they are just arbitrary attributes of the port, and then elsewhere specify that all attributes of the port are made available to the upload recipe. It's a small change, but I think this might be good for the understanding and flexibility later.

Then there is the question of identificationPrefs, and I wonder if we really need them at all. Having separate identificationPrefs again makes an assumption about how these attributes are used and lets the discovery tool decide which attributes are used for identification and which are not. If we want to keep the discovery tools generic and simple, maybe the discovery should just return a single list of attributes (i.e. prefs in the current proposal, which seems to always include all identificationPrefs anyway, at least in the current examples) and then put the control over which attributes are used for matching a port to a board with the boards.txt. This is (I now realize) neatly supported by the port_match suggestion I made earlier (point 7 in #2 (comment)), e.g. something like:

myboard.port_match.0.vid=0x1234
myboard.port_match.0.pid=0x5678

One remaining case for having a separate identificationPrefs (maybe under a different name) could be the observation that prefs would be attributes of the port, while identificationPrefs would be attributes of the board model. However, I think this is a very thin line, and not really relevant in practice (it's not up to the tooling to make this distinction, but if needed platform.txt/boards.txt can still make this distinction through the properties they select to use in port matching or recipe generation).

@PaulStoffregen
Copy link

Several of your questions ask "why". Just to explain, to quite some degree this RFC began as a description of the actual implementation of Pluggable Discovery which already exists in Arduino IDE 1.8.9 - 1.8.13. As far as I know, Teensy and Omzlo may be the only boards using Pluggable Discovery. But do keep in mind Arduino 1.8.9 was released on March 15, 2019, so Pluggable Discovery has been a public API for over 2 years. It's certainly not new. Other boards may already be using Pluggable Discovery.

Of course future Arduino CLI & IDE are not bound by the protocol as it exists today. My understanding of this RFC is to both document and discuss the protocol. If changes are to be made that break backwards compatibility with 1.8.13, hopefully this RFC process and whatever gets implemented in IDE 2.0 can become a long-term stable public API, where future improvements maintain backwards compatibility.

I believe @cmaglie chose to use JSON for discovery utility output but a very simple & ad-hoc plain text command input format because of the expected difference in programming environments. Discovery utilities are anticipated to usually be written in C, since they make use of low-level hardware access APIs. While C libraries do exist for JSON parsing, I believe the expected use case is very simple C programming where stdin is parsed for fixed strings and stdout is generated by printf() statements which happen to print JSON. I can tell you that is the way I wrote Teensy's discovery utility, and this early proof of concept discovery utility.

Your points about the naming are good, especially "prefs". Personally, I do not really care what field names are used. What matters most (at least to me) is the meaning is well documented. And hopefully in the future the names can remain long-term stable, so I don't have to again rewrite Teensy's discovery utility, and Omzlo doesn't have to change theirs, and likewise for everyone who will use this public API.

@matthijskooijman
Copy link
Contributor

Several of your questions ask "why". Just to explain, to quite some degree this RFC began as a description of the actual implementation of Pluggable Discovery which already exists in Arduino IDE 1.8.9 - 1.8.13.

Right, seems I had not realized that, and apparently the wording of this RFC had lead me to believe something different. Thanks for clarifying. This also means that changing details (such as naming) must probably be more carefully considered, because of the required compatibility with the current implementation.

I believe @cmaglie chose to use JSON for discovery utility output but a very simple & ad-hoc plain text command input format because of the expected difference in programming environments. Discovery utilities are anticipated to usually be written in C, (...)

Right, that is indeed reasonable rationale to make the commands simple strings and still use JSON for output. If we'd ever need commands with parameters, then we can always revisit this and encode just the parameters in JSON or so.

@cmaglie
Copy link
Member Author

cmaglie commented Apr 26, 2021

Hi @matthijskooijman, thanks for the brainstorming, let me try to answer some of your questions:

Regarding the stdin/stdout protocol of discovery tools:

The rationale behind the choice of plaintext/JSON for stdin/stdout respectively has already been well explained by Paul.

  1. For START_SYNC it says "After calling START_SYNC an initial burst of add events may be generated" [...] However, this says "may be generated", shouldn't this be mandatory [...]?

you're right, I'll change "may be generated" to "is generated"

Also, should these intitial add events maybe be marked as such so callers can, if the need to, distinguish between real new ports and pre-existing ports [...] I guess I'm wondering if START_SYNC should maybe start with a list reply rather than a burst of initial add events?

The goal of the discovery is to list the available ports to allow the user to select one and upload to a board (we use it to populate the Tools->Port menu basically), so it doesn't matter if the board has been just plugged in or it was already present. Anyway, there should be no problems for GUIs (we are talking about a handful of ports not thousands!) IMHO it's not worth adding this extra complexity.

About matching port to boards:

  1. Which ports and boards are considered and which prefs are available to match? [...]

The idea is that each discovery provides port metadata (identificationPrefs) that may match any board on any platform (even a different platform from the discovery).

Must all preferences listed in identificationPrefs be matched? Or just the ones common to the port and board?

all identificationPrefs must match, if a platform defines less prefs it did not match.

There is an example of identificationPrefs in the proposal:

myboard.pears.0=20
myboard.apples.0=30
myboard.pears.1=30
myboard.apples.1=40

[...] But will this also match pears=20, apples=40 (i.e. >mixing index 0 and 1)?

no

how about when some prefs are indexed multiples time and some are not?

they match only when all identificationPrefs provided by the discovery are present in the board properties. If the board declares a subset of the given identificationPrefs it won't match.

When matching ports to boards, must available upload protocols for a board be considered? Or is a port matched to a board regardless of available upload protocols and are those only used for subsequently filtering ports available for upload?

The latter: the board is matched regardless of the protocol.

Would it make sense to make the prefs used to match against identificationPrefs explicit in boards.txt, e.g. using arduino_zero_edbg.port_match.vid.0=0x03eb or so (probably better name than port_match)? This reduces the chances of accidental false positive or false negative matches on prefs that are not actually intended for matching but just happen to have the same name (especially when new identificationPrefs are added to builtin discoveries later). Such a structure could also be used to disambguate the previous two points, e.g. by having something like:

myboard.port_match.0.vid=0x1234 # Match just one particular vidpid
myboard.port_match.0.pid=0x5678
myboard.port_match.1.vid=0x1234 # Match *all* boards with this vid
myboard.port_match.2.vid=0x0000 # And all boars with this vid that also have foo=bar
myboard.port_match.2.foo=bar

that makes sense, but let me think about it for a moment: currently, the board definitions are something like

# Arduino Zero (Native USB Port)
# --------------------------------------
arduino_zero_native.name=Arduino Zero (Native USB Port)
arduino_zero_native.vid.0=0x2341
arduino_zero_native.pid.0=0x804d
arduino_zero_native.vid.1=0x2341
arduino_zero_native.pid.1=0x004d

that should be changed to something like:

# Arduino Zero (Native USB Port)
# --------------------------------------
arduino_zero_native.name=Arduino Zero (Native USB Port)
arduino_zero_native.port_match.0.vid=0x2341
arduino_zero_native.port_match.0.pid=0x804d
arduino_zero_native.port_match.1.vid=0x2341
arduino_zero_native.port_match.1.pid=0x004d

but let's consider also that it's highly desirable to keep backward compatibility too, so we should keep both definitions:

# Arduino Zero (Native USB Port)
# --------------------------------------
arduino_zero_native.name=Arduino Zero (Native USB Port)
# for old IDE
arduino_zero_native.vid.0=0x2341
arduino_zero_native.pid.0=0x804d
arduino_zero_native.vid.1=0x2341
arduino_zero_native.pid.1=0x004d
# for Pluggable Discovery
arduino_zero_native.port_match.0.vid=0x2341
arduino_zero_native.port_match.0.pid=0x804d
arduino_zero_native.port_match.1.vid=0x2341
arduino_zero_native.port_match.1.pid=0x004d

and possibly we should automatically convert "old-style" vid/pid definitions into the new boardname.port_match.* so old platforms will continue to work on the pluggable discovery.
BTW I like this approach, it makes the port identification much more flexible, so I'll probably adjust the RFC to add this prefix.

Ok for now I stop here, @matthijskooijman I've quickly read the remaining wall of text (damn you! :-D), but it requires a more detailed answer, tomorrow I'll add some more comments.

@PaulStoffregen
Copy link

Backward compatibility is desirable. But if a breaking change is good for the long-term Arduino ecosystem, I believe changes should be considered.

@cmaglie
Copy link
Member Author

cmaglie commented Apr 28, 2021

ok, @matthijskooijman let me answer some more questions and after that, I'll try to explain a bit more in depth the RFC...

  1. Can a port implement multiple protocols?

No, every port has only one protocol.

  1. [prefs field is not clear maybe renaming to...] uploadPrefs might slightly better reflect this?

Yes, that may be a better alternative, but thinking on this a bit more, we are not talking about preferences but properties, so I'm going to rename the fields as follows:

  • prefs -> portProperties
  • identificationPrefs -> boardIdentificationProperties
  1. In the constraints section, it says "Each port may provide metadata to identify a specific instance of the board (serial number / MAC address)", which I think refers to the prefs attribute? This initially made me thought that the prefs attribute is actually exclusively info on the board instance (implying that two different ports belonging to the same board instance would have identical prefs), but it seems that prefs is really metatadata on a specific instance of the port (which is somewhat equivalent to a specific instance of a board for single-port boards). So maybe this should be rephrased somehow to clarify this.

Agreed, I'll rephrase it

  1. Maybe some explicit mention should be made of what constitutes the identity of a port. In particular, I think that address now constitutes the identifier of each port and should be unique (re-reading, this is already mentioned in the constraints section). Or is it protocol + port (this ties into the multiple protocols per port question, I guess)?

In theory, the user should be able to select a port by specifying the pair (protocol, address), so each port should have a unique identifier made by the pair (protocol, address) and this should be true across all ports of all discoveries. BTW this is not guaranteed by the specification because it requires some sort of agreement between all the discoveries (and consequently between all the developers... this is not going to happen).

This is also relevant for correlating add and remove events. And should such identify/address be globally unique, or only within a given discovery tool? How to deal with two discoveries returning the same address?

The remove event has only the address field and this is wrong, it should report both address and protocol, I'll fix this in the specification 👍🏼 good catch.
If multiple discoveries report the same pair (protocol, address) the discovery included in the board's package gets priority (thanks to @PaulStoffregen suggested this change). We may still have a tie, in that case we should let the user decide, I see no other solutions.

  1. Do we need a way to correlate multiple ports belonging to the same board instance?

This is a "nice to have" I've thought in the early draft, I liked the idea that the GUI may show the ports belonging to the same board nicely grouped together.
BTW I dropped this idea because there are some challenges here:

  • we must find a "cross-protocol" boardSerialNumber, something like a CPUID
  • once we have the boardSerialNumber we must find a way to add its value as extra metadata in the payload of each protocol, for example, let's consider the USBSerial discovery: how the boardSerialNumber is supposed to reach the discovery? should it be added as a payload to the USB descriptor? is this even possible? The same applies for all the other protocols but, unless they have a field planned for this specific purpose, this looks quite hacky.
    BTW if there is interest we may also add a field boardSerialNumber in a later revision of the RFC (adding is always possible without much risk)
  1. Platforms can define custom discovery tools, but when should these run? When any board from such platform is selected? Or should tools be connected to specific boards? When doing a generic port list (e.g. using arduino-cli or so), I guess all tools from all platforms should be ran?

All discovery from all installed platforms are run simultaneously. If you run arduino-cli board list the LIST command is used to get a one-shot list of all ports. When running in daemon mode instead the START_SYNC method is used so the IDE does not need to poll.

  1. Should we make more details available for each port? I.e. for a USB-serial port, in addition to the vid-pid, you could more specifically trace the port to a USB configuration index, interface index, maybe even endpoint index. [...] I guess all of these identifiers are just additional info about a particular port and could be put into prefs, which also allows using them in recipes. It would be good if we could have some guidance about such properties, so different discovery tools would use the same names and same formats for these things were possible.

Exactly, you can add as many metadata as you want in the prefs section. About the "guidance" we may provide some guidelines, but I'm not very optimist on the outcome, if the pluggable discovery will gain popularity we will probably see all kind of variants popping up, regardless of the guidelines. Also who is going to provide these guidelines?

  1. Historically, there was a serial.port=ttyACM0 and serial.port.file=/dev/ttyACM0, but the examples suggest that now only /dev/ttyACM0 is returned by the discovery tool. Should the short form also be returned by the discovery tool for upload tools that need it (and do not want to rely on backward compatibility variables)? I guess this is just a very specific instance of the previous point.

There is a specific paragraph about that:
For backward compatibility we will keep a copy of the address also in {serial.port} and in the specific case of a protocol=serial we will populate also {serial.port.file}.

unfortunately this can not be resolved in the discovery, even adding the property serial.port.file=ttyACM0 inside the prefs would lead to the {upload.serial.port.file} property to be provided in the upload recipe, but we want {serial.port.file}

  1. In the examples a protocol network is mentioned, but I wonder if that is not way too generic? To really return a meaningful list of network devices, some selection based on e.g. available services is needed?

network is really too generic, probably I should change it to mdns? In any case the meaningful part is that the protocol field is used to select the upload recipe. Let's look the example from the RFC, before the pluggable discovery we have:

[in boards.txt:]
myboard.name=MyBoard
myboard.upload.tool=bossac

[in platform.txt]
tools.bossac.upload.pattern=.......

bossac is the recipe-identifier that tells which recipe from platform.txt we must use for the upload, in general the myboard.upload.tool=RECIPEID is a pointer to the tools.RECIPEID.upload.pattern in platform.txt.
With the pluggable discovery we must change it this way:

[in boards.txt:]
myboard.name=MyBoard
# Upload recipes
myboard.upload.tool.serial=bossac
myboard.upload.tool.network=arduino_ota

[in platform.txt]
tools.bossac.upload.pattern=...(this one is used for serial protocol)...

tools.arduino_ota.upload.pattern=...(this one is used for network protocol)...

this means that each board can have his own particular way to upload via network for example:

[in boards.txt:]
linuxboard.name=A board running linux
linuxboard.upload.tool.network=scp

anotherboard.name=A board with a microcontroller + wifi module
anotherboard.upload.tool.network=arduino_ota

even if the discovery is the same we may run different recipes for upload, for example the linuxboard run scp to upload the binary while anotherboard run an ota client that transfers the binary via TCP.
The discovery should provide just the port address+protocol and some meaningful properties, it's a duty of the platform to decide how to use them.

  1. How does all this relate to upload using programmer? In a way, I guess a programmer could be treated as a board by itself,
    with its own prefs (e.g. vid/pid) matching against available ports, etc. but I haven't thought this through.

The programmer works exactly as before: the platform provides a programmers.txt file with a list of definitions that overrides part of the boards.txt definitions (in particular the upload.tool.* which allows to use a different recipe for uploading).

@matthijskooijman
Copy link
Contributor

matthijskooijman commented Apr 28, 2021

@cmaglie, Thanks for your extensive followup, here's some more responses to that (no new things, I promise :-p)

how about when some prefs are indexed multiples time and some are not?

You misunderstood what I meant by this. I meant, what if I have:

myboard.pears.0=20
myboard.pears.1=30
myboard.apples=40

Will this match e.g. pears=20, apples=40 (or any other combination involving pears and apples). I suspect the answer is "no", which is fine, but that might need to be specified somewhere (maybe not even here, though).

and possibly we should automatically convert "old-style" vid/pid definitions into the new boardname.port_match.* so old platforms will continue to work on the pluggable discovery.

Yeah, I suspect that currently it's almost exclusively the vid/pid values that are used, so autoconverting just those probably makes sense (provided there is no port_match value at all, if there is, of course no autoconverting anymore). I guess that if pluggable discovery is already in use, there could be other values than vid/pid in use currently, but I have no idea if there are.

Yes, that may be a better alternative, but thinking on this a bit more, we are not talking about preferences but properties, so I'm going to rename the fields as follows:

  • prefs -> portProperties
  • identificationPrefs -> boardIdentificationProperties

Right, sound good. Note that I made a similar suggestion to just use attributes in a later #2 (comment), so seems we're of the same mind there. Note that in that same comment I also wondered if we need identificationPrefs / boardIdentificationProperties at all, though.

  1. port identification
    In theory, the user should be able to select a port by specifying the pair (protocol, address), so each port should have a unique identifier made by the pair (protocol, address) and this should be true across all ports of all discoveries.

So this means identity involves address? Does that mean that a discovery can return the same address twice, but with different protocols? I think not, currently, unless the delete event necessarily deletes both ports with the same address.

and this should be true across all ports of all discoveries. BTW this is not guaranteed by the specification because it requires some sort of agreement between all the discoveries (and consequently between all the developers... this is not going to happen).

Yeah, this is why I thought of only applying discoveries that are defined by the board's platform (plus builtins, which are then implicitly or explicitly included by the platform). That would solve at least some of the overlap issues between different platforms that use different address and property conventions?

  1. Do we need a way to correlate multiple ports belonging to the same board instance?
    This is a "nice to have" I've thought in the early draft, I liked the idea that the GUI may show the ports belonging to the same board nicely grouped together.

Agreed that this is probably more trouble than it's worth. However, I do think there's two more things were port grouping by board might be needed/helpful:

  • To find ports after a (e.g. 1200bps) reset that re-enumerates ports (see my later comment above). However, as suggested there, you might actually want to define a way to match the post-reset port from the pre-upload port, rather than just assuming that any port connected to the same board is ok (which it might not be if a board has multiple ports).
  • To find a serial monitor port corresponding to the board you are uploading to, in case that the upload port and monitor port are different. But maybe then boards.txt could, in a similar way to the 1200bps reset thing, somehow provide rules to find the serial monitor port based on the upload port properties, which is probably more powerful than grouping by board and making assumptions.
  1. more port metadata
    Exactly, you can add as many metadata as you want in the prefs section. About the "guidance" we may provide some guidelines, but I'm not very optimist on the outcome, if the pluggable discovery will gain popularity we will probably see all kind of variants popping up, regardless of the guidelines. Also who is going to provide these guidelines?

Well, one starting point would be to write up a few properties to be returned by the builtin discovery tools (I'm assuming there will be a few builtins, at least serial, maybe DFU, maybe a generic USB one using vid/pid matching or even USB descriptor matching), with what they are expected to contain, in order to ensure that some common properties (most obvious and widespread are probably a few USB properties) are the same across builtin tools, which could also be a recommendation for third party tools. Anyone is free to pick their own convention, of course, but I suspect that third-party tools mimicking the builtin ones is convenient for those third parties as well.

  1. /dev/ttyACM0 vs ttyACM0
    There is a specific paragraph about that:

For backward compatibility we will keep a copy of the address also in {serial.port} and in the specific case of a protocol=serial we will populate also {serial.port.file}.

Yes, but I wasn't talking about compatibility with existing recipes, I was asking: How should a modern recipe (not intended to rely on this backward compatible {serial.port.file}) access the ttyACM0 version. Most likely this is something for the serial discovery tool, which should return name: ttyAMC0 or so in the prefs/uploadProperties, so not something to change in this RFC (except maybe in the examples).

  1. network too generic?
    even if the discovery is the same we may run different recipes for upload, for example the linuxboard run scp to upload the binary while anotherboard run an ota client that transfers the binary via TCP.

Yup, this is true, but this does rely on a generic network discovery tool to provide sufficient identification properties so each board can match against only network ports that it can actually use. OTOH, if this is not the case, a platform can always provide a custom discovery tool that discovers network hosts using additional filtering (based on port probing, mdns service records, UPNP, etc.), give that a new protocol name and use that. So, I guess a generic network name is actually appropriate, assuming that it just tries to discover as much network hosts on the local network as possible, without any filtering (but with providing as much info as possible, of course).

The discovery should provide just the port address+protocol and some meaningful properties, it's a duty of the platform to decide how to use them.

Yup.

  1. upload using programmer
    The programmer works exactly as before: the platform provides a programmers.txt file with a list of definitions that overrides part of the boards.txt definitions (in particular the upload.tool.* which allows to use a different recipe for uploading).

Yeah, but how would port matching and selection work for programmers? Currently, the selected board serial port also selects the programmer port to use, for serial programmers, while non-serial-based (e.g. native USB) programmers typically ignore the port selection and just select the first available device based on e.g. vidpid matching internal to the tool (which can be problematic when you have multiple programmers attached, which is not so unlikely when using boards like the STM32 Nucleo/discovery, which have an ST-link programmer integrated, or the zero which has a CMSIS-DAP programmer integrated IIRC). It would be nice if this generalization of the port concept could also be applied to programmers (which would then need to define their own port_match properties, I guess?).

@cmaglie
Copy link
Member Author

cmaglie commented Apr 29, 2021

Will this match e.g. pears=20, apples=40 (or any other combination involving pears and apples). I suspect the answer is "no", which is fine, but that might need to be specified somewhere (maybe not even here, though).

Ahh, of course not, it will not match. I'll try to specify better in the RFC

Note that in that same comment I also wondered if we need identificationPrefs / boardIdentificationProperties at all, though.

Right, once the identification properties are clearly prefixed in the boards.txt definition, the only reason to keep identificationPrefs is that these properties refer to a board while the prefs refers to a port, but as you said, this is a really thin line, and thinking about this a bit more it's probably better to leave this choice to the platform. So I'll implement this change in the RFC (removing the identificationPrefs).

Does that mean that a discovery can return the same address twice, but with different protocols? I think not, currently, unless the remove event necessarily deletes both ports with the same address.

instead yes! (like, just to say, a network host that is called exactly COM1... very unlikely to happen but still possible)
As I said the remove event must report the protocol and the address, this is an error in the RFC that I must fix.

this is why I thought of only applying discoveries that are defined by the board's platform (plus builtins, which are then implicitly or explicitly included by the platform). That would solve at least some of the overlap issues between different platforms that use different address and property conventions?

So to recap your suggestion is:

  1. the builtin discoveries (serial and network) are shared between all platforms by default
  2. a platform must declare the discoveries used for port discovery
  3. a platform may refer to another discovery from another platform

in particular, I like 3 which should reduce the urge for implementors to create their own discovery and push for reuse/contribute to the existing ones.

That works for me, I'll try to adapt the specification for that.

Well, one starting point would be to write up a few properties to be returned by the builtin discovery tools (I'm assuming there will be a few builtins, at least serial, maybe DFU, maybe a generic USB one using vid/pid matching or even USB descriptor matching), with what they are expected to contain, in order to ensure that some common properties (most obvious and widespread are probably a few USB properties) are the same across builtin tools, which could also be a recommendation for third party tools. Anyone is free to pick their own convention, of course, but I suspect that third-party tools mimicking the builtin ones is convenient for those third parties as well.

For now, the only discovery we will provide is serial and network. Having a good starting implementation for the most common discoveries right from the start will surely help to make the ecosystem more coherent in the long run. But time is a factor... writing a good-quality cross-platform discovery is quite hard, especially when running in START_SYNC mode, for DFU and generic USB we will probably need a big help from the community to make it happen.

Yes, but I wasn't talking about compatibility with existing recipes, I was asking: How should a modern recipe (not intended to rely on this backward compatible {serial.port.file}) access the ttyACM0 version. Most likely this is something for the serial discovery tool, which should return name: ttyAMC0 or so in the prefs/uploadProperties, so not something to change in this RFC (except maybe in the examples).

If you tell me I would say: you must fix it in the upload tool (to make it accept /dev/ttyACM0 instead of ttyACM0), but yes, adding another property in the prefs set is a no-brainer and solve a lot of problems with minimal effort.
So 👍🏼 I'm going to add it to the discovery and change the example in the RFC.

Yeah, but how would port matching and selection work for programmers? Currently, the selected board serial port also selects the programmer port to use, for serial programmers, while non-serial-based (e.g. native USB) programmers typically ignore the port selection and just select the first available device based on e.g. vidpid matching internal to the tool (which can be problematic when you have multiple programmers attached, which is not so unlikely when using boards like the STM32 Nucleo/discovery, which have an ST-link programmer integrated, or the zero which has a CMSIS-DAP programmer integrated IIRC). It would be nice if this generalization of the port concept could also be applied to programmers (which would then need to define their own port_match properties, I guess?).

mmm not sure I get this one. At the moment the port selection is independent of the action you do later.
Just to clarify, the "action" can be:

  1. upload
  2. upload with programmer
  3. burn bootloader

what changes between these actions is the recipe that is run to perform the action itself, to be precise, the recipe used are respectively pointed by:

  1. board.upload.tool=RECIPEID
  2. board.program.tool=RECIPEID
  3. board.bootloader.tool=RECIPEID

these definitions may be overridden by selecting a programmer from the programmer.txt so, for example, if you need a different command line for the ST-Link you can write:

[in programmers.txt]
stlink.name=ST-Link
stlink.program.tool=openocd-stlink

For extra clarity let me try to write an example that combines everything together:

[in boards.txt]
board1.name=Board One
board1.upload.tool.serial=bossac
board1.upload.tool.network=arduino_ota
board1.bootloader.tool.serial=openocd
board1.program.tool.serial=openocd
board1.program.tool.dfu=openocd-dfu

[in platform.txt]
tools.bossac.upload.pattern=...(serial upload)...

tools.arduino_ota.upload.pattern=...(network upload)...

tools.openocd.program.pattern=...(serial upload with programmer)...

tools.openocd.bootloader.pattern=...(serial burn bootloader)...

tools.openocd-dfu.program.pattern=...(dfu upload with programmer)...

anyway in each recipe you will always have the port properties available under the prefix upload.* (so {upload.address}, {upload.port.vid}, etc.). So it seems to me that there is enough expressivity to handle all the cases, but maybe not? could you give an example that may not fit here?

BTW great feedback 👍🏼 you're basically stress testing the specification that is really a good thing.

I'll go ahead and amend the RFC with the suggestion above.

@PaulStoffregen
Copy link

PaulStoffregen commented Apr 29, 2021

About the communication from IDE / CLI to the discovery utility, could we add a required (sent by the IDE) initial message to inform the discovery utility which software & version is used? And if that initial ID message is not transmitted, the discovery utility would assume it is being used by Arduino IDE 1.8.13. If the message is present but unparseable or an unknown version, the discovery utility would assume this RFC.

As an implementer of a 3rd party discovery utility, of course my main wish is for the protocol to be perfectly stable and forever backwards compatible. But if reality does not turn out to be so kind, knowing which IDE and which version will be receiving my JSON output would let me transmit "prefs" when used with IDE 1.8.13 and "portProperties" when working with 1.8.14 or 2.0, and perhaps tailor the JSON as needed when the protocol is modified in the future, or as future IDEs might use of the same protocol in slightly different ways.

@matthijskooijman
Copy link
Contributor

Regarding the port_match proposal, I would suggeste replacing port_match (which I did not really like much anyway), maybe use upload_port? e.g. myboard.upload_port.0.vid=0x1234? This makes a bit more explicit what port is matched, and also (in the future) allows adding e.g. myboard.monitor_port.0.vid=0x1234 to apply the same matching algorithm to matching the serial monitor port, in case the upload port and monitor port are different.

instead yes! (like, just to say, a network host that is called exactly COM1... very unlikely to happen but still possible)
As I said the remove event must report the protocol and the address, this is an error in the RFC that I must fix.

Right, good motivating example. Agreed.

the builtin discoveries (serial and network) are shared between all platforms by default

If we add a way to include discoveries from other platforms, it might even be nice to also apply this to builtin discoveries, so a platform must explicitly include the builtin discoveries it needs. This prevents "polluting" the port namespace with discoveries that are not needed at all (i.e. AVR has no need for network, I think), and also makes it easier to (maybe temporarily) replace a builtin discovery (i.e. using a customized version of the serial discovery that adds extra properties) without having to use a different upload protocol. There is the question of backward compatibility, though, how to distinguish between new platforms that need no builtin discoveries and old platforms that rely on the current behavior of implicitly making serial (and I guess also network?) ports available? Maybe this can be done based on whether boards use the upload.tool or upload.PROTOCOL.tool syntax? (this is where platform.txt-spec-versioning suggested in arduino/arduino-cli#985 (comment) would have been helpful)

If you tell me I would say: you must fix it in the upload tool (to make it accept /dev/ttyACM0 instead of ttyACM0),

Yeah, fixing this in the tool would be obvious, but when using third-party tools, this is not always easy (and adding a wrapper script is always possible, but extra effort because of cross-platform).

but yes, adding another property in the prefs set is a no-brainer and solve a lot of problems with minimal effort.
So 👍🏼 I'm going to add it to the discovery and change the example in the RFC.

Tnx

for DFU and generic USB we will probably need a big help from the community to make it happen.

Yeah, I feel the limited time thing all too well. I would be motivated to give these a stab, except that I really shouldn't be taking on new big projects until I finish a lot of my existing projects first... But just serial and network would indeed be a good start, then maybe third-party cores could implement others, which could be imported as builtins at some point later, when they're proven.

mmm not sure I get this one. At the moment the port selection is independent of the action you do later.

I think this might actually be a problem, since this approach was chosen when regular uploads and programmer uploads could only use serial ports, so sharing the same selection was pragmatic (not conceptually correct, and also impractical in same cases, i.e. arduino/Arduino#5554). However, when expanding beyond serial ports, this becomes problematic.

Consider you have two Unos attached, and each has an external USB-ASP programmer attached. So, you select the "Uno" board to program, which offers two ports for uploading: The ACM serial ports of both Unos. However, when I then select "Upload using programmer", neither of these ports is applicable, instead I should be offered both USB-ASP devices.

This is currently "solved" by letting programmers like the USB-ASP simply ignore the selected (serial) port and just pick the first USB-ASP device they see (based on vid/pid filtering internal to avrdude), but this is a hack at best, and something that would be solvable within the context of this discussion.

What I think would be needed, is that:

  • The port selection is specific to the action you intend to do later (regular upload or upload using programmer).
  • For regular upload, the port matching/filtering happens based on the selected board, for programmer upload, it happens based on the selected programmer.
  • Programmers in programmers.txt can specify a port_match just like boards, to select the ports that are valid for this programmer.

Also, you give this example:

[in boards.txt]
board1.name=Board One
board1.upload.tool.serial=bossac
board1.upload.tool.network=arduino_ota
board1.bootloader.tool.serial=openocd
board1.program.tool.serial=openocd
board1.program.tool.dfu=openocd-dfu

AFAICS, the board1.program.tool.xxx lines are an addition, right, these are not currently used? I wonder if those make sense, though, since this now seems to require that you list available programmer upload tools for each board separately? Looking at the AVR core, it defines 15 different programmers already, each of which define their own tool (all 15 use avrdude, but these could be many different ones as well). So I'm unsure how these lines in your examples are intended.

One additional thing to note: There is currently not any way to specify which programmers are applicable to which boards. Originally, all programmers in all platforms where made available for all boards, including completely incompatible combinations. Since arduino/Arduino#9900, only programmers from the same platform as the selected board (and maybe a referenced platform). In practice this works well enough, since typically all boards in a single platform use the same programmer technology (i.e. all ISP programmers work on all AVR boards). It might be useful to make this more explicit, i.e. for platforms where multiple programmer protocols exist and not all boards support all protocols, but maybe this can be left undefined (and you just rely on the user to know which programmer they have and for which boards it is suitable).

BTW great feedback 👍🏼 you're basically stress testing the specification that is really a good thing.

Thanks, that helps motivate me to free up some time for this (which I consider an import step in making the Arduino ecosystem a bit more generic in terms of supported devices).

@PaulStoffregen said:

About the communication from IDE / CLI to the discovery utility, could we add a required (sent by the IDE) initial message to inform the discovery utility which software & version is used?

I agree that this would be useful, this is something that should be part of any protocol, I think (weird that I did not realize this in my initial review :-p). However, I wonder if the software and version itself is sufficient, or whether a protocol version should be included instead? Checks for software versions can be ok, but are not really scalable. Consider for example the -DARDUINO used in compilation now, which specifies the IDE version, but is essentially meaningless when using arduino-cli, so then you end up using a "Compatible with version"-type of thing. This is partly mitigated by specifying the software and version, but even then, version checks in tools become outdated when new tools are introduced.

In that light, I would suggest adding a protocol version (probably in addition to software+version, which can also be useful to account for specific quirks). The protocol that is currently in use could be v1, which is assumed when no protocol is advertised, the RFC under discussion could then be v2.

@PaulStoffregen
Copy link

A protocol version number is a good idea for the future.

As the maintainer of a board package, I also want info about which software and which version, similar to the User-Agent info web browsers transmit. The anticipated use case is crafting a workaround for issues in specific versions of the IDE. It is messy, much like how many websites have special code for Internet Explorer. I hope to never need this. But if any particular IDE version needs a special workaround, I will go to great lengths to provide a smooth experience to my users. Alternative ways to deduce the IDE version are much worse.

@cmaglie
Copy link
Member Author

cmaglie commented May 27, 2021

@matthijskooijman @PaulStoffregen
I've pushed a bunch of fixes to the RFC.
It took quite a while but it turned out very well, I hope I've addressed all the concerns with this revision.

Even if the specification is not 100% complete, I see that it's starting to settle down. I would like to start implementing it from next week, maybe beginning from the protocol implementation that is the part of the RFC that seems more "stable" and leaving the parts that are most discussed, like installation and upload, as last so we can iterate a bit more.

@PaulStoffregen
Copy link

PaulStoffregen commented May 28, 2021

Yes, agreed, time for implementation.

Is it still too soon to talk of pluggable serial monitor? Or pluggable visualization monitor?

Copy link
Contributor

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

I gave the entire spec another read through, looking better and better :-)

I agree that starting to implement is probably good. There's still some points that might need more work (I'm thinking of protocolLabel and the potential need for QUERY), but having an implementation will probably give more insights in those.

I also left some more inline comments about the discoveryDependencies proposal, which I'm not quite happy with, but I'm not sure how to do it better right now. I do feel the urge to figure out something that is a bit cleaner and less complex, since once we add the extra complexity, we'll end up supporting it for a long time...

RFCs/0002-pluggable-discovery.md Outdated Show resolved Hide resolved

or

- The upload port is specified but the protocol of the selected port doesn't match any of the available upload protocols for a board
Copy link
Contributor

Choose a reason for hiding this comment

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

Falling back to no-port seemse like it could end up being confusing, but maybe we should just look how it will work in implementation first, and the (re)consider this again later (this is just a UI implementation detail, not something set in stone).

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I renamed it default it's much more straightforward I think but, I agree, we will found out after the first implementation...

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that default is indeed more matching with the specified behavior, but I do wonder if having a "default" upload tool actually makes sense. It suggests that it can handle ports with different protocols, which I would expect to never really be the case (since different types of ports, i.e. different protocols specify their address and other info in different formats, so the tool recipes must be matched to that). In that sense, a "no port" is more descriptive and explicit, since it names a tool that needs no port data at all (and that can also be enforced by not exposing any such data to the recipe, which no longer makes sense with default).

I do think that, ideally, no-port entries are really a temporary thing, since with the proper discovery tools, I think all upload tools should be able to select a port explicitly (i.e. you can specify the USB device path to openocd), though I can imagine that in practice that some tools will remain no-port indefinitely.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact is that no-port, as specified here, actually will forward port information if a port is selected (and that would be the case 99% of the time if you use an IDE, you cannot "unselect" a port once you have selected once even if the port disappears).

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact is that no-port, as specified here, actually will forward port information

That's an implementation choice, and I would think it would be worth considering not to forward any port data to a no-port entry (to make it clearer what the intent is and to prevent creating situations that do not make complete sense but need to be supported indefinitely later).

if you use an IDE, you cannot "unselect" a port once you have selected once

This is also an implementation choice. A port menu could have a "No port" entry. Also, the offered port list could be filtered based on the (protocols supported by the) selected port even.

I'm not entirely sure this is the best way forward, but do think that choices made in the current implementation should not limit this RFC.

RFCs/0002-pluggable-discovery.md Outdated Show resolved Hide resolved
@cmaglie cmaglie merged commit 23cfef6 into main Jun 21, 2021
@cmaglie cmaglie deleted the pluggable-discovery-rfc branch June 21, 2021 14:23
@cmaglie
Copy link
Member Author

cmaglie commented Jun 21, 2021

I'm closing this RFC as-is for now, if we found that we need to change something we will release an updated version later after the base implementation is done.

@cmaglie
Copy link
Member Author

cmaglie commented Jun 21, 2021

Is it still too soon to talk of pluggable serial monitor? Or pluggable visualization monitor?

No, it's not too soon, I'd like to write down something in the coming days.
@PaulStoffregen I know that you already implemented a pluggable monitor for the Arduino IDE, I think that you already faced some critical issues. If you have insights or suggestions, may you open a new issue in this repo and write it down so we can start discussing it?

@PaulStoffregen
Copy link

If you have insights or suggestions, may you open a new issue in this repo and write it down so we can start discussing it?

Ok, here it is. #4

@PaulStoffregen
Copy link

PaulStoffregen commented Feb 3, 2022

@cmaglie - How would you feel about extending Board Identification to allow JSON properties to match the menu options, so arduino-cli could know more of the FQBN?

VENDOR:ARCHITECTURE:BOARD_ID[:MENU_ID=OPTION_ID[,MENU2_ID=OPTION_ID ...]]

As I understand Board Identification, today arduino-cli can at best report the "VENDOR:ARCHITECTURE:BOARD_ID" portion of FQBN, because the spec only defines board.txt entries for matching the main part of FQBN.

If the Board Identification part of spec were extended slightly to specify boards.txt entries such as

{boardname}.menu.{menuname}.{menuoption}.upload_port.{identifier}=Value
{boardname}.menu.{menuname}.{menuoption}.upload_port.#.{identifier}=Value

then a discovery tools capable of detecting which menu options where used could report properties to match those lines. Future arduino-cli could use this property matching to give a more complete FQBN for the detected hardware. Then in an even farther future, the IDE could utilize the more specific FQBN to initialize those menu options when a user clicks the port+board from the toolbar's drop-down list.

@cmaglie
Copy link
Member Author

cmaglie commented Feb 14, 2022

@cmaglie - How would you feel about extending Board Identification to allow JSON properties to match the menu options, so arduino-cli could know more of the FQBN?

Looks reasonable, I'll add this proposal to my backlog.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants