Glider instrumentation

[galvedro]

Aha! Then it can be done C style, sort of..

Code: Select all

if (!contains(me, "_LIBRARY_NAS_"))
    io.load_nasal(library, me);


Or something like that, will probably need to play with namespaces a bit. That should load the library in the current namespace if it is not loaded already.

[Hooray]

Yes, that's correct - sorry for not mentioning it, but you seem to have figured it out already: check the environment hash for your own namespace/hash and if it's not there, load your code.
There should be several examples in $FG_ROOT/Nasal for this, basically whenever code is loaded "on demand".

But do note that the 2nd argument is optional and should be a string (name of your namespace) - not a "me" reference, and that you'll want to check the "globals" hash, not "me" - something along these lines:

Code: Select all

var load_if_needed = func(file, namespace)
if (!contains(globals, namespace))
    io.load_nasal(file, namespace);


See io.nas for details, as I said, there should be several examples in $FG_ROOT whenever "io.load_nasal" is used (use grep)

Here are some related discussions:
http://www.flightgear.org/forums/viewto ... 70#p177196
http://www.flightgear.org/forums/viewto ... 92#p168615
http://www.flightgear.org/forums/viewto ... 30&t=17950
http://www.flightgear.org/forums/viewto ... 44#p166413
http://www.flightgear.org/forums/viewto ... 44#p166574
http://www.flightgear.org/forums/viewto ... 44#p166796

will probably need to play with namespaces a bit

http://wiki.flightgear.org/Nasal_Namespaces
http://wiki.flightgear.org/Howto:Unders ... nd_Methods
http://wiki.flightgear.org/Nasal_Namespaces_in-depth


Please feel free to add missing stuff there ! (i.e. just copy paste our postings)

[Philosopher]

salve, galvedro! (that's a pun, btw)

Consider this something of a code review, if you're looking for feedback.

TODO:

• document the toolkit better
• error handling: don't trust the user

Both admirable goals, I'd say . On looking over the code I would say “code est optimus!” (“the code is excellent” in Latin, because that's the best language ofc), though there are a few things I would like to talk about.

Do you happen to come from a Python background? Or just a strong system/scripting one? The reason I ask is this code, which is valid Python, but not "good" Nasal, per se:

Code: Select all

me.speed_lsnr = setlistener("/sim/speed-up",
    func(n) { me.sim_speed = n.getValue() },
    startup = 1, runtime = 0);

(FTR, 110% of everything is an expression in Nasal, including if-(else)-statements and assignment; this means they have a "value" and it explains the behavior later.) In Python, arguments are declared in four flavors: mandatory (e.g. foo), optional (e.g. bar=expr), extra positional (e.g. *list), and extra keywords (e.g. **kwds). Functions can be called with positional arguments (e.g. 100) and then named arguments (e.g. spacer="\t"). In Nasal, however, there's just mandatory, optional, and extra positional/overflow (which is like: arg... – note the ellipsis). (The overflow is sort-of always present, i.e. calling a func(a,b) with args (1,2,3,4) will result in a=1,b=2,arg=[3,4].) They are usually called positionally, just like Python. However, when specifying named arguments, the : operator is used (just like if it is a hash) – not the = operator present in Python. Additionally, they cannot be mixed, e.g. no (1,2,3,foo:9).

The reason the above code works is the startup = 1 essentially is an expression that assigns a variable (“startup” – more on this in a moment) and evaluates to the thing set – that is 1 here (aka True in Python). Variable assignments using the var keyword (~cough~, not like this one) are guaranteed to be local; those not using it look in outer scopes, trying to set an existing variable, or making a new local one if the variable was undefined. Let's just say that the possible implications of that are horrendous, and that you should either (a) use the `var` keyword or (b) leave out the startup=1 and substitute just 1 instead or (c) name all the arguments properly, viz.

Code: Select all

me.speed_lsnr = setlistener(node: "/sim/speed-up",
    fn: func(n) { me.sim_speed = n.getValue() },
    init: 1, runtime: 0);

Now onto this:

Code: Select all

disable: func {
    me.timer.stop();
    me.reinit_lsnr and removelistener(me.reinit_lsnr);
    me.speed_lsnr and removelistener(me.speed_lsnr);
}

Two questions here:

1. Why do you want to stop the loop?
2. Why do you want to stop it only the first time?

Your instruments will only be created once – not on every sim reset – and you already ensure the start routine is called only once:

Code: Select all

enable: func {
    var lsnr = setlistener("sim/signals/fdm-initialized", func {
        removelistener(lsnr);
        me.init();
        me.timer.start();
    });
}

Basically, my understanding is that your instruments would stop working on reset. (Apologies since I haven't tested it.) Also, isn't the "enable" step better handled on creation of an instrument (i.e. the .new() method)?

Lastly on the agenda, we have pedantry:

• In general, try to never use builtin variable names as local variables, like your "size" argument: soaring-instrumentation-sdk.nas @ L203. Good syntax highlighting helps with this, and is how I spotted . What editor do you use, btw?
• For classes (that's a hint, classes vs objects) derived from InstrumentComponent, I think it's best to go this route:
  

  Code: Select all

  var InputSwitcher = {
    parents:[InstrumentComponent]
    new: func(...) {
        return {
            parents: [me], ...
        };
    },
...
};

  
  versus this:
  

  Code: Select all

  var InputSwitcher = {
    new: func(...) {
        return {
            parents: [me, InstrumentCompenent], ...
        };
    },
...
};

  
  because the actual "InputSwitcher" class is inheriting/subtyping the methods, while the object returned from InputSwitcher.new() is really just an instance of "InputSwitcher", which happens to be an implementation of "InstrumentComponent".
• For SoaringInstrument, you can always run two loops for the update rates. That way it's more abstracted and asynchronous. I would also remove this line (a user might want to update the instrument again within the same frame, which as near as I can tell is when the condition would occur):
  

  Code: Select all

  if (dt == 0) return;

  
  and use this for dt instead (ref: aircraft.nas::lowpass, you can leave the me.sim_speed in there, though):
  

  Code: Select all

  var dt = getprop("/sim/time/delta-sec")*getprop("/sim/speed-up");

As I mentioned above, it's really looking great! You also do a good job of providing obvious examples, and thanks for converting existing aircraft's instruments (sadly that's not often done in this community, it seems to take too much chutzpah and work ). It seems like it would happen to be a good interface for more general systems, and with a few modifications, maybe even ANNs too, Hooray?

[Hooray]

Excellent feedback, you mentioned quite a few things that I completely missed when skimming over the code - I guess, I need to review my own code with your feedback in mind, galvedro's code is much cleaner than most of the Nasal code we get to see, including my own
BTW: I talked to galvedro about generalizing the framework, at least for systems simulation (electrical, see the wiki) - using it for some form of ANN framework should als o be possible, and even did it occur to me, too - but that's currently not very high priority - because bombable isn't being actively developed ... otherwise, there wouldn't be much required, just some adapters to map the whole thing to the property tree, with arbitrary numbers of I/O properties - I'm just afraid that it would be too slow, given the number of props - so one would probably prefer native Nasal data structures for most of the internal state, or even add some cppbind/C++ bindings to make this more efficient.

It's definitely an intersting idea though, and we should add a pointer to this thread to the ANN article

[galvedro]

Hi!

Wow! Thank you Philosopher for taking the time to review the code. Very much appreciated!

when specifying named arguments, the : operator is used (just like if it is a hash) – not the = operator present in Python.

Aargh! Good catch! I have already fallen in this Nasal trap a couple of times, and it never ends well . This is actually a very subtle problem to debug, for one because the pythonic syntax looks so familiar (as you guessed), and also because incidentally, I happened to place the arguments in the same order they are expected, so it actually works as expected.. Until it doesn't!

Two questions here:
Why do you want to stop the loop?
Why do you want to stop it only the first time?

This is a bug I noticed yesterday when I actually tested the reinit call. For some reason I misinterpreted the reinit procedure. I thought the script should cleanup all resources upon receiving the reinit signal, and assumed that the script would be called again afterwards, as in a fresh start. But that is not the case. The script shall leave its internal state as it should be just after initialization.

Basically, my understanding is that your instruments would stop working on reset. (Apologies since I haven't tested it.)

Aparently, you don't even need to test it. That is exactly what it happens

Also, isn't the "enable" step better handled on creation of an instrument (i.e. the .new() method)?

I have an internal debate with this, and I am not entirely happy with how it's done now. On one side, I do like the enable() step, as I think it is easier to understand for users. It's a quite "mechanical" interface if you like: you define the instrument, and turn it on. Everybody can understand that. It also allows for better synchronization in more complex scripts, where you might want to prepare the runtime (i.e. acquire resources, etc) in a setup step, and then start several processes in a coordinated fashion.

On the other hand, an enable() method calls for a disable() method, and the question is: what should disable() actually do? With my previous understanding of how reinit worked, "unpluging" the instrument from the sim was the obvious choice. However, reinit works in a different way and this has to change. So my current line of thinking is to make enable()/disable() control the update loop, i.e. start/stop the internal timer, and do all resource acquisition in new().

In general, try to never use builtin variable names as local variables, like your "size" argument: soaring-instrumentation-sdk.nas @ L203. Good syntax highlighting helps with this, and is how I spotted . What editor do you use, btw?

Aha, good point. I am using Notepad++ in Windows and gedit on my Linux environments. gedit seems to have better highlighting for Nasal but unfortunately, my Linux box at home is currently broken, so I work in Windows instead.

For classes (that's a hint, classes vs objects) derived from InstrumentComponent, I think it's best to go this route:

Agree, I like it.

I would also remove this line (a user might want to update the instrument again within the same frame, which as near as I can tell is when the condition would occur):

Code: Select all

if (dt == 0) return;

Nope. I don't like this check on every loop turn either, but it became necessary when I switched to "stoppable" timers. For some reason I don't yet understand, they fire immediately when I call timer.start(), i.e. rather than waiting for the next frame. The consequence is that the very first update gets a dt = 0, becase it happens in the same frame as init(). This leads to numerical instabilities in some components (divide by zero, etc).

With that in mind, I rather prefer to guaranty components that dt will always be strictly greater than 0, than have every component checking for it on every update, or worse, forgetting to do so and crashing. Not that it happened to me..

and use this for dt instead (ref: aircraft.nas::lowpass, you can leave the me.sim_speed in there, though):

Code: Select all

var dt = getprop("/sim/time/delta-sec")*getprop("/sim/speed-up");

I can't do that. The loop can be programmed to run at any rate so we can't assume it is running once per frame. And that actually leads to the question of how accurate "speed-up" is, taking into account that it can change arbitrarily within an update period. Is there a property somewhere that exposes total simulated time (i.e. integrating delta-sec * speed-up every frame)?

[Hooray]

I thought the script should cleanup all resources upon receiving the reinit signal, and assumed that the script would be called again afterwards, as in a fresh start. But that is not the case. The script shall leave its internal state as it should be just after initialization.

just keep in mind that a reset may also involve change of location, so I'd tend to re-initialize() the system normally ?

Basically, my understanding is that your instruments would stop working on reset. (Apologies since I haven't tested it.)

Aparently, you don't even need to test it. That is exactly what it happens

Usually, he's exactly right - he's able to run even sophisticated Nasal code in his mind's VM, we don't have anybody else here who's as intimately familiar with Nasal internals. I think it started out of necessity, when he came here he was browsing the forums on an IPhone or IPad - so he needed a way to run all those code examples somehow

On one side, I do like the enable() step, as I think it is easier to understand for users. It's a quite "mechanical" interface if you like: you define the instrument, and turn it on. Everybody can understand that. It also allows for better synchronization in more complex scripts, where you might want to prepare the runtime (i.e. acquire resources, etc) in a setup step, and then start several processes in a coordinated fashion.

you could support an optional argument in the ctor, which runs me.enable() automatically - so that the user gets to decide how the system behaves, i.e. retain enable() - but provide an option to run it automatically during the new() call.

and the question is: what should disable() actually do? With my previous understanding of how reinit worked, "unpluging" the instrument from the sim was the obvious choice. However, reinit works in a different way and this has to change. So my current line of thinking is to make enable()/disable() control the update loop, i.e. start/stop the internal timer, and do all resource acquisition in new().

disable() should ideally suspend the script for the duration of the reset/re-init, which may involve change of location, environmental settings and/or aircraft (eventually) - this may seem like a nobrainer on modern hardware, but on older hardware, resets actually do take a few seconds - and it does make a huge difference if aircraft code is properly suspended then, otherwise there are usually all sorts of weird errors shown in the console.

[Philosopher]

Haha, I think I had maybe one month (after joining the forums) that I had my computer available with FlightGear . Now I have it occasionally.

Anyways, Hooray raises a good point: one probably wants to reset the instruments on a simulator reset. Currently there are three things to be aware of:

• Nasal files are loaded once on startup
• /sim/signals/reinit can be listened to
• but also /sim/signals/fdm-initialized - the FDM restarts on a simulator restart (useful for testing FDM changes )

With this in mind, I would probably keep a permanent listener on fdm-initialized which will make a clean start with the instrument - that is, erase all "memory" and start the loop (if it was stopped).

Thanks for the clarification on timing, my mistake . I wonder if it should just be integrated into SGTimer...

[Hooray]

Hi, just briefly, regarding: https://gitorious.org/fg/galvedros-fgda ... d53f25e6f8

Unless I missed it and you are somewhere wrapping setlistener() there, you should be saving each listener's ID (as you did previously), so that you can easily remove previously allocated listeners, to avoid multiple instances of the callback running. That will also enable you to have a destructor where you can really clean up everything to start over again. Note that destructors are commonly called "del" by convention, and that they must be invoked explicitily - i.e. they don't run automatically*

*: although that would be possible with some minor hooks into the GC, i.e. to register Nasal callbacks that are invoked prior to the object/naRef being GC'ed - and it would actually be fairly useful, i.e. for RAII and other purposes

[galvedro]

Ok, I think I got it now
This is what I'm doing:

Apparently, signals/reinit marks the start of a reinit routine, and signals/fdm-initialized more or less marks the end. I say more or less, because obviously any system waiting on fdm-initialized will not be ready just yet, but it should be good enough for our purposes, as these instruments don't have dependencies on anything other than the fdm.

With this in mind, I now keep two permanent listeners: one on signals/reinit and another one on signals/fdm-initialized, as follows

Code: Select all

setlistener("sim/signals/reinit", func {
   m.timer.stop();
   m.initialized = 0;
});

setlistener("sim/signals/fdm-initialized", func {
   if (m.timer.isRunning) m.timer.stop();
   call(me.init, [], m);
   if (m.enabled) m.timer.start();
});


instrument.enable() / instrument.disable() now control the update timer only, giving control to the user on when the instrument should be active. Hooray, I have also incorporated your suggestion of adding an optional "enable" flag to the instrument constructor for more flexibility.

I wonder if it should just be integrated into SGTimer...

Not having the total elapsed (simulated) time available anywhere sounds like a problem, and I would say it also calls for timing bugs here and there . It basically forces everybody to integrate (delta-sec * speed-up) every frame. It's the only way to correctly compute elapsed time in the simulated world. This should definitively be done only once per frame and published in a property.

When I started this project I thought time/elapsed-sec was that property, but it isn't. Its value grows in real world increments, it doesn't honor the speed-up setting, but, it stops growing when the sim is paused Perhaps it is a bug? I can't really devise how such a property could be used...

[galvedro]

Haha, we cross-posted

you should be saving each listener's ID (as you did previously), so that you can easily remove previously allocated listeners,

Yes, but my question is when should they be removed?

[Philosopher]

Actually, no listeners need to be removed. They're all permanent and set only once at Nasal-loading time.

[Hooray]

that is, until we are able to reload/switch aircraft

regardless of current simulator restrictions, I'd suggest to never allocate a resource like a listener without storing the handle to manage/release it afterwards (even if you don't know what to do with it at the moment) - either by using a corresponding wrapper or by simply saving each handle. Otherwise, it's a potentially leaking implementation.

The real issue is that not even the C++ code keeps track of resources like listeners and their scope. As long as we continue building our code around the asssumption that there will only ever be a single aircraft and a single session, we are adding to the underlying problem and not really helping solve it - same goes for assuming that there will only ever be a single aircraft simulated, or that simulated vehicles are always "aircraft" in the first place. There are so many hardcoded assumptions that it's getting increasingly difficult to get rid of them - just look at the number of discussions we've had over the years about running multiple FDM instances, or having AI traffic with FDM support - as can be seen, such hard-coded assumptions eventually end up seeming short-sighted...

[Philosopher]

Good point, Hooray, sorry for forgetting about that one .

After having thought about it, ideally there will be some kind of load/unload property for every subsystem and for whole aircraft as well. Currently /sim/model/path might be a good indicator, otherwise I don't know if we store the path to -set.xml anywhere. I can see aircraft-specific objects, such as galvedro's nice library, having a .del() method to clean everything up and registering a listener for a change of aircraft that calls .del().

Actually, instead of manual timer/listener management, it would be really convenient to have some kind of Nasal wrapper to make objects with a "lifetime": once only (only applicable to listeners), per-aircraft (remove on aircraft change), or permanent (default, to ensure backwards-compatibility). That way it's just abstracted away, particularly for when things change. I know I would be the first to volunteer to overhaul outdated code for these purposes, let's just hope that outside aircraft won't need it.

[Philosopher]

Alvedro, do you compile from Git? I could fairly easily make a patch that will pass elapsed simtime and/or realtime to timers. Otherwise, I would swear that simtime should be exposed somewhere in the property tree .

[Hooray]

Like you said, generally wrapping things like settimer/setlistener at the Nasal or C++ level would only work well up to a certain degree - simply because we do not currently differentiate between different types of code, i.e. aircraft vs. GUI/system-code (think canvas) - so this would either need some exceptions, possibly based on namespaces (hackish) - or overriding callback-registration APIs like settimer/setlistener to specify a valid scope, as in "aircraft" or "gui", to help the underlying code release resources and unload/reload code as needed.

After having thought about it, ideally there will be some kind of load/unload property for every subsystem and for whole aircraft as well. Currently /sim/model/path might be a good indicator, otherwise I don't know if we store the path to -set.xml anywhere. I can see aircraft-specific objects, such as galvedro's nice library, having a .del() method to clean everything up and registering a listener for a change of aircraft that calls .del().

right, that would be explicit design then - otherwise, we'd really have to load aircraft code in some different fashion to ensure that it can be kept separate and safely released. But that touches the whole reset/re-init discussion already - according to which, the initial plan seems to be shut down the whole thing and reload things from scratch.

instead of manual timer/listener management, it would be really convenient to have some kind of Nasal wrapper to make objects with a "lifetime": once only (only applicable to listeners), per-aircraft (remove on aircraft change), or permanent (default, to ensure backwards-compatibility). That way it's just abstracted away, particularly for when things change. I know I would be the first to volunteer to overhaul outdated code for these purposes, let's just hope that outside aircraft won't need it.

yeah, that's basically the whole lifetime vs. scope idea - but you'd still need to use some form of marker to identify such code, no matter if it's a string or a namespace/hash - on the other hand, without explicit design things do get tricky once there are mutual references held between valid and invalid namespaces. Certainly an interesting challenge