#2164 Allow Add-ons to define new Protocols

[Slawek Mikula]

When someone is using own Hud/Protocol files, one have to copy this items to the FGDATA folder. In this way it litters the original FGDATA folder. It is cumbersome because:

• when installing new version of the flightgear one have to remember to copy this data from FGDATA from eg. 2018.3.2 to 2019.1.1
• when working on the git FGDATA version there is constantly new unversioned files lying in the FGDATA folder

In the attachment small patches which:
add FGHOME/fgdata folder as a another resource folder. It can be then searched via standard globals->resolve_aircraft_path method
modifies generic protocol handler to use above method rather than hardcoded FGROOT

Additionally using the second patch it can be possible to include protocol files in the addons.

Of course naming of the fgdata folder inside FGHOME is temporary. I don't want to select main FGHOME folder because it's original content.

[James Turner]

The change in Network::generic is fine, but the one to always search FGHome for data files makes me slightly worried.

I'd sooner make Huds / Protocols work as an add-on, I think it's not too difficult

[Slawek Mikula]

Then it can be only in generic protocol.

Regarding 'fgdata' in fghome I was looking a way for current data flow to externalize changes in fgdata. Right now I had always files, that were still lying and shown in the git as unversioned changes. Earlier when using releases, there were always work to copy custom data from one to the other version.

Using addon would be the best approach, but right now there are lots of protocol files that are lying everywhere and one have to create addon or know how to do it. Anyway, change in Network::generic can be applied, changes in resource manager can be left out.

[Slawek Mikula]

There is some small issue with this. I've created two exemplary addons:
https://github.com/slawekmikula/fg-addon-protocolkml
https://github.com/slawekmikula/fg-addon-hudheli

One thing. When Florent has implemented all of the addon infrastructure it has added explicitly addon folder for searching in resource manager. BUT it is somewhat different than normal searching. See https://sourceforge.net/p/flightgear/fgdata/ci/next/tree/Docs/README.add-ons#l436 the path has to be prepended with [addon=<addon-id>]</addon-id>.

• in HudHeli - (it is only encapsulation for Heli HUD from Alex D-HUND (http://wiki.beggabaur.rocks/ya/HeliHUD). I'm using it ocassionally because it has stick(mouse) orientation display) - i've modified xml files to prepend addon info. It is working right now.

• in ProtocolKml - only exemplary export to kml format using fgcommand initialization. In this case there is a problem with the current modification for protocol loader. I've created patch for this (lame but this is only for a case "works now"). I haven't got any smarter idea but i've to get rid of the "Protocol" adding to the file path because of this [addon=] case. Anyway with this patch the fg-addon-protocolkml works.

All of the work is also on my private branch. This change is on 'fixes' branch: https://sourceforge.net/u/slawekmikula/flightgear/ci/64ff2795aa9c119f187008c0cbaba4d44e19e7b5/

[Slawek Mikula]

@jmturner James, if it possible, can you look at my last comment/patch ? I know it's low priority but regarding protocol intergration via addons it should be something done with it. Thanks.

[James Turner]

I'm not sure, but this doesn't seem like a good solution to me - most places in the code should not care about add-ons vs normal path. But I think there's not many options, since the generic-protocol is taking a file_name ands expecting it can extend it to be a path.

So I guess, I don't really like this, but I don't have a better solution that works either :/

[Slawek Mikula]

Yep, as for now it can be left as is (maybe Florent will have some time and it is his idea with [addon= prepending - it's not a bad idea). The problem is with protocol handling text concatenation. Another solution is to get rid with appending "Protocol" in source code but it would break anything right now -> so not possible.

Right now noone is using this. I've some addons which uses this - linuxtrack, vfrflight, and not completed littlenavmap (there is small glitch on lnm side so it doesn't bother). I'm sending it anyway because there is info/working examples what can be done with addons and what is not working ATM (let's face it - core releases are longstanding at best, so it is better to let people enhance fg with their release pace).

[Slawek Mikula]

@frougon Florent if I might ask You for Your opinion on this item. Especially what arises when I tried to place protocol files inside the addon https://sourceforge.net/p/flightgear/codetickets/2164/#c126 ? The resource manager prepend addon items with addon identifier but the protocol loader forcefully prepend "Protocol" before the protocol file path specifed on the command line. What in your opinion is the right way to deal with this ?

@jmturner James, can someone change title of this issue to something like "Allow to place Protocol files in addons" ?

[James Turner]

Updating meta-data

[Florent Rougon]

Hi,

Maybe this is completely dumb, but: since README.add-ons says that “The add-on directory is added to the list of aircraft paths,” what happens if you put your protocol file inside your-addon-dir/Protocol (with a unique name to be sure nothing overrides it from an earlier path)? Does the loading (without any patch) not work then? Did you debug why? I say this because flightgear/src/Main/globals.cxx has:

class AircraftResourceProvider : public simgear::ResourceProvider
{
public:
  AircraftResourceProvider() :
 ...
  virtual SGPath resolve(const std::string& aResource, SGPath&) const
  {

  ...

  // try each aircraft dir in turn
    std::string res(aResource, 9); // resource path with 'Aircraft/' removed
    const PathList& dirs(globals->get_aircraft_paths());
    PathList::const_iterator it = dirs.begin();
    for (; it != dirs.end(); ++it) {
        SGPath p(*it);
        p.append(res);
      if (p.exists()) {
        return p;
      }
    } // of aircraft path iteration

    return SGPath(); // not found
  }
};

and:

    auto resMgr = simgear::ResourceManager::instance();
    resMgr->addProvider(new AircraftResourceProvider());

The [addon=...] "tag" is for targetting a specific add-on when the SG resource manager is used to look up a file, but in this case, as far as I understand it, you want to use an add-on to make a new protocol available—globally. So, the first thing I would try is to add Protocol/foo.xml to an add-on dir (Protocol must be a subdir of your add-on dir in this case). No need for an [addon=...] syntax. If this doesn't work, I'd like to know why.

[Slawek Mikula]

@frougon Thanks for a quick reply. I'll check it carefully when I look at the whole code, but I think your if assumption is correct but in order this to work one should:

• download an addon with e.g. /Protocol/example.xml file
• enable addon in the Qt GUI (or enable it via -addon commandline directive)
• add to the commandline correct --protocol option

My goal was to get rid of manual registration of protocols - user should download addon, activate it and it is all. Anyway I'll check and give more detailed answer.

Quick test:

• repo: https://github.com/slawekmikula/fg-addon-protocolkml
• it has kml.xml protocol in Protocol folder
• i've disabled addon protocol registration via nasal (in addon-main.nas)
• added to commandline (Qt): --generic=file,out,1,/home/user/test.kml,kml

log:
flightgear/src/Main/fg_io.cxx:95: Parse I/O channel request: generic,file,out,1,/home/user/test.kml,kml
flightgear/src/Network/generic.cxx:764: Couldn't find protocol file for 'kml.xml'

So it does not find the kml file. I think (AFAIR) your implementation of ResourceManager specially for the addon restricts the search path and it can't be right now be treated the same as aircraft folder. If I have time (today/tomorrow) i'll look into the code more carefully.

[Florent Rougon]

Slawek,

Thanks for the nice test case & description.

1) The [addon=...] requirement is when files are to be found by the AddonResourceProvider, however I don't think this is the right tool for the job here.

2) I haven't tested, but based on a closer look at the code, the reason why what I described above doesn't work (as per your report) is because AircraftResourceProvider does this:

  virtual SGPath resolve(const std::string& aResource, SGPath&) const
  {
    string_list pieces(sgPathBranchSplit(aResource));
    if ((pieces.size() < 3) || (pieces.front() != "Aircraft")) {
      return SGPath(); // not an Aircraft path
    }
  ...
  }

whereas FGGeneric::reinit() uses globals->resolve_maybe_aircraft_path("Protocol/" + file_name);. The AircraftResourceProvider can only find resources whose paths look like Aircraft/some_name/...
Given this observation, I wonder whether FG commit c48c1a06e80c3bd380852c1586e8753f2ed78b74 ("Look for protocols in aircraft paths") really does something useful. I believe it could only find things in an aircraft dir if this is the current aircraft dir, due to the CurrentAircraftDirProvider. Was this the goal? Yes, FGGlobals::set_fg_root() makes the simgear::ResourceManager look under $FG_ROOT, but this location was already searched before the aforementioned FG commit (c48c1a06).

3) I believe a good way to solve this Protocol thing would be to (possibly revert FG c48c1a06 and) add simple code in the same place as was touched by said commit (FGGeneric::reinit()), that additionally looks into:

a) addon_dir/Protocol/ for every registered add-on (use a->getBasePath() for each a in the return value of addons::AddonManager::instance()->registeredAddons()).

b) $FG_HOME/Protocol/ (I guess that would be popular, right?).

Do you think this would fly? :-)

[Slawek Mikula]

Thanks again. I think it is not exactly as You said. Of course it depends on my knowledge/analysis of the code but AFAIK:

• globals->resolve_maybe_aircraft_path does not search only in the aircraft dir :) it is something historic that resolves to the ResourceManager and it searches inside all providers. Don't know why this happens, but i've seen usage of this method in the code so that's why i've used it.

see:

• ResourceManager.cxx line 112 (SGPath ResourceManager::findPath(const std::string& aResource, SGPath aContext))
• globals.cxx - lines 176-181 (registering ResourceManagers)
• globals.cxx - line 285 - fgroot base path
• globals.css - line 381 - scenery base path

What i wanted to accomplish:

• ability to save protocol files inside fghome dir - and another files like e.g. Huds - thats why i've created patch 0001-src-Main-main.cxx-add-FGHOME-fgdata-directory-for-se.patch (in this issue) which registers fghome\fgdata as a resource provider. In this case everything can be stored there e.g. /fghome/fgdata/Protocols/ as well as /fghome/fgdata/Huds/ etc. I've decided to use additional "fgdata" folder inside fghome in order to keep this folder more clean and have separate folder for "fgdata-like" folder.
• ability to create addons with protocols and register/deregister it dynamically. In this way normal user could omit configuring commandline execution which might be for them cumbersome. In this situation I thought it would be good not to introduce into generic.cxx (or anywhere else) code for specific handling e.g. addons and use ResourceManager magic

so that's why the exemplary addon (kml export) but also other from my github account are using API to register protocol on startup. In this situation i've only discovered that addon ResourceManager is appending addon id before the search path and that's why generic.cxx reinit() is not finding the protocol files because it append Protocol/ before the protocol path given by the API call or commandline options. So if I declare e.g. "generic,file,out,1,/home/user/test.kml,[addon=com.slawekmikula.flightgear.ProtocolKml]/Protocol/kml" the path will be in the result "Protocol/[addon=com.slawekmikula.flightgear.ProtocolKml]/Protocol/kml.xml" and the addon resourcemanager won't catch it.

In my opinon it would be good to use resourcemanager magic in order to search for protocols (it can be then placed in the places handled by one of the resourcemanager implementations). This assignment "Protocol/" + file_name is a not good design when there is ResourceManager. Removing this appending will cause all of current implementation fails.

So options in my opinion are:
add this patch as mine - rather bad idea because of the hardcoded implementations for addon - it was only to provide some background and possible solution
leave previous implementation for the fg_root path (revert the mentioned commit) this will provide backward compatibility AND when the resolve does not succeed add implementation to search for EXACT protocol path in all ResourceManager providers (addon, aricraft, ...)

[Florent Rougon]

Edit: I wrote this before seeing your previous message. I know that globals->resolve_maybe_aircraft_path() does not search only in the aircraft dir, that's why I previously wrote “FGGlobals::set_fg_root() makes the simgear::ResourceManager look under $FG_ROOT”... For the rest, I'll have to think about it. You seem to want a secondary resource provider that looks into each add-on but doesn't use the [addon=...] syntax? I fear this might bring too many "found by mistake" files...

Edit 2: fix code indentation in the patch and improve the error message.

The attached patch 'generic-loader.patch' works for me. I tested with this applied to your repository:

diff --git a/addon-main.nas b/addon-main.nas
index 1c79c97..dc20356 100644
--- a/addon-main.nas
+++ b/addon-main.nas
@@ -5,14 +5,6 @@

 var main = func( addon ) {
     var root = addon.basePath;
-    var my_addon_id  = "com.slawekmikula.flightgear.ProtocolKml";
-    var my_version   = getprop("/addons/by-id/" ~ my_addon_id ~ "/version");
-    var my_root_path = getprop("/addons/by-id/" ~ my_addon_id ~ "/path");
-
-    var fdm_init_listener = _setlistener("/sim/signals/fdm-initialized", func {
-        removelistener(fdm_init_listener);
-
-        fgcommand("add-io-channel", props.Node.new( { "config": "generic,file,out,1,/home/zorba/test.kml,[addon=com.slawekmikula.flightgear.ProtocolKml]/Protocol/kml", "name":"kml"}));
-    });
-
+    var my_addon_id  = addon.id;
+    var my_version   = addon.version.str();
 }

The output with:

fgfs --addon=".../protocolkml.git" --generic=file,out,1,/tmp/test.xml,kml

is

    4.81 [INFO]:io         Parse I/O channel request: generic,file,out,1,/tmp/test.xml,kml
    4.81 [INFO]:io           protocol = generic
    4.81 [INFO]:io           medium = file
    4.81 [INFO]:io           direction = out
    4.81 [INFO]:io           hertz = 1
    4.81 [INFO]:io           file name = /tmp/test.xml

and /tmp/test.xml is written. Using globals->get_fg_home() to handle $FG_HOME/Protocol if this is deemed desirable is left as an exercise for the reader. ;-)

[Slawek Mikula]

Thanks for the patch!

As for me the patch is mostly OK - it will search all ResourceProviders for protocol files (in /Protocol subfolder). Of course it will not find it with the addon::ResourceProvider but it OK. Next steps will be to check addons directly.

One small change (AFAIK) would be to move the path checking outside the addon part. In this way we won't try to process non existing protocol file.

[Florent Rougon]

Glad to hear you like the patch, but there are two things in your reply that I don't understand :

1) What do you mean by “Next steps will be to check addons directly.”?

2) The whole of your last paragraph. :-) Which non-existent protocol file is (tentatively) being processed?

[Slawek Mikula]

Sorry, thats what I do when there is no access to correct code/computer.

Patch attached. This is a slight modification of Your patch to bail out when there is no protocol file at all with correct log message (outside the addon search routine).

One last thing is to include fghome in resourceManager (patch 0001-src-Main-main.cxx-add-FGHOME-fgdata-directory-for-se.patch). It is placed in the main.cxx (rather wrong place?) and it appends "fgdata" to fghome.

@jmturner James can you look at this and say if this is a good idea to search for e.g. "<fghome>/Prococol/ ? This attempt (registering it as a basePath to ResourceManager will cause all searches through common functions as resolve_maybe_aircraft_path to search there. Maybe this is over exaggerated and this should be not inserted into the code ? I don't have good opinion on this but i think moving additional data from fgdata folder to user configurable places (fghome) would be better when there is need for upgrading the application </fghome>

[James Turner]

I'll take a look later!

[Florent Rougon]

Hi @SlawekMikula,

1) I don't really understand why you modified my patch. If you use a non-existent protocol with my patch and have the 'network' class in your logging setup, you'll see for instance:

[WARN]:network    Couldn't find generic protocol file 'non-existent.xml'

which seems to be the expected behavior. Can you provide a way to reproduce undesirable behavior with my patch? (I'd just like to understand...)

2) I don't see your 0001-src-Main-main.cxx-add-FGHOME-fgdata-directory-for-se.patch. Maybe you've been fooled by the fact that, apparently, the SF interface only allows one attachment per comment? (hence the patch I included inline yesterday...) Apart from that, yes: James' opinion will be quite welcome.

3) You seem to prepare patches with git format-patch. IIRC, I had problems with this in the past because the output of this command is in an mbox-ish format (at least by default, I'd say), so it's targetted at programs like sendmail, for people who want to send patches as emails (which recipients can then apply with git am after saving the emails). For this reason (i.e., I had prepared “patch files” this way in the past that didn't apply cleanly with git apply due to the special formatting), in order to prepare a patch to be sent as an attachment, I now either use git diff >foo.patch or, if the code is committed, git show commit-id >foo.patch. This doesn't allow to (easily) recreate the exact same commit with the same metadata and thus the same commit id, but is enough for simply communicating a patch.

Edit: regarding 3), your “patch files” can be applied with git am /path/to/file which is quite easy too, so you may ignore this point. :-)

Edit 2: minor update to the error message since I've improved it a tiny bit in the generic-loader.patch file attached to my earlier message.

[Slawek Mikula]

@frougon, sorry my bad.
ad.1 - yes, you're right. Little more thinking on my side could be better. Your code is correct :)
ad.2 - its on the top (main issue text). There is this this patch.
ad.3 - agreed, sorry. Doing without commandline but from gui.

[Florent Rougon]

@SlawekMikula

Thanks for the precisions. Given that git am works fine with your patches, I think you may continue sending them this way. The only requirement is that people recognize that they are for git am (which is easy once one has seen the particular file naming scheme used by git format-patch by default).

I'll wait for James or other developers to comment for further processing (especially regarding what to do with $FG_HOME).

[James Turner]

Hey, some feedback:
I'm okay with Florent's patch, as an immediate fix

I'm not okay with the FG_HOME change, becuase there is (sort of!) a better way: 'additional data dirs'. Which is not really used but was assed to allow part of FGdata to be TerraSynced. In this case we'd add FG_HOME/Data or FG_HOME/<version>_Data as an additional data dir, and then, importantly, add a 'additional data dirs' resource provider which searches them before FG_ROOT.</version>

What I wonder, is if we should allow Add-ons to optionall define a Data subdir, whcih would be added to additional_data_dirs automatically when the add-on is enabled.

FLorent, do you think this would be dangerous or acceptable (in terms of file-name collisions) : it means an Add-on could still replace any file?

The /other/ option is to search 'additional data dirs' after FG_DATA: this avoids various bugs and security issues, but means you can't replace an FG_DATA file. Maybe that's better anyway?

[Florent Rougon]

Tough question... what about a middle ground?

• For each add-on, $addon_dir/data/ is searched by the last provider and therefore allows add-ons to provide files that none of the other providers has (in particular, files that don't exist in FGData).

• If the $FG_HOME feature makes people very happy, add the corresponding provider first so that they can customize things at will if needed (e.g., use their own mice.xml). I they shoot themselves in the foot, it is their problem... mainly: in order to ease the analysis of bug reports, one could make FG log the list of files in $FG_HOME/data-overrides (or whatever it's called).

The idea is that people are responsible of their overrides in $FG_HOME, but loading a few add-ons without inspecting all of their code isn't likely to create too big of a mess... hopefully. :)

The main downside I see (apart from having to add one or two more providers) is that if an add-on provides some/file.xml that does not exist in FGData, and later FGData is updated to have a possibly incompatible some/file.xml, the add-on would get broken by the FGData update (since FGData would take precedence). But that may be an acceptable price for what we gain. Plus, this would naturally allow add-ons to provide experimental features that later get integrated into the core (FGData) if deemed useful and mature enough: as soon as some/file.xml is added to FGData, it would hide the copy in the add-on. If the shadowing was intentional, the add-on should continue to work fine with the file from FGData.

[Slawek Mikula]

If I may add some main grounds about this feature. When I was using FlightGear from the release packages (compiled and installed on linux) or even some times on Windows the installation has it own fgdata.

I had some additional tools to work: LinuxTrack (head tracking), some protocols to export flight history to eg. VfrFlight.org software and additionally Heli-Hud just because it has visualization of yoke deflection (i was using mouse and it was handy to see where to move mouse especially when mouse was used for looking and control). So there was some files that went to FGData. When new FlightGear version was released one have to remember to copy all this files to new fgdata folder.

So basically this was main reason for this ticket - it original topic was about fgdata folder in fghome. But when there was nice addon infrastructure i thought about giving the same functionality to addons and convert some of the "additiona files in fgdata" to addons (some is on my github) because i think for normal Windows/Mac user talking about folder in fghome can be not easy and easier is tell them to download addon and install it via GUI (Qt) and it should work just like that.

The reasoning that is giving right now James/Florent is much more than original but this is a way to go :)

[James Turner]

I think the 'addons/Data goes last' provder makes a lot of sense.

For the next parts, there's some options:
- add the command line switch for it (--extra-data-dir=/path) and add it to additional_data_dirs this keeps it as 'advanced' feature, the user needs to arrnage versioning, etc themselves
- add a Launcher 'add extra data directory' UI to select a dir. The problem here is versioning, I'd really prefer in that case that we pick a sub-dir based on FG version.

My current thought is to do the addons/Data feature, and potentially the command line one, but not worry about the rest? It means the best way to make such functionality availaable is an add-on ,which I agree with anyway.

Does this seem ok? The good thing is it's quite easy to implement.

Florent, I am guessing the add-on would need to explicitly declare itself (in its metadata) as 'I have a Data dir to be searched'?