Improved J661 support: Difference between revisions

From FlightGear wiki
Jump to navigation Jump to search
 
(6 intermediate revisions by 4 users not shown)
Line 1: Line 1:
= Background =
This article discusses establishing interface requirements for better [[J661]] support, so that J661 and FlightGear can collaborate in a more efficient, and overall better, fashion. So that J661 may connect to a running FlightGear instance in order to use simulated flight data to drive avionics simulated by j661.
Establish the interface requirements for better [[J661]] support, so that J661 and FlightGear can collaborate in a more efficient, and overall better, fashion. So that J661 may connect to a running FlightGear instance in order to use simulated flight data to drive avionics simulated by j661.


Referring to the code in http://gitorious.org/fg/flightgear/blobs/next/src/Network/props.cxx My suggestion would be to extend the telnet ("props") module in FlightGear, so that we add a new helper class named "PropertySubscription" and then extend the class "PropsChannel" to get a new member std::vector<PropertySubscription*> Then, we'd add two new commands "subscribe" and "unsubscribe" to the telnet command handler, where "subscribe" would simply take the property tree string, create a new instance of PropertySubscription( property ) and then append it to the std::vector "subscriptions" which would be an instance specific member field. The command "unsubscribe" would then merely lookup the listener and delete it from the vector.
== Status 07/2012 ==
<del>The original patch discussed in 09/2011 is pending review in {{gitorious merge request|proj=fg|repo=flightgear|mr=28|text=merge request 28}}.</del> (now committed)
 
== Issues 03/2017 ==
{{Note|{{FGCquote
  |unlike the 'get'/set commands, the 'subscribe' command needs its argument to be an absolute path from the root, for example:
<syntaxhighlight lang="teraterm">
subscribe /sim/time/real/seconds
</syntaxhighlight>
  |{{cite web |url=http://forum.flightgear.org/viewtopic.php?p=197611#p197611
    |title=<nowiki>Re: Virtual Panel/Control from Web Browser</nowiki>
    |author=<nowiki>Kabuki</nowiki>
    |date=<nowiki>Fri Jan 10</nowiki>
  }}
}}
 
two important gotchas:
* The subscribe command does return an echo reply even in data mode. The other non retrieving does not, including unsubscribe(!).
* The ls command replies with one line per item but nothing that signals that all items are listed. If you want to use ls in data mode you have to set a timeout to the reader. <ref>{{cite web
  |url    =  https://forum.flightgear.org/viewtopic.php?p=307251#p307251
  |title  =  <nowiki> Python3 class for telnet </nowiki>
  |author =  <nowiki> jam007 </nowiki>
  |date  =  Mar 19th, 2017
  |added  =  Mar 19th, 2017
  |script_version = 0.40
  }}</ref>
 
 
}}
 
== Enhancement suggestions ==
Referring to the code in {{flightgear url|path=src/Network/props.cxx}} My suggestion would be to extend the telnet ("props") module in FlightGear, so that we add a new helper class named "PropertySubscription" and then extend the class "PropsChannel" to get a new member std::vector<PropertySubscription*> Then, we'd add two new commands "subscribe" and "unsubscribe" to the telnet command handler, where "subscribe" would simply take the property tree string, create a new instance of PropertySubscription( property ) and then append it to the std::vector "subscriptions" which would be an instance specific member field. The command "unsubscribe" would then merely lookup the listener and delete it from the vector.


The telnet-based approach is the cleanest and most portable solution without breaking backward compatibility, simply because it'd just be new commands that clients could make use of or not. However, it would probably make sense to really require each session to configure its own subscriptions (listeners actually), right? I mean, we don't want to register listeners that fire for any other sessions - only the once that actually registered the listener. Obviously, you need to respond not just with the value of the property, but also a property identifier - so that the connected client knows what property the value belongs to. And then we could either have the telnet server respond with a full "property=value" reply like:
The telnet-based approach is the cleanest and most portable solution without breaking backward compatibility, simply because it'd just be new commands that clients could make use of or not. However, it would probably make sense to really require each session to configure its own subscriptions (listeners actually), right? I mean, we don't want to register listeners that fire for any other sessions - only the once that actually registered the listener. Obviously, you need to respond not just with the value of the property, but also a property identifier - so that the connected client knows what property the value belongs to. And then we could either have the telnet server respond with a full "property=value" reply like:


/foo/bar/altitude-ft=1500  
/foo/bar/altitude-ft=1500  


(Which'd still be fairly verbose, but certainly better than right now) Obviously, that'd be the simplest method - fairly easy to implement without too much thinking... However, I think it would make sense to use a compressed form, e.g. one where a hash of the property is internally used - and also in responses, such as a 3-4 byte hash of the full property tree string, stored in hexadecimal- so that the response is more compressed and more efficient than using the full property string every time, i.e. consider: 7FEA=100
(Which'd still be fairly verbose, but certainly better than right now) Obviously, that'd be the simplest method - fairly easy to implement without too much thinking... However, I think it would make sense to use a compressed form, e.g. one where a hash of the property is internally used - and also in responses, such as a 3-4 byte hash of the full property tree string, stored in hexadecimal- so that the response is more compressed and more efficient than using the full property string every time, i.e. consider: 7FEA=100


= Requirements =
== Requirements ==
* use the existing telnet/props interface  
* Use the existing telnet/props interface  
* do not touch existing behavior
* Do not touch existing behavior
* make the protocol more efficient to get/set properties
* Make the protocol more efficient to get/set properties
* i.e. avoid polling
* I.e. avoid polling
* provide some form of push/pull support
* Provide some form of push/pull support
* use hashing
* Use hashing


= Ideas =
== Ideas ==
== Handling multiple properties ==
=== Handling multiple properties ===
* One thing that would be great would maybe to be able to have more than one parameter to the output for the client in one telnet output (for example all the properties that have been requested), but even without that, performance should be far better than with polling I think.
* One thing that would be great would maybe to be able to have more than one parameter to the output for the client in one telnet output (for example all the properties that have been requested), but even without that, performance should be far better than with polling I think.
* For your information, in the ARINC 661 protocol, the User Application concatenates several "commands" in the same buffer, denoting each specific command by its ID (and in the case of ARINC, also it's widget ID, but it's not relevant to our problem). In our User Application implementation (which happens to be coded in C++), a lot of parameters which are considered as cyclic are send regardless of their value change, because they change often. However the Server checks for their change, and in this case, it does not go further than their decoding. It turned out that the time the Server takes to decode a huge buffer and make its checks is negligible (of the order of 1 ms for example).
* For your information, in the ARINC 661 protocol, the User Application concatenates several "commands" in the same buffer, denoting each specific command by its ID (and in the case of ARINC, also it's widget ID, but it's not relevant to our problem). In our User Application implementation (which happens to be coded in C++), a lot of parameters which are considered as cyclic are send regardless of their value change, because they change often. However the Server checks for their change, and in this case, it does not go further than their decoding. It turned out that the time the Server takes to decode a huge buffer and make its checks is negligible (of the order of 1 ms for example).
Line 28: Line 58:
* supporting multiple values in one response (i.e. like CSV) will inevitably require server-side escaping and client-side "unescaping" of properties.
* supporting multiple values in one response (i.e. like CSV) will inevitably require server-side escaping and client-side "unescaping" of properties.


== Hashing ==
=== Hashing ===
* hashing is already supported by the property tree itself (see SimGear library), it is in fact used internally for storing properties, so one of the easiest ways to hashing support would be directly exposing and using the hashes computed by the property tree: http://simgear.sourceforge.net/doxygen/classSGPropertyNode.html It seems, the hash code is stored as an unsigned int (private storage), see line #01758 in http://simgear.sourceforge.net/doxygen/props_8hxx_source.html So it would just be a matter of adding the following to the public interface of the "bucket" class: inline unsigned int getHashcode() const {return hashcode;} And then add a corresponding wrapper to the top level SGPropertyNode class for accessing a node's hash. Either as a static method or as a member function using the "this" pointer. Something along these lines would then allow using hashes of properties internally, which would also reduce bandwidth requirements.
* Hashing is already supported by the property tree itself (see SimGear library), it is in fact used internally for storing properties, so one of the easiest ways to hashing support would be directly exposing and using the hashes computed by the property tree: http://simgear.sourceforge.net/doxygen/classSGPropertyNode.html It seems, the hash code is stored as an unsigned int (private storage), see line #01758 in http://simgear.sourceforge.net/doxygen/props_8hxx_source.html So it would just be a matter of adding the following to the public interface of the "bucket" class: inline unsigned int getHashcode() const {return hashcode;} And then add a corresponding wrapper to the top level SGPropertyNode class for accessing a node's hash. Either as a static method or as a member function using the "this" pointer. Something along these lines would then allow using hashes of properties internally, which would also reduce bandwidth requirements.
* I realize that this whole idea of hashing properties makes things more obfuscated, but I guess it'd still be beneficial. Also, it would be possible to implement the "subscribe" command in a fashion so that it implicitly accepts a valid property argument and automatically responds with the computed hash value for the corresponding properties. That way, client's would not need to know about (or replicate) the hashing mechanism being used in FG, rather they would just parse the server-side response after issuing a "subscribe" command and then store hash value in a client-side map, invalid/erroneous subscriptions could be handled by a hard-coded "INVALID" response instead, which'd also be easy to parse and process in the client.
* I realize that this whole idea of hashing properties makes things more obfuscated, but I guess it'd still be beneficial. Also, it would be possible to implement the "subscribe" command in a fashion so that it implicitly accepts a valid property argument and automatically responds with the computed hash value for the corresponding properties. That way, client's would not need to know about (or replicate) the hashing mechanism being used in FG, rather they would just parse the server-side response after issuing a "subscribe" command and then store hash value in a client-side map, invalid/erroneous subscriptions could be handled by a hard-coded "INVALID" response instead, which'd also be easy to parse and process in the client.
* The server would sort of confirm a valid subscription by echoing back the computed hash value for the registered property listener. From an API point of view that would also be fairly clean and actually well-encapsulated, because the hashing algorithm is contained inside FG and could even change arbitrarily with any upcoming releases, without introducing breakage.
* The server would sort of confirm a valid subscription by echoing back the computed hash value for the registered property listener. From an API point of view that would also be fairly clean and actually well-encapsulated, because the hashing algorithm is contained inside FG and could even change arbitrarily with any upcoming releases, without introducing breakage.


= Patch prototype =
== Patch prototype ==
 
It turned out to be quite simple actually (as we anticipated already), so we have a working prototype ready. In its current form, it is still rather basic (i.e. no support for multiple properties or hashing), but subscribing and unsubscribing properties does work already. I prepared hashing support but didn't actually implement it fully. So it could definitely be enhanced rather easily if you think that'd be useful. I guess, it's now a matter of coming up with a list of changes that would be useful for j661, so that we can adapt the patch accordingly and submit it to the FG repository for review/inclusion.
It turned out to be quite simple actually (as we anticipated already), so we have a working prototype ready. In its current form, it is still rather basic (i.e. no support for multiple properties or hashing), but subscribing and unsubscribing properties does work already. I prepared hashing support but didn't actually implement it fully. So it could definitely be enhanced rather easily if you think that'd be useful. I guess, it's now a matter of coming up with a list of changes that would be useful for j661, so that we can adapt the patch accordingly and submit it to the FG repository for review/inclusion.


<syntaxhighlight lang="diff">
<syntaxhighlight lang="diff">
    diff --git a/src/Network/props.cxx b/src/Network/props.cxx
diff --git a/src/Network/props.cxx b/src/Network/props.cxx
    index 7240d5c..bd014db 100644
index 7240d5c..bd014db 100644
    --- a/src/Network/props.cxx
--- a/src/Network/props.cxx
    +++ b/src/Network/props.cxx
+++ b/src/Network/props.cxx
    @@ -33,6 +33,7 @@
@@ -33,6 +33,7 @@
    #include <simgear/misc/strutils.hxx>
#include <simgear/misc/strutils.hxx>
    #include <simgear/props/props.hxx>
#include <simgear/props/props.hxx>
    #include <simgear/props/props_io.hxx>
#include <simgear/props/props_io.hxx>
    +#include <simgear/structure/exception.hxx>
+#include <simgear/structure/exception.hxx>
 
#include <sstream>
#include <iostream>
@@ -45,6 +46,12 @@


    #include <sstream>
#include "props.hxx"
    #include <iostream>
    @@ -45,6 +46,12 @@


    #include "props.hxx"
+#include <map>
+#include <vector>
+#include <string>
+
+#include <boost/functional/hash.hpp> // for property hashing (should probably be replaced eventually)
+
using std::stringstream;
using std::ends;


    +#include <map>
@@ -55,7 +62,7 @@ using std::endl;
    +#include <vector>
  * Props connection class.
    +#include <string>
  * This class represents a connection to props client.
    +
  */
    +#include <boost/functional/hash.hpp> // for property hashing (should probably be replaced eventually)
-class PropsChannel : public simgear::NetChat
    +
+class PropsChannel : public simgear::NetChat, public SGPropertyChangeListener
    using std::stringstream;
{
    using std::ends;
    simgear::NetBuffer buffer;


    @@ -55,7 +62,7 @@ using std::endl;
@@ -75,6 +82,7 @@ public:
      * Props connection class.
       * Constructor.
       * This class represents a connection to props client.
       */
       */
    -class PropsChannel : public simgear::NetChat
    PropsChannel();
    +class PropsChannel : public simgear::NetChat, public SGPropertyChangeListener
+   ~PropsChannel();
    {
        simgear::NetBuffer buffer;


    @@ -75,6 +82,7 @@ public:
    /**
          * Constructor.
      * Append incoming data to our request buffer.
          */
@@ -89,10 +97,38 @@ public:
        PropsChannel();
      */
    +    ~PropsChannel();
    void foundTerminator();
 
        /**
          * Append incoming data to our request buffer.
    @@ -89,10 +97,38 @@ public:
          */
        void foundTerminator();


    +    // callback for registered listeners (subscriptions)
+    // callback for registered listeners (subscriptions)
    +    void valueChanged(SGPropertyNode *node);
+    void valueChanged(SGPropertyNode *node);
    private:
private:
    +
+
    +    typedef std::vector<std::string> ParameterList;
+    typedef std::vector<std::string> ParameterList;
    +
+
    +
+
        inline void node_not_found_error( const string& s ) const {
    inline void node_not_found_error( const string& s ) const {
            throw "node '" + s + "' not found";
        throw "node '" + s + "' not found";
        }
    }
    +
+
    +    void error(std::string msg) {  // wrapper: prints errors to STDERR and to the telnet client
+    void error(std::string msg) {  // wrapper: prints errors to STDERR and to the telnet client
    +      push( msg.c_str() ); push( getTerminator() );
+      push( msg.c_str() ); push( getTerminator() );
    +      SG_LOG(SG_GENERAL, SG_ALERT, __FILE__<<"@" << __LINE__ <<" in " << __FUNCTION__ <<":"<< msg.c_str() << std::endl);
+      SG_LOG(SG_GENERAL, SG_ALERT, __FILE__<<"@" << __LINE__ <<" in " << __FUNCTION__ <<":"<< msg.c_str() << std::endl);
    +    }
+    }
    +
+
    +
+
    +    bool check_args(ParameterList tok, unsigned int num, const char* func) {
+    bool check_args(ParameterList tok, unsigned int num, const char* func) {
    +      if (tok.size()-1 < num) {
+      if (tok.size()-1 < num) {
    +          error(string("Wrong argument count for:")+string(func) );
+          error(string("Wrong argument count for:")+string(func) );
    +          return false;
+          return false;
    +      }
+      }
    +      return true;
+      return true;
    +    }
+    }
    +
+
    +    std::vector<SGPropertyNode_ptr> _listeners;
+    std::vector<SGPropertyNode_ptr> _listeners;
    +    typedef void (PropsChannel::*TelnetCallback) (const ParameterList&);
+    typedef void (PropsChannel::*TelnetCallback) (const ParameterList&);
    +    std::map<std::string, TelnetCallback> callback_map;
+    std::map<std::string, TelnetCallback> callback_map;
    +
+
    +    // callback implementations:
+    // callback implementations:
    +    void subscribe(const ParameterList &p);
+    void subscribe(const ParameterList &p);
    +    void unsubscribe(const ParameterList &p);
+    void unsubscribe(const ParameterList &p);
    };
};


    /**
/**
    @@ -104,6 +140,67 @@ PropsChannel::PropsChannel()
@@ -104,6 +140,67 @@ PropsChannel::PropsChannel()
          mode(PROMPT)
      mode(PROMPT)
    {
{
        setTerminator( "\r\n" );
    setTerminator( "\r\n" );
    +    callback_map["subscribe"]    =    &PropsChannel::subscribe;
+    callback_map["subscribe"]    =    &PropsChannel::subscribe;
    +    callback_map["unsubscribe"]  =  &PropsChannel::unsubscribe;
+    callback_map["unsubscribe"]  =  &PropsChannel::unsubscribe;
    +}
+}
    +
+
    +PropsChannel::~PropsChannel() {
+PropsChannel::~PropsChannel() {
    +  // clean up all registered listeners   
+  // clean up all registered listeners   
    +  for (unsigned int i=0;i< _listeners.size(); i++)
+  for (unsigned int i=0;i< _listeners.size(); i++)
    +    _listeners[i]->removeChangeListener( this  );
+    _listeners[i]->removeChangeListener( this  );
    +
+
    +}
+}
    +
+
    +void PropsChannel::subscribe(const ParameterList& param) {
+void PropsChannel::subscribe(const ParameterList& param) {
    +  if (! check_args(param,1,"subscribe")) return;
+  if (! check_args(param,1,"subscribe")) return;
    +
+
    +  std::string command = param[0];
+  std::string command = param[0];
    +  const char* p = param[1].c_str();
+  const char* p = param[1].c_str();
    +  if (!p) return;
+  if (!p) return;
    +
+
    +        SG_LOG(SG_GENERAL, SG_ALERT, p << std::endl);
+        SG_LOG(SG_GENERAL, SG_ALERT, p << std::endl);
    +        push( command.c_str() ); push ( " " );
+        push( command.c_str() ); push ( " " );
    +        push( p );
+        push( p );
    +        push( getTerminator() );
+        push( getTerminator() );
    +
+
    +        SGPropertyNode *n = globals->get_props()->getNode( p,true );
+        SGPropertyNode *n = globals->get_props()->getNode( p,true );
    +  if ( n->isTied() ) {
+  if ( n->isTied() ) {
    +      error("Error:Tied properties cannot register listeners");
+      error("Error:Tied properties cannot register listeners");
    +      return;
+      return;
    +  }
+  }
    +    if (n) {
+    if (n) {
    +        n->addChangeListener( this );
+        n->addChangeListener( this );
    +    std::stringstream hashcode;
+    std::stringstream hashcode;
    +    boost::hash<std::string> string_hash;
+    boost::hash<std::string> string_hash;
    +    hashcode << "id:" << string_hash(n->getPath()) << getTerminator();
+    hashcode << "id:" << string_hash(n->getPath()) << getTerminator();
    +    // push( hashcode.str().c_str() );  // not yet very useful
+    // push( hashcode.str().c_str() );  // not yet very useful
    +    _listeners.push_back( n ); // housekeeping, save for deletion in dtor later on
+    _listeners.push_back( n ); // housekeeping, save for deletion in dtor later on
    +        }
+        }
    +        else {
+        else {
    +      error("listener could not be added");
+      error("listener could not be added");
    +        }
+        }
    +
+
    +
+
    +}
+}
    +
+
    +void PropsChannel::unsubscribe(const ParameterList &param) {
+void PropsChannel::unsubscribe(const ParameterList &param) {
    +  if (!check_args(param,1,"unsubscribe")) return;
+  if (!check_args(param,1,"unsubscribe")) return;
    +
+
    +  try {
+  try {
    +  SGPropertyNode *n = globals->get_props()->getNode( param[1].c_str() );
+  SGPropertyNode *n = globals->get_props()->getNode( param[1].c_str() );
    +  n->removeChangeListener( this );
+  n->removeChangeListener( this );
    +  } catch (sg_exception &e) {
+  } catch (sg_exception &e) {
    +    error("Error:Listener could not be removed");
+    error("Error:Listener could not be removed");
    +  }
+  }
    +}
+}
    +
+
    +
+
    +//TODO: provide support for different types of subscriptions MODES ? (child added/removed, thesholds, min/max)
+//TODO: provide support for different types of subscriptions MODES ? (child added/removed, thesholds, min/max)
    +void PropsChannel::valueChanged(SGPropertyNode* ptr) {
+void PropsChannel::valueChanged(SGPropertyNode* ptr) {
    +  SG_LOG(SG_GENERAL, SG_ALERT, __FILE__<< "@"<<__LINE__ << ":" << __FUNCTION__ << std::endl);  
+  SG_LOG(SG_GENERAL, SG_ALERT, __FILE__<< "@"<<__LINE__ << ":" << __FUNCTION__ << std::endl);  
    +  std::stringstream response;
+  std::stringstream response;
    +  response << ptr->getPath(true) << "=" <<  ptr->getStringValue() << "\r\n"; //TODO: use hashes, echo several properties at once, use proper terminator
+  response << ptr->getPath(true) << "=" <<  ptr->getStringValue() << "\r\n"; //TODO: use hashes, echo several properties at once, use proper terminator
    +  push( response.str().c_str() );
+  push( response.str().c_str() );
    }
}


    /**
/**
    @@ -160,7 +257,7 @@ PropsChannel::foundTerminator()
@@ -160,7 +257,7 @@ PropsChannel::foundTerminator()
        const char* cmd = buffer.getData();
    const char* cmd = buffer.getData();
        SG_LOG( SG_IO, SG_INFO, "processing command = \"" << cmd << "\"" );
    SG_LOG( SG_IO, SG_INFO, "processing command = \"" << cmd << "\"" );


    -    vector<string> tokens = simgear::strutils::split( cmd );
-    vector<string> tokens = simgear::strutils::split( cmd );
    +    ParameterList tokens = simgear::strutils::split( cmd );
+    ParameterList tokens = simgear::strutils::split( cmd );


        SGPropertyNode* node = globals->get_props()->getNode( path.c_str() );
    SGPropertyNode* node = globals->get_props()->getNode( path.c_str() );


    @@ -292,7 +389,7 @@ PropsChannel::foundTerminator()
@@ -292,7 +389,7 @@ PropsChannel::foundTerminator()
                        if ( !globals->get_commands()
                    if ( !globals->get_commands()
                                  ->execute( "reinit", &args) )
                              ->execute( "reinit", &args) )
                        {
                    {
    -                        SG_LOG( SG_NETWORK, SG_ALERT,
-                        SG_LOG( SG_NETWORK, SG_ALERT,
    +                        SG_LOG( SG_GENERAL, SG_ALERT,
+                        SG_LOG( SG_GENERAL, SG_ALERT,
                                    "Command " << tokens[1] << " failed.");
                                "Command " << tokens[1] << " failed.");
                            if ( mode == PROMPT ) {
                        if ( mode == PROMPT ) {
                                tmp += "*failed*";
                            tmp += "*failed*";
    @@ -357,7 +454,7 @@ PropsChannel::foundTerminator()
@@ -357,7 +454,7 @@ PropsChannel::foundTerminator()
                        if ( !globals->get_commands()
                    if ( !globals->get_commands()
                                  ->execute(tokens[1].c_str(), &args) )
                              ->execute(tokens[1].c_str(), &args) )
                        {
                    {
    -                        SG_LOG( SG_NETWORK, SG_ALERT,
-                        SG_LOG( SG_NETWORK, SG_ALERT,
    +                        SG_LOG( SG_GENERAL, SG_ALERT,
+                        SG_LOG( SG_GENERAL, SG_ALERT,
                                    "Command " << tokens[1] << " failed.");
                                "Command " << tokens[1] << " failed.");
                            if ( mode == PROMPT ) {
                        if ( mode == PROMPT ) {
                                tmp += "*failed*";
                            tmp += "*failed*";
    @@ -386,7 +483,14 @@ PropsChannel::foundTerminator()
@@ -386,7 +483,14 @@ PropsChannel::foundTerminator()
                    mode = DATA;
                mode = DATA;
                } else if ( command == "prompt" ) {
            } else if ( command == "prompt" ) {
                    mode = PROMPT;
                mode = PROMPT;
    -            } else {
-            } else {
    +            } else if (command  == "subscribe" || command == "unsubscribe") {
+            } else if (command  == "subscribe" || command == "unsubscribe") {
    +        TelnetCallback t = callback_map[ command.c_str() ]; //FIXME: use map lookup for condition
+        TelnetCallback t = callback_map[ command.c_str() ]; //FIXME: use map lookup for condition
    +        if (t)
+        if (t)
    +                (this->*t) (tokens);
+                (this->*t) (tokens);
    +        else
+        else
    +            error("No matching callback found for command:"+command);
+            error("No matching callback found for command:"+command);
    +      }
+      }
    +      else {
+      else {
                    const char* msg = "\
                const char* msg = "\
    Valid commands are:\r\n\
Valid commands are:\r\n\
    \r\n\
\r\n\
    @@ -400,7 +504,9 @@ prompt            switch to interactive mode (default)\r\n\
@@ -400,7 +504,9 @@ prompt            switch to interactive mode (default)\r\n\
    pwd                display your current path\r\n\
pwd                display your current path\r\n\
    quit              terminate connection\r\n\
quit              terminate connection\r\n\
    run <command>      run built in command\r\n\
run <command>      run built in command\r\n\
    -set <var> <val>    set <var> to a new <val>\r\n";
-set <var> <val>    set <var> to a new <val>\r\n";
    +set <var> <val>    set <var> to a new <val>\r\n\
+set <var> <val>    set <var> to a new <val>\r\n\
    +subscribe <var>      subscribe to property changes (returns hash code for property)\r\n\
+subscribe <var>      subscribe to property changes (returns hash code for property)\r\n\
    +unscubscribe <id>  unscubscribe from property changes (id must be the property hash as returned from subscribe command)\r\n";
+unscubscribe <id>  unscubscribe from property changes (id must be the property hash as returned from subscribe command)\r\n";
                    push( msg );
                push( msg );
                }
             }
             }
    diff --git a/src/Network/props.hxx b/src/Network/props.hxx
        }
    index e55f7c1..09b5a09 100644
diff --git a/src/Network/props.hxx b/src/Network/props.hxx
    --- a/src/Network/props.hxx
index e55f7c1..09b5a09 100644
    +++ b/src/Network/props.hxx
--- a/src/Network/props.hxx
    @@ -40,7 +40,8 @@
+++ b/src/Network/props.hxx
      * FlightGear properties.
@@ -40,7 +40,8 @@
      */
  * FlightGear properties.
    class FGProps : public FGProtocol,
  */
    -      public simgear::NetChannel
class FGProps : public FGProtocol,
    +      public simgear::NetChannel,
-      public simgear::NetChannel
    +      public SGPropertyChangeListener // for subscriptions
+      public simgear::NetChannel,
    {
+      public SGPropertyChangeListener // for subscriptions
    private:
{
 
private:
 
</syntaxhighlight>
</syntaxhighlight>


= Suggested modifications and enhancements =
== Suggested modifications and enhancements ==


= Related Discussions =
== Related discussions ==
* [http://sourceforge.net/projects/j661/forums/forum/1223353/topic/4713924 Roadmap for Flightgear connection]
* [http://sourceforge.net/projects/j661/forums/forum/1223353/topic/4713924 Roadmap for Flightgear connection]
* [http://sourceforge.net/projects/j661/forums/forum/1223353/topic/4713706 0.51 beta 2 : FlightGear connection]
* [http://sourceforge.net/projects/j661/forums/forum/1223353/topic/4713706 0.51 beta 2 : FlightGear connection]


 
[[Category:Core development projects]]
[[Category: Core development projects]]
[[Category:ARINC 661]]
[[Category: ARINC661]]

Latest revision as of 15:19, 19 March 2017

This article discusses establishing interface requirements for better J661 support, so that J661 and FlightGear can collaborate in a more efficient, and overall better, fashion. So that J661 may connect to a running FlightGear instance in order to use simulated flight data to drive avionics simulated by j661.

Status 07/2012

The original patch discussed in 09/2011 is pending review in merge request 28. (now committed)

Issues 03/2017

Note
Cquote1.png unlike the 'get'/set commands, the 'subscribe' command needs its argument to be an absolute path from the root, for example:
subscribe /sim/time/real/seconds

— Kabuki (Fri Jan 10). Re: Virtual Panel/Control from Web Browser.
(powered by Instant-Cquotes)
Cquote2.png

two important gotchas:

  • The subscribe command does return an echo reply even in data mode. The other non retrieving does not, including unsubscribe(!).
  • The ls command replies with one line per item but nothing that signals that all items are listed. If you want to use ls in data mode you have to set a timeout to the reader. [1]


Enhancement suggestions

Referring to the code in https://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Network/props.cxx My suggestion would be to extend the telnet ("props") module in FlightGear, so that we add a new helper class named "PropertySubscription" and then extend the class "PropsChannel" to get a new member std::vector<PropertySubscription*> Then, we'd add two new commands "subscribe" and "unsubscribe" to the telnet command handler, where "subscribe" would simply take the property tree string, create a new instance of PropertySubscription( property ) and then append it to the std::vector "subscriptions" which would be an instance specific member field. The command "unsubscribe" would then merely lookup the listener and delete it from the vector.

The telnet-based approach is the cleanest and most portable solution without breaking backward compatibility, simply because it'd just be new commands that clients could make use of or not. However, it would probably make sense to really require each session to configure its own subscriptions (listeners actually), right? I mean, we don't want to register listeners that fire for any other sessions - only the once that actually registered the listener. Obviously, you need to respond not just with the value of the property, but also a property identifier - so that the connected client knows what property the value belongs to. And then we could either have the telnet server respond with a full "property=value" reply like:

/foo/bar/altitude-ft=1500 

(Which'd still be fairly verbose, but certainly better than right now) Obviously, that'd be the simplest method - fairly easy to implement without too much thinking... However, I think it would make sense to use a compressed form, e.g. one where a hash of the property is internally used - and also in responses, such as a 3-4 byte hash of the full property tree string, stored in hexadecimal- so that the response is more compressed and more efficient than using the full property string every time, i.e. consider: 7FEA=100

Requirements

  • Use the existing telnet/props interface
  • Do not touch existing behavior
  • Make the protocol more efficient to get/set properties
  • I.e. avoid polling
  • Provide some form of push/pull support
  • Use hashing

Ideas

Handling multiple properties

  • One thing that would be great would maybe to be able to have more than one parameter to the output for the client in one telnet output (for example all the properties that have been requested), but even without that, performance should be far better than with polling I think.
  • For your information, in the ARINC 661 protocol, the User Application concatenates several "commands" in the same buffer, denoting each specific command by its ID (and in the case of ARINC, also it's widget ID, but it's not relevant to our problem). In our User Application implementation (which happens to be coded in C++), a lot of parameters which are considered as cyclic are send regardless of their value change, because they change often. However the Server checks for their change, and in this case, it does not go further than their decoding. It turned out that the time the Server takes to decode a huge buffer and make its checks is negligible (of the order of 1 ms for example).
  • However I need to check the number of parameters which should be sent to the Client in real scenarios. I don't think it's a lot, because ther is further logic in the Client itself, and it could be possible for the Client to unsubscribe for example when a display format is not presented anymore.
  • I was also thinking of supporting several parameters (and even return values) at once, my suggestion would be to still consider hex-encoding the UINT hash and then pre-pend it to each property value, the values could then be separated using a normal comma, so that we end up with a list of CSVs.
  • In addition, it might be worthwhile to consider pre-pending a UNIX timestamp to each server-side response, so that clients get some timing information, I guess?
  • The list of CSVs sounds good to me. And I agree that having the timestamps could be useful in some usecases.
  • supporting multiple values in one response (i.e. like CSV) will inevitably require server-side escaping and client-side "unescaping" of properties.

Hashing

  • Hashing is already supported by the property tree itself (see SimGear library), it is in fact used internally for storing properties, so one of the easiest ways to hashing support would be directly exposing and using the hashes computed by the property tree: http://simgear.sourceforge.net/doxygen/classSGPropertyNode.html It seems, the hash code is stored as an unsigned int (private storage), see line #01758 in http://simgear.sourceforge.net/doxygen/props_8hxx_source.html So it would just be a matter of adding the following to the public interface of the "bucket" class: inline unsigned int getHashcode() const {return hashcode;} And then add a corresponding wrapper to the top level SGPropertyNode class for accessing a node's hash. Either as a static method or as a member function using the "this" pointer. Something along these lines would then allow using hashes of properties internally, which would also reduce bandwidth requirements.
  • I realize that this whole idea of hashing properties makes things more obfuscated, but I guess it'd still be beneficial. Also, it would be possible to implement the "subscribe" command in a fashion so that it implicitly accepts a valid property argument and automatically responds with the computed hash value for the corresponding properties. That way, client's would not need to know about (or replicate) the hashing mechanism being used in FG, rather they would just parse the server-side response after issuing a "subscribe" command and then store hash value in a client-side map, invalid/erroneous subscriptions could be handled by a hard-coded "INVALID" response instead, which'd also be easy to parse and process in the client.
  • The server would sort of confirm a valid subscription by echoing back the computed hash value for the registered property listener. From an API point of view that would also be fairly clean and actually well-encapsulated, because the hashing algorithm is contained inside FG and could even change arbitrarily with any upcoming releases, without introducing breakage.

Patch prototype

It turned out to be quite simple actually (as we anticipated already), so we have a working prototype ready. In its current form, it is still rather basic (i.e. no support for multiple properties or hashing), but subscribing and unsubscribing properties does work already. I prepared hashing support but didn't actually implement it fully. So it could definitely be enhanced rather easily if you think that'd be useful. I guess, it's now a matter of coming up with a list of changes that would be useful for j661, so that we can adapt the patch accordingly and submit it to the FG repository for review/inclusion.

diff --git a/src/Network/props.cxx b/src/Network/props.cxx
index 7240d5c..bd014db 100644
--- a/src/Network/props.cxx
+++ b/src/Network/props.cxx
@@ -33,6 +33,7 @@
#include <simgear/misc/strutils.hxx>
#include <simgear/props/props.hxx>
#include <simgear/props/props_io.hxx>
+#include <simgear/structure/exception.hxx>

#include <sstream>
#include <iostream>
@@ -45,6 +46,12 @@

#include "props.hxx"

+#include <map>
+#include <vector>
+#include <string>
+
+#include <boost/functional/hash.hpp> // for property hashing (should probably be replaced eventually)
+
using std::stringstream;
using std::ends;

@@ -55,7 +62,7 @@ using std::endl;
  * Props connection class.
  * This class represents a connection to props client.
  */
-class PropsChannel : public simgear::NetChat
+class PropsChannel : public simgear::NetChat, public SGPropertyChangeListener
{
     simgear::NetBuffer buffer;

@@ -75,6 +82,7 @@ public:
      * Constructor.
      */
     PropsChannel();
+    ~PropsChannel();

     /**
      * Append incoming data to our request buffer.
@@ -89,10 +97,38 @@ public:
      */
     void foundTerminator();

+    // callback for registered listeners (subscriptions)
+    void valueChanged(SGPropertyNode *node);
private:
+
+    typedef std::vector<std::string> ParameterList;
+
+
     inline void node_not_found_error( const string& s ) const {
         throw "node '" + s + "' not found";
     }
+
+    void error(std::string msg) {  // wrapper: prints errors to STDERR and to the telnet client
+       push( msg.c_str() ); push( getTerminator() );
+       SG_LOG(SG_GENERAL, SG_ALERT, __FILE__<<"@" << __LINE__ <<" in " << __FUNCTION__ <<":"<< msg.c_str() << std::endl);
+    }
+
+
+    bool check_args(ParameterList tok, unsigned int num, const char* func) {
+       if (tok.size()-1 < num) {
+          error(string("Wrong argument count for:")+string(func) );
+          return false;
+       }
+       return true;
+    }
+
+    std::vector<SGPropertyNode_ptr> _listeners;
+    typedef void (PropsChannel::*TelnetCallback) (const ParameterList&);
+    std::map<std::string, TelnetCallback> callback_map;
+
+    // callback implementations:
+    void subscribe(const ParameterList &p);
+    void unsubscribe(const ParameterList &p);
};

/**
@@ -104,6 +140,67 @@ PropsChannel::PropsChannel()
       mode(PROMPT)
{
     setTerminator( "\r\n" );
+    callback_map["subscribe"]    =    &PropsChannel::subscribe;
+    callback_map["unsubscribe"]   =   &PropsChannel::unsubscribe;
+}
+
+PropsChannel::~PropsChannel() {
+  // clean up all registered listeners   
+  for (unsigned int i=0;i< _listeners.size(); i++)
+    _listeners[i]->removeChangeListener( this  );
+
+}
+
+void PropsChannel::subscribe(const ParameterList& param) {
+   if (! check_args(param,1,"subscribe")) return;
+
+   std::string command = param[0];
+   const char* p = param[1].c_str();
+   if (!p) return;
+
+        SG_LOG(SG_GENERAL, SG_ALERT, p << std::endl);
+        push( command.c_str() ); push ( " " );
+        push( p );
+        push( getTerminator() );
+
+        SGPropertyNode *n = globals->get_props()->getNode( p,true );
+   if ( n->isTied() ) {
+      error("Error:Tied properties cannot register listeners");
+      return;
+   }
+    if (n) {
+         n->addChangeListener( this );
+    std::stringstream hashcode;
+    boost::hash<std::string> string_hash;
+    hashcode << "id:" << string_hash(n->getPath()) << getTerminator();
+    // push( hashcode.str().c_str() );  // not yet very useful
+    _listeners.push_back( n ); // housekeeping, save for deletion in dtor later on
+         }
+         else {
+       error("listener could not be added");
+         }
+
+
+}
+
+void PropsChannel::unsubscribe(const ParameterList &param) {
+  if (!check_args(param,1,"unsubscribe")) return;
+
+  try {
+   SGPropertyNode *n = globals->get_props()->getNode( param[1].c_str() );
+   n->removeChangeListener( this );
+  } catch (sg_exception &e) {
+     error("Error:Listener could not be removed");
+  }
+}
+
+
+//TODO: provide support for different types of subscriptions MODES ? (child added/removed, thesholds, min/max)
+void PropsChannel::valueChanged(SGPropertyNode* ptr) {
+  SG_LOG(SG_GENERAL, SG_ALERT, __FILE__<< "@"<<__LINE__ << ":" << __FUNCTION__ << std::endl); 
+  std::stringstream response;
+  response << ptr->getPath(true) << "=" <<  ptr->getStringValue() << "\r\n"; //TODO: use hashes, echo several properties at once, use proper terminator
+  push( response.str().c_str() );
}

/**
@@ -160,7 +257,7 @@ PropsChannel::foundTerminator()
     const char* cmd = buffer.getData();
     SG_LOG( SG_IO, SG_INFO, "processing command = \"" << cmd << "\"" );

-    vector<string> tokens = simgear::strutils::split( cmd );
+    ParameterList tokens = simgear::strutils::split( cmd );

     SGPropertyNode* node = globals->get_props()->getNode( path.c_str() );

@@ -292,7 +389,7 @@ PropsChannel::foundTerminator()
                     if ( !globals->get_commands()
                              ->execute( "reinit", &args) )
                     {
-                        SG_LOG( SG_NETWORK, SG_ALERT,
+                        SG_LOG( SG_GENERAL, SG_ALERT,
                                 "Command " << tokens[1] << " failed.");
                         if ( mode == PROMPT ) {
                             tmp += "*failed*";
@@ -357,7 +454,7 @@ PropsChannel::foundTerminator()
                     if ( !globals->get_commands()
                              ->execute(tokens[1].c_str(), &args) )
                     {
-                        SG_LOG( SG_NETWORK, SG_ALERT,
+                        SG_LOG( SG_GENERAL, SG_ALERT,
                                 "Command " << tokens[1] << " failed.");
                         if ( mode == PROMPT ) {
                             tmp += "*failed*";
@@ -386,7 +483,14 @@ PropsChannel::foundTerminator()
                 mode = DATA;
             } else if ( command == "prompt" ) {
                 mode = PROMPT;
-            } else {
+            } else if (command  == "subscribe" || command == "unsubscribe") {
+         TelnetCallback t = callback_map[ command.c_str() ]; //FIXME: use map lookup for condition
+         if (t)
+                 (this->*t) (tokens);
+         else
+            error("No matching callback found for command:"+command);
+       }
+       else {
                 const char* msg = "\
Valid commands are:\r\n\
\r\n\
@@ -400,7 +504,9 @@ prompt             switch to interactive mode (default)\r\n\
pwd                display your current path\r\n\
quit               terminate connection\r\n\
run <command>      run built in command\r\n\
-set <var> <val>    set <var> to a new <val>\r\n";
+set <var> <val>    set <var> to a new <val>\r\n\
+subscribe <var>      subscribe to property changes (returns hash code for property)\r\n\
+unscubscribe <id>  unscubscribe from property changes (id must be the property hash as returned from subscribe command)\r\n";
                 push( msg );
             }
         }
diff --git a/src/Network/props.hxx b/src/Network/props.hxx
index e55f7c1..09b5a09 100644
--- a/src/Network/props.hxx
+++ b/src/Network/props.hxx
@@ -40,7 +40,8 @@
  * FlightGear properties.
  */
class FGProps : public FGProtocol,
-      public simgear::NetChannel
+      public simgear::NetChannel,
+      public SGPropertyChangeListener // for subscriptions
{
private:

Suggested modifications and enhancements

Related discussions

  1. jam007  (Mar 19th, 2017).  Python3 class for telnet .