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: search in platform.txt (WIP) #8038

Merged
merged 28 commits into from Mar 7, 2019

Conversation

5 participants
@cmaglie
Copy link
Member

commented Sep 28, 2018

/cc @PaulStoffregen
this is very WIP, but should be a good starting point.

This PR changes the DiscoveryManager so it search for custom discovery in the platform.txt as explained in #7942 (comment), you can try it by editing an existing platform.txt.

The DiscoveryManager then runs a PluggableDiscovery that is a stub for now but, once implemented, it should start the custom discovery tool and handle all the communication with it. If you would like to try to implement this it would be great, I'll be 100% on IDE and CLI next week (so hopefully we should make good progress on this).

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2018

Thanks. I will work on a PluggableDiscovery implementation over the weekend.

How to push commits to this pull request is probably well beyond my limited knowledge of git & github, but I can certainly work on the Java code and update the SerailDiscovery tool.

@arduino arduino deleted a comment from ArduinoBot Sep 28, 2018

@arduino arduino deleted a comment from ArduinoBot Sep 28, 2018

@facchinm

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

@ArduinoBot build this please

DNS problems on our side, no problem with the PR 😉

@arduino arduino deleted a comment from ArduinoBot Sep 28, 2018

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Sep 29, 2018

I started an implementation of PluggableDiscovery.java. Here is the code so far:

https://github.com/PaulStoffregen/Arduino/blob/acb22b5d644ca44eca5e3a540c88d7fd68de8cfe/arduino-core/src/cc/arduino/packages/discoverers/PluggableDiscovery.java

Main question: Should I create a new class for the JSON parsing, rather than using BoardPort? And add code to translate it to BoardPort for use by DiscoveryManager? Or is the idea to change BoardPort to match the desired format?

Here's a quick screenshot with PluggableDiscovery.java using SerialDiscovery_JSON:

screen

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2018

@cmaglie - Should I add another class to use as the JSON format?

// Discovery tools not supporting START_SYNC require a periodic
// LIST command. A second thread is created to send these
// commands, while the run() thread above listens for the
// discovery tool output.

This comment has been minimized.

Copy link
@cmaglie

cmaglie Oct 1, 2018

Author Member

Not sure if this is the best way: we can call "LIST" just once directly in listDiscoveredBoards() (basically it will poll only when the Tools menu is triggered and not every 2.5 sec). On the other hand this may add some delay on opening the menu unless there is some internal cache in the discovery tool.

This comment has been minimized.

Copy link
@PaulStoffregen

PaulStoffregen Oct 3, 2018

Collaborator

We already do have the internal cache as the portList of discovered ports in PluggableDiscovery. listDiscoveredBoards() just returns a copy of the list. At least that part is easy.

To make this work, I believe DiscoverManager would need to give all discoverer instances an early notice that listDiscoveredBoards() may soon be called. Maybe this could be done by detecting mouse hover over the Tools menu? Or maybe mouse-over any part of the menus?

Early versions of Teensy's software waited in listDiscoveredBoards(). The result was usually ok, but some Windows & Mac users had a terrible experience with the GUI becoming unresponsive. I believe it's best to always quickly return the list of previously discovered ports.

@cmaglie

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

Very nice!

Should I add another class to use as the JSON format?

you mean to allow passing some more metadata?
yes, something like PluggableDiscoveryMessage should do I think.

@@ -47,6 +47,18 @@ public BoardPort() {
this.prefs = new PreferencesMap();
}

public BoardPort(BoardPort bp) {
prefs = new PreferencesMap();
// TODO: copy bp.prefs to prefs

This comment has been minimized.

Copy link
@cmaglie

cmaglie Oct 1, 2018

Author Member

new PreferencesMap(bp.prefs) should do the job

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2018

you mean to allow passing some more metadata?
yes, something like PluggableDiscoveryMessage should do I think.

Two issues.

1: Yes, metadata for the event type "add", "remove", "update" (or just "add" again with a label/address already used), "error" (currently only START_SYNC not supported). But this could be also be done easily with just 1 extra variable in BoardPort.

2: Currently BoardPort is nothing like the JSON format you described. Adding another class could give the JSON format you want without changes to BoardPort, but extra code & complexity. Using only BoardPort is simpler, but BoardPort is widely used by the rest of the IDE, so many edits needed for any changes to BoardPort.

So far, I have not tried to change BoardPort, other than adding the copy constructor.

@cmaglie

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

At the moment BoardPort is defined as:

  private String address;
  private String protocol;
  private String boardName;
  private String vid;
  private String pid;
  private String iserial;
  private String label;
  private final PreferencesMap prefs;
  private boolean online;

I'd like to change it and move "fixed" fields like vid and pid inside prefs (and change where they are used with the corresponding map get).
For the online field I'm unsure I need to see where it is used and if it can be removed.
In the end the BoardPort should be something like:

  private String protocol;
  private String address;
  private String label;
  private final PreferencesMap prefs; // should be really called `props`, but let's keep it as is for now
  private final PreferencesMap identificationPrefs;
  // private boolean online; // Maybe?

If we reach this point we can change also the board detector so it can match the board model using the identificationPrefs and the boards.txt content.

Does the Teensy hid use some identifiers similar to vid and pid?

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2018

I'd like to change it and move "fixed" fields like vid and pid inside prefs (and change where they are used with the corresponding map get).

I'll start working on this. Do you want it to be on this pull request?

If we reach this point we can change also the board detector so it can match the board model using the identificationPrefs and the boards.txt content.

I do not have a clear understanding how you want identificationPrefs to work. My guess is you're imagining adding a separate class or package for board detection? Maybe you're thinking identificationPrefs info would be the input to this board detection class?

So far, I have understood the board detection process happens within each discoverer instance. Both SerialDiscovery and NetworkDiscovery have this detection process built in. So I have imagined any board detection logic for PluggableDiscovery would occur within the external discovery tool and be communicated to the PluggableDiscovery instance as JSON fields.

I'm imagining identificationPrefs would be the output of board detection logic. Rather than leaving the rest of the IDE to struggle with how VID, PID, HID descriptors, bluetooth info and other arbitrary info matches, all that info-to-board logic would be inside each discoverer. Then identificationPrefs would have fields the rest of the IDE could readily use without guesswork, like the board name.

Before I do any work with identificationPrefs, I need to clearly understand how you want it to work. Where the properties-to-identification logic will exist is the critical decision.

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2018

In the end the BoardPort should be something like: ...

I added a commit to move closer to this goal. Moving vid, pid, serial number into prefs is easy, because everything uses the getter & setting functions. I've updated SerialDiscovery_JSON. Now it looks like this:

{
  "eventType": "add",
  "address": "_/dev/ttyUSB0",
  "label": "/dev/ttyUSB0 (FT232R USB UART)",
  "boardName": "FT232R USB UART",
  "prefs": {
    "vendorId": "0403",
    "productId": "6001",
    "serialNumber": "A800I6T1"
  },
  "protocol": "Serial Device"
}
{
  "eventType": "add",
  "address": "_/dev/ttyACM0",
  "label": "/dev/ttyACM0 (USB Serial)",
  "boardName": "USB Serial",
  "prefs": {
    "vendorId": "16c0",
    "productId": "0483",
    "serialNumber": "3990740"
  },
  "protocol": "Serial Device"
}

PluggableDiscovery is no longer using the "online" boolean. But it seems to be used by SerialBoardsLister. I don't fully understand it, but the use appears to be related to making a port inaccessible during upload, and perhaps handling the case where the same board reappears as a different port shortly after the upload completes. Pretty sure this can't be removed from BoardPort without breaking stuff.

Not sure what to do with boardName. Need your opinion on how you want identificationPrefs to work and where you envision the properties-to-board logic implemented. My hope for PluggableDiscovery would be to put that logic into the discovery tool and send the board info as JSON fields. But if you're planning to add another package or some other layer to do that stuff, need to understand how you want it to work.

@cmaglie

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

I do not have a clear understanding how you want identificationPrefs to work. My guess is you're imagining adding a separate class or package for board detection? Maybe you're thinking identificationPrefs info would be the input to this board detection class?

exactly, let me expand a bit on this one, the idea is really simple:

  • each board should have the discovery information written in the boards.txt as generic properties, for example the Arduino Uno has:
uno.name=Arduino/Genuino Uno
uno.vid.0=0x2341
uno.pid.0=0x0043
uno.vid.1=0x2341
uno.pid.1=0x0001
uno.vid.2=0x2A03
uno.pid.2=0x0043
uno.vid.3=0x2341
uno.pid.3=0x0243
  • each discovery will report the discovered port with all the identification information, for example (slightly changing the output of your Serial Discovery) a report may be:
{
  "eventType": "add",
  "address": "_/dev/ttyACM0",
  "label": "/dev/ttyACM0 (USB Serial)",
  "boardName": "USB Serial",
  "prefs": {
    "vendorId": "0x2341",
    "productId": "0x0043",
    "serialNumber": "3990740"
  },
  "identificationPrefs": {
    "vid": "0x2341",
    "pid": "0x0043"
  }
  "protocol": "Serial Device"
}
  • the hypothetical BoardPortMatcher will scan all boards.txt searching for the board that match the identificationPrefs. In other words, looking at the example above, the matcher will search for a board that has the propertes vid.x and pid.x set respectively to 0x2341 and 0x0043 (in this case the Arduino Uno will be matched).

  • this example has vid and pid but it really may be anything... macaddressprefix, mdsntype... oranges and apple :-) it just needs the discovery to provide these info and the matcher will do the rest.

So far, I have understood the board detection process happens within each discoverer instance. Both SerialDiscovery and NetworkDiscovery have this detection process built in. So I have imagined any board detection logic for PluggableDiscovery would occur within the external discovery tool and be communicated to the PluggableDiscovery instance as JSON fields.

Doing this will not scale well, for example the discovery will not match 3rd party boards (for example the serial discovery will match only Arduino boards because they are the only one known to him), unless we find a way to pass the content of each installed boards.txt to the discovery tool, but this seems to complicate things IMHO.

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 2, 2018

Oh, ok, I believe I understand now. Will try to add this later today.

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2018

I added identificationPrefs and a function which searches for a board which matches all the identificationPrefs fields. Hopefully this is close to what you were thinking?

So far, it's only actually being used within PluggableDiscovery to set the board name.

@arduino arduino deleted a comment from ArduinoBot Oct 3, 2018

@arduino arduino deleted a comment from ArduinoBot Oct 3, 2018

@arduino arduino deleted a comment from ArduinoBot Oct 3, 2018

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2018

@cmaglie - Is there anything more you wanted to see done here? Other than when to send "LIST", not sure what more I can do here?

Maybe getting close to being able to merge this?

@arduino arduino deleted a comment from ArduinoBot Oct 4, 2018

@cmaglie

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

The algorithm is correct, but I'd like to move it away from the BoardPort class.
BTW I'm on it now, will report back soon.

@cmaglie

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

The SerialDiscovery_JSON repo is updated with the version you used to test?

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2018

Yes, SerialDiscovery_JSON is up to date. The last change added identificationPrefs, so if you see that in the JSON output you can be sure you've got the latest.

@cmaglie cmaglie force-pushed the cmaglie:pluggable-discovery branch from 095bfb4 to 5fc0eae Oct 4, 2018

@arduino arduino deleted a comment from ArduinoBot Oct 4, 2018

@arduino arduino deleted a comment from ArduinoBot Oct 4, 2018

@cmaglie cmaglie force-pushed the cmaglie:pluggable-discovery branch from c4d2f51 to e1caaf1 Jan 23, 2019

@facchinm facchinm merged commit 0e45f4e into arduino:master Mar 7, 2019

@facchinm facchinm deleted the cmaglie:pluggable-discovery branch Mar 7, 2019

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2019

Trying to work with Pluggable Discovery in just-released 1.8.9. Quick question about how I'm supposed to write the pattern in platform.txt.

What I want to do is something like this:

discovery.teensy.pattern="{runtime.hardware.path}/tools/teensy_ports" -J2

The problem is {runtime.hardware.path} doesn't get replaced with the pathname to the files I've installed. I believe there are 2 issues. First, discoverers are started very early, before the runtime.hardware.path pref exists. Second, even if it did exist, this refers to the path for the currently selected board, which wouldn't let my discoverer start unless the user had selects the board and then quits & restarts the IDE.

I was able to make it work with this:

discovery.teensy.pattern="{runtime.ide.path}/hardware/tools/teensy_ports" -J2

But this pattern will only work if my platform is installed inside the IDE, at that specific location.

I did write a patch for DiscoveryManager.java to create a special PreferencesMap with the platforms paths and then do a 2nd StringReplacer.replaceFromMapping() on the pattern. That works great... but patching DiscoveryManager.java can't be the proper answer!

Hoping you can tell me how you intended for the pattern to be written for the path a platform-provided discoverer?

@cmaglie

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

The idea is to deliver the discovery as a board-manager tool and use {runtime.tools.TOOLNAME.path} or {runtime.tools.TOOL_NAME-TOOL_VERSION.path} (as explained here and here)

BTW we could allow also {runtime.hardware.path}, the problem of the selected board may be solved by setting the variable to the platform path that is currently scanned.

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Mar 18, 2019

Ah, ok, now I understand. The problem comes because I am just copying my platform stuff into the hardware folder, not using a JSON index file.

@arduino arduino deleted a comment from ArduinoBot Mar 18, 2019

@arduino arduino deleted a comment from ArduinoBot Mar 18, 2019

@omzlo

This comment has been minimized.

Copy link

commented Mar 20, 2019

This is awesome. I've been following this new feature with great interest. I've built an IoT platform called NoCAN that identifies nodes with a special unique identifier (instead of a serial port or a ip address). After playing a bit with 1.8.9, I'm able to list all the nodes in the network in the Arduino IDE, with the Pluggable Discovery you have implemented.

I have one question though: once the user selects a port, how to I fetch the port information in my tools.....upload.pattern definition. For example, how do I pass the "address" defined for the selected port to my uploader command?

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

Use {serial.port}. So it should look something like this:

tools.yourtoolname.upload.pattern="{runtime.hardware.path}/programname" "{build.path}/{build.project_name}" "{serial.port}"

It's still called "serial.port" in the prefs, even though we're now able to do any arbitrary protocol and any "address".

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

Unfortunately, there is still no pluggable serial monitor. I have some code for Teensy which hard-codes this, using stdin/stdout pipes for the data, and stderr for status. Thinking about converting it over to use these recipes, and probably JSON instead of my ad-hoc text messages. @cmaglie - maybe that can serve as a starting point for a pluggable serial monitor in 1.8.10?

@omzlo

This comment has been minimized.

Copy link

commented Mar 20, 2019

Use {serial.port}. So it should look something like this:

tools.yourtoolname.upload.pattern="{runtime.hardware.path}/programname" "{build.path}/{build.project_name}" "{serial.port}"

It's still called "serial.port" in the prefs, even though we're now able to do any arbitrary protocol and any "address".

@PaulStoffregen This is perfect thanks! I just tried it and it works in my proof of concept.

@omzlo

This comment has been minimized.

Copy link

commented Mar 20, 2019

Hi. My POC works well with the START_SYNC mechanism, however it failed with the fallback LIST mechanism.
After looking at the code, I believe that line 146 in PluggableDiscovery.java

        BoardPort port = mapJsonNodeToBoardPort(mapper, node);

shoud be changed to

        BoardPort port = mapJsonNodeToBoardPort(mapper, portNode);

This made things work in my setup.
I hope this helps.

@facchinm

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@cmaglie what do you think about this 🔝 ?

@omzlo

This comment has been minimized.

Copy link

commented Apr 24, 2019

I've been trying to release a package that takes advantage of the new discovery features of the Arduino IDE.
My tool is called "nocanc". I run into the following issue.

The following fails in platform.txt:

discovery.nocanc.path={runtime.tools.nocanc.path}
discovery.nocanc.cmd=nocanc
discovery.nocanc.cmd.windows=nocanc.exe
discovery.nocanc.pattern="{path}/{cmd}" arduino-discovery

However, the following works (at least on Mac OSX):

discovery.nocanc.cmd=nocanc
discovery.nocanc.cmd.windows=nocanc.exe
discovery.nocanc.pattern="{runtime.tools.nocanc.path}/{cmd}" arduino-discovery

The reason seems to be that variable substitution using PreferencesData only happens on the pattern and not on the other discovery.* fields, as the code in DiscoveryManager suggests:

  String pattern = discoveryPrefs.get("pattern");
  ...
  pattern = StringReplacer.replaceFromMapping(pattern, PreferencesData.getMap());`

I believe that this will cause further issues on Windows where the cmd.windows substitution won't happen as well.
This could explain some problems previously reported by @PaulStoffregen above.

@facchinm

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@omzlo the differentiation between windows and unix, as far as the filename is the same (without the extension), is not needed anymore (at least from IDE 1.6.something).
However, I understand your point; the discoverer should try to resolve exactly as the normal platform.txt parser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.