Michael, Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works. Here are some notes one what I have done so far: Created a stub DuktapeConsoleCommand (derived from DuktapeObject) in ovms_duktape.{h,cpp}. This should hold enough to be able to call the javascript callback method for that object. It also stores the module filename (so the registration can be removed when the module is unloaded). Provide a DuktapeCommandMap m_cmdmap in ovms_duktape.{h,cpp} OvmsDuktape class that stores a mapping from OvmsCommand to DuktapeConsoleCommand. Created a OvmsDuktape::RegisterDuktapeConsoleCommand in ovms_duktape.{h,cpp) that (a) creates the OvmsCommand() object, (b) registers it, (c) creates the DuktapeConsoleCommand() object, and (d) updates a map from OvmsCommand->DuktapeConsoleCommand. There Is also a single callback DukOvmsCommandRegisterRun designed to be run by all. Created hooks NotifyDuktapeModuleLoad, NotifyDuktapeModuleUnload, and NotifyDuktapeModuleUnloadAll in OvmsDuktape. The javascript module is identified by filename (path to module or script on vfs, usually, but may also be an internal module). The Unload functions look through the m_cmdmap and unregister commands for javascript modules being unloaded. Provide an implementation for ovms_command DukOvmsCommandRegister to support registering commands from Javascript modules. This should extract the details, and then call OvmsDuktape::RegisterDuktapeConsoleCommand to do the actual registration. This has been implemented, except for the callback method (and somehow passing that method from Javascript in the OvmsCommand.Register javascript call). Provide a stub implementation for DukOvmsCommandRegisterRun. This uses m_cmdmap to lookup the DuktapeConsoleCommand object for the command to be run. It should execute the callback method (but that part is not yet implemented). I still need help with #5 and #6. What needs to be implemented in DuktapeConsoleCommand, and how is the parameter in OvmsCommand.Register used to store the callback (#5)? Then how to callback the command method from DukOvmsCommandRegisterRun (#6)? If you have time, it is probably much quicker for you to simply make those changes. An alternative implementation would be to do something like the pubsub framework, where the mapping command->callback is done from within a javascript module. That I could do, but it seems your DuktapeObject can do it better. Thanks, Mark.

On 15 Jul 2020, at 3:34 PM, Michael Balzer <dexter@expeedo.de> wrote:

Mark,

yes, I needed that persistence for the HTTP and VFS classes, but I also needed to be able to couple a dynamic C++ instance with a JS object and have a mechanism to prevent garbage collection while the C++ side is still in use. If the C++ side is no longer needed, the JS finalizer also needs to imply the C++ instance can be deleted.

That is all implemented by DuktapeObject. DuktapeObject also provides JS method invocation on the coupled JS object and a mutex for concurrency protection.

We probably need some more framework documentation than the header comments (applies to all of our framework classes…):

/*************************************************************************************************** * DuktapeObject: coupled C++ / JS object * * Intended for API methods to attach internal API state to a JS object and provide * a standard callback invocation interface for JS objects in local scopes. * * - Override CallMethod() to implement specific method calls * - Override Finalize() for specific destruction in JS context (garbage collection) * - call Register() to prevent normal garbage collection (but not heap destruction) * - call Ref() to protect against deletion (reference count) * - call Lock() to protect concurrent access (recursive mutex) * * - GetInstance() retrieves the DuktapeObject associated with a JS object if any * - Push() pushes the JS object onto the Duktape stack * * Note: the DuktapeObject may persist after the JS object has been finalized, e.g. * if some callbacks are pending after the Duktape heap has been destroyed. * Use IsCoupled() to check if the JS object is still available. * * Ref/Unref: * Normal life cycle is from construction to finalization. Pending callbacks extend * the life until the last callback has been processed. A subclass may extend the life * by calling Ref(), which increases the reference count. Unref() deletes the instance * if no references are left. */

You normally just need to use Register/Deregister & Ref/Unref, and to implement the constructor and CallMethod. Coupling of the instances normally is done on construction, as a JS object is normally already needed for the parameters and can simply be attached to.

Have a look at DuktapeHTTPRequest, DuktapeVFSLoad and DuktapeVFSSave, these are the current subclasses using this.

For the command registration I would probably couple the OvmsCommand instance with a JS command object providing an execution method.

Tell me if you need more info.

Regards, Michael

Am 15.07.20 um 08:12 schrieb Mark Webb-Johnson:

...

@Michael this is probably for you.

I am trying to implement javascript command registration. The idea is that a javascript module can call something like:

OvmsCommand.Register(basecommand, name, title, callbackfn, usage, min, max)

Then we reflect that into MyCommandApp.RegisterCommand, and keep a track of which command is for which javascript callbackfn. When the command is executed, we pass it into duktape.

I also have tracking for javascript module loading and unloading, so I can DeregisterCommand() if duktape is reloaded (and also protected against commands being registered in short-lived scripts run from the command line).

To implement this, I need to store the callbackfn as a persistent reference to a duktape javascript function.

The issue with callback function references in duktape is summarised here:

https://wiki.duktape.org/howtonativepersistentreferences <https://wiki.duktape.org/howtonativepersistentreferences>

When a Duktape/C function is called, Duktape places the call arguments on the value stack. While the arguments are on the value stack, they're guaranteed to be reachable and the Duktape/C function can safely work with the arguments.

However, when the Duktape/C function returns, the value stack is unwound and references in the function's value stack frame are lost. If the last reference to a particular value was in the function's value stack frame, the value will be garbage collected when the function return is processed.

The standard approach is to store the reference back in the duktape duk_push_global_stash so it won’t get garbage-collected. But, that seems messy.

I see that Michael has already implemented something that seems similar in ovms_script.{h, cpp}, for the async http callbacks. Presumably to avoid this issue. But, the approach seems very different, and I am not sure if it is stopping _all_ garbage collection for the duration of the async query, or just that particular object being garbage collected. The work seems extensive (quite a few objects involved).

So @Michael, any suggestions for this? I don’t want to reinvent the wheel...

Regards, Mark.

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

Mark,

Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works.

I admit my documentation has some shortcomings. I'll try to fill that gap first: *_Concept #1: Reference counting_* A DuktapeObject is meant to be a C++/OS extension of a Javascript object, for example to hold some system binding associated with the JS object. The primary goal is to provide asynchronous operations on the system side, initiating JS callbacks when finishing/failing. For example a HTTP operation can be started by a script, passing some "done" callback. The system then starts the network operation asynchronously, the JS task can continue processing other scripts. Once the network operation is done, the DuktapeObject sends a request to the Duktape task to execute the "done" callback on itself. So the DuktapeObject is normally shared by multiple contexts and parallel operations. Asynchronous operation also means the JS object or context may already be gone when the operation finishes. So the DuktapeObject needs to stay locked in memory independent of the JS context. That's implemented by counting the active references to the DuktapeObject (methods Ref() / Unref()). The last Unref() automatically deletes the DuktapeObject instance. For example: JS requests a network operation. The initial JS binding (see coupling) sets the reference count to 1. The DuktapeObject starts the network request, increasing the ref count to 2. If the JS context now dies (decreasing the reference count), the DuktapeObject will still remain valid until the network operation returns. As the Ref/Unref operations need a (recursive) mutex, that's also part of the DuktapeObject and exposed by the API for other uses: Lock() / Unlock(). *_Concept #2: Coupling_* Javascript does not have a destructor concept. JS objects get deleted by heap destruction or by the garbage collector when no reference to the JS object is left. In both cases, the actual object deletion is called "finalization", and a special finalizer method/callback can be installed on a JS object to be called just before the object gets deallocated. That is done by the DuktapeObject::Couple() method (implicitly called when constructed directly with a JS object reference). There is no way to force finalization on a JS object. So a DuktapeObject cannot tell Duktape to delete it's coupled object, that means a DuktapeObject should normally not be deleted from outside the Duktape context, at least not if still coupled to the JS object. Coupling and decoupling can only be done in the Duktape context. The standard finalizer DuktapeObject::Finalizer() simply decouples, automatically deleting itself if the coupling was the last reference. This is a virtual method, so can be overridden as necessary. The coupling operation additionally adds a hidden pointer to the DuktapeObject instance in the JS object. That allows to check for and retreive associated DuktapeObject instances from any JS object, which is provided by the GetInstance() call. *_Concept #3: Registration_* For asynchronous operations, it's normally very convenient to have a "fire & forget" API. Example from the documentation: VFS.Save({ path: "/sd/mydata/telemetry.json", data: Duktape.enc('jx', telemetry), fail: function(error) { print("Error saving telemetry: " + error); } }); I.e. you simply pass the operation arguments including the done/fail callbacks to an API method and don't need to care about storing a reference to some handle. In JS that normally means the object used won't have any reference left after the call, so would be deleted by the garbage collector on the next run. To avoid garbage collection and lock the JS object in memory, we need to store a reference to it in a "public" place. Duktape provides a special public place for this, hidden from scripts, called the global stash. DuktapeObject maintains a dedicated global object registry in that stash. Adding and removing the coupled object reference to/from that registry is done by the Register() and Deregister() methods. So for asynchronous system operations, or system integrations that shall be persistent, you normally do a Register() call together with the coupling, unless some ressource isn't available. Deregistration is then normally done when all pending JS callbacks have been executed, or when the persistent system integration has been unbound. Other API designs are possible here: if you'd rather like the script needing to store a reference to your operation handle, you don't need to do a registration. The object will then be deleted (finalized) by the garbage collector automatically after the script deletes the reference. *_Concept #4: Callback invocation_* Triggers on the system side, for example a finished or failed network operation, shall normally trigger a JS method execution. JS callback methods are simply passed as part of the arguments object in modern JS APIs. This allows to pass simple function definitions inline, as well as to reference a separately defined general handler function. JS allows functions to be excuted in the context of any object, and callbacks normally are executed in the context of the API object. This adds even more convenience, as the callbacks can easily access the other API arguments still stored in the object, as well as additional data added by the call. JS callbacks cannot be executed directly from any system context, they need to run in the Duktape context. So the DuktapeObject callback invocation mechanism includes a general method to request a callback execution by Duktape: RequestCallback() Note: RequestCallback() is an asynchronous operation. A synchronous variant can be added if necessary (and probably will be for command execution from a console). A pending callback automatically increments the reference count, so the object is locked in memory until the callback has been executed (or aborted) by the Duktape task. The callback invocation API provides a void* for simple data (e.g. a fixed string) to be passed to the callback method, but for more complex data, you will normally fill some DuktapeObject member variables before invoking the callback. In Duktape context, the callback invocation translates the data returned or provided by the system side into the Duktape callback arguments and then runs the callback (if the object actually has the requested callback set). The default implementation for this is DuktapeObject::CallMethod(), which can be used directly for simple callbacks without arguments. For more complex handling, override this with your custom implementation. The callbacks are by default executed on the coupled JS object, so data can also be transported by setting properties on that object. The callback can then simply access them via "this". To simplify callback invocation from code parts that may run outside or inside Duktape, it's convenient to allow calling CallMethod() without a Duktape context, and let CallMethod() translate that into a RequestCallback() call as necessary. Pattern: duk_ret_t DuktapeHTTPRequest::CallMethod(duk_context *ctx, const char* method, void* data /*=NULL*/)   {   if (!ctx)     {     RequestCallback(method, data);     return 0;     }   … A CallMethod() implementation isn't limited to executing a single callback. A common example is an API defining "done" & "fail" callbacks, as well as a general final "always" callback. DuktapeHTTPRequest::CallMethod() also serves as an example implementation for this. Wow… that's become more to write & read than I expected. Please provide some feedback: is that explanation sufficient & clear? I'll refine it for the developer docs then. Regards, Michael Am 01.09.20 um 19:52 schrieb Michael Balzer:

Mark,

I'll have a look.

Regards, Michael

Am 01.09.20 um 07:30 schrieb Mark Webb-Johnson:

...

Michael,

Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works.

Here are some notes one what I have done so far:

1. Created a stub DuktapeConsoleCommand (derived from DuktapeObject) in ovms_duktape.{h,cpp}. This should hold enough to be able to call the javascript callback method for that object. It also stores the module filename (so the registration can be removed when the module is unloaded).

2. Provide a DuktapeCommandMap m_cmdmap in ovms_duktape.{h,cpp} OvmsDuktape class that stores a mapping from OvmsCommand to DuktapeConsoleCommand.

3. Created a OvmsDuktape::RegisterDuktapeConsoleCommand in ovms_duktape.{h,cpp) that (a) creates the OvmsCommand() object, (b) registers it, (c) creates the DuktapeConsoleCommand() object, and (d) updates a map from OvmsCommand->DuktapeConsoleCommand. There Is also a single callback DukOvmsCommandRegisterRun designed to be run by all.

4. Created hooks NotifyDuktapeModuleLoad, NotifyDuktapeModuleUnload, and NotifyDuktapeModuleUnloadAll in OvmsDuktape. The javascript module is identified by filename (path to module or script on vfs, usually, but may also be an internal module). The Unload functions look through the m_cmdmap and unregister commands for javascript modules being unloaded.

5. Provide an implementation for ovms_command DukOvmsCommandRegister to support registering commands from Javascript modules. This should extract the details, and then call OvmsDuktape::RegisterDuktapeConsoleCommand to do the actual registration. This has been implemented, except for the callback method (and somehow passing that method from Javascript in the OvmsCommand.Register javascript call).

6. Provide a stub implementation for DukOvmsCommandRegisterRun. This uses m_cmdmap to lookup the DuktapeConsoleCommand object for the command to be run. It should execute the callback method (but that part is not yet implemented).

I still need help with #5 and #6. What needs to be implemented in DuktapeConsoleCommand, and how is the parameter in OvmsCommand.Register used to store the callback (#5)? Then how to callback the command method from DukOvmsCommandRegisterRun (#6)? If you have time, it is probably much quicker for you to simply make those changes.

An alternative implementation would be to do something like the pubsub framework, where the mapping command->callback is done from within a javascript module. That I could do, but it seems your DuktapeObject can do it better. 

Thanks, Mark.

...

On 15 Jul 2020, at 3:34 PM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

yes, I needed that persistence for the HTTP and VFS classes, but I also needed to be able to couple a dynamic C++ instance with a JS object and have a mechanism to prevent garbage collection while the C++ side is still in use. If the C++ side is no longer needed, the JS finalizer also needs to imply the C++ instance can be deleted.

That is all implemented by DuktapeObject. DuktapeObject also provides JS method invocation on the coupled JS object and a mutex for concurrency protection.

We probably need some more framework documentation than the header comments (applies to all of our framework classes…):

/***************************************************************************************************  * DuktapeObject: coupled C++ / JS object  *  *  Intended for API methods to attach internal API state to a JS object and provide  *    a standard callback invocation interface for JS objects in local scopes.  *   *  - Override CallMethod() to implement specific method calls  *  - Override Finalize() for specific destruction in JS context (garbage collection)  *  - call Register() to prevent normal garbage collection (but not heap destruction)  *  - call Ref() to protect against deletion (reference count)  *  - call Lock() to protect concurrent access (recursive mutex)  *   *  - GetInstance() retrieves the DuktapeObject associated with a JS object if any  *  - Push() pushes the JS object onto the Duktape stack  *   *  Note: the DuktapeObject may persist after the JS object has been finalized, e.g.  *    if some callbacks are pending after the Duktape heap has been destroyed.  *    Use IsCoupled() to check if the JS object is still available.  *   *  Ref/Unref:  *    Normal life cycle is from construction to finalization. Pending callbacks extend  *    the life until the last callback has been processed. A subclass may extend the life  *    by calling Ref(), which increases the reference count. Unref() deletes the instance  *    if no references are left.  */

You normally just need to use Register/Deregister & Ref/Unref, and to implement the constructor and CallMethod. Coupling of the instances normally is done on construction, as a JS object is normally already needed for the parameters and can simply be attached to.

Have a look at DuktapeHTTPRequest, DuktapeVFSLoad and DuktapeVFSSave, these are the current subclasses using this.

For the command registration I would probably couple the OvmsCommand instance with a JS command object providing an execution method.

Tell me if you need more info.

Regards, Michael

Am 15.07.20 um 08:12 schrieb Mark Webb-Johnson:

...

@Michael this is probably for you.

I am trying to implement javascript command registration. The idea is that a javascript module can call something like:

OvmsCommand.Register(basecommand, name, title, callbackfn, usage, min, max)

Then we reflect that into MyCommandApp.RegisterCommand, and keep a track of which command is for which javascript callbackfn. When the command is executed, we pass it into duktape.

I also have tracking for javascript module loading and unloading, so I can DeregisterCommand() if duktape is reloaded (and also protected against commands being registered in short-lived scripts run from the command line).

To implement this, I need to store the callbackfn as a persistent reference to a duktape javascript function.

The issue with callback function references in duktape is summarised here:

https://wiki.duktape.org/howtonativepersistentreferences

/When a Duktape/C function is called, Duktape places the call arguments on the value stack. While the arguments are on the value stack, they're guaranteed to be reachable and the Duktape/C function can safely work with the arguments.

However, when the Duktape/C function returns, the value stack is unwound and references in the function's value stack frame are lost. If the last reference to a particular value was in the function's value stack frame, the value will be garbage collected when the function return is processed./

The standard approach is to store the reference back in the duktape duk_push_global_stash so it won’t get garbage-collected. But, that seems messy.

I see that Michael has already implemented something that seems similar in ovms_script.{h, cpp}, for the async http callbacks. Presumably to avoid this issue. But, the approach seems very different, and I am not sure if it is stopping _all_ garbage collection for the duration of the async query, or just that particular object being garbage collected. The work seems extensive (quite a few objects involved).

So @Michael, any suggestions for this? I don’t want to reinvent the wheel...

Regards, Mark.

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Mark, for the synchronous callback execution, that's simply a variant of OvmsDuktape::DuktapeRequestCallback() using the already existing DuktapeDispatchWait() method along with passing the OvmsWriter* in dmsg.writer. Could be named "…ExecuteCallback" to reflect the synchronous nature. There is no need to extend OvmsCommand. The DuktapeObject is meant to be used as a general binding of arbitrary system objects to Duktape, so the system objects don't need to be extended. From my first check of your implementation, I would probably: * Redefine the register API to expect an object with an "execute" (and later possibly also a "validate") callback, and bind the DuktapeCommandObject to that object. A "fail" callback could also make sense. * Delegate the full OvmsCommand life cycle and execution handling to the DuktapeCommandObject. * Remove the forced unregistering of commands on script file unloading. * Inhibit implicit sub command deletions by parent deletions. My API design would be like this: // Command registration: var cmd = OvmsCommand.Register({   parent: "test",   name: "dukcommand",   title: "Test/demonstrate Duktape command registration",   usage: "Pass 1 - 3 arguments",   min: 1,   max: 3,   execute: function(verbosity, argv) {},   fail: function(error) {} }); // Command deregistration: OvmsCommand.Unregister({ parent: "test", name: "dukcommand" }); // …or in case we still have the cmd object, simply: OvmsCommand.Unregister(cmd); The OvmsCommand execution callback would be a bound method of the DuktapeCommandObject instance. The method passes the verbosity and arguments vector to ExecuteMethod() via the data pointer using a custom struct. CallMethod() then pushes these on the Duktape stack and calls the "execute" property. The forced unregistering limits registering commands to library plugins (which don't have an unload operation yet). I think this is an unnecessary limitation, I would like to be able to register a command by adding a registration call to "ovmsmain.js" or by executing a script. Of course that means commands registered that way need to be unregistered explicitly, but that's up to the script developers & users then. I don't think we should limit them here without good reason. On a Duktape unload/reload operation, all JS objects get finalized anyway because of heap destruction, so deletion would happen automatically by the DuktapeConsoleCommand finalizer. If we add some module unload operation later on, modules may e.g. define a cleanup callback, or we can add the module filename to the DuktapeObject registry -- the latter option would also automatically apply to all DuktapeObject instances, eliminating the need for separate registries for other system bindings potentially to be added in the future. Implicit OvmsCommand deletions can happen as the order of deletion is undefined. Inhibiting this should be a straight forward use of the reference count though: on registration of a sub command, simply Ref() all parent commands registered by Duktape as well. Unref() them after the sub command has been removed, the last Unref() on a parent then automatically deletes it (the first Ref() is done by the coupling). Regards, Michael Am 08.09.20 um 09:06 schrieb Mark Webb-Johnson:

Michael,

Very clear, and very helpful. Only thing I would suggest would be to have a minimum example in the documentation. The bare minimum required for an implementation of an object.

Reading through what you write, it seems the correct approach is:

* Extend OvmsCommand to have a virtual ExecuteCommand method (same parameters as ‘m_execute’ callback). * Extend OvmsCommand::execute to check if m_execute is null, then call ‘ExecuteCommand’ instead. * Perhaps do the same for m_validate in OvmsCommand. * Then, the duktape implementation can be object (rather than callback function) based, as your DuktapeObject expects. The javascript would call a function to register a command, with a command object to be used for the callback. * Need to extend the DuktapeObject system to support a synchronous command (presumably implemented with a passed mutex like we do in several other parts of the system).

Is that correct, and what you were expecting? Or any other suggestions?

Regards, Mark.

...

On 7 Sep 2020, at 2:55 AM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

...

Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works.

I admit my documentation has some shortcomings. I'll try to fill that gap first:

*_Concept #1: Reference counting_*

A DuktapeObject is meant to be a C++/OS extension of a Javascript object, for example to hold some system binding associated with the JS object.

The primary goal is to provide asynchronous operations on the system side, initiating JS callbacks when finishing/failing. For example a HTTP operation can be started by a script, passing some "done" callback. The system then starts the network operation asynchronously, the JS task can continue processing other scripts. Once the network operation is done, the DuktapeObject sends a request to the Duktape task to execute the "done" callback on itself.

So the DuktapeObject is normally shared by multiple contexts and parallel operations. Asynchronous operation also means the JS object or context may already be gone when the operation finishes. So the DuktapeObject needs to stay locked in memory independent of the JS context.

That's implemented by counting the active references to the DuktapeObject (methods Ref() / Unref()). The last Unref() automatically deletes the DuktapeObject instance.

For example: JS requests a network operation. The initial JS binding (see coupling) sets the reference count to 1. The DuktapeObject starts the network request, increasing the ref count to 2. If the JS context now dies (decreasing the reference count), the DuktapeObject will still remain valid until the network operation returns.

As the Ref/Unref operations need a (recursive) mutex, that's also part of the DuktapeObject and exposed by the API for other uses: Lock() / Unlock().

*_Concept #2: Coupling_*

Javascript does not have a destructor concept. JS objects get deleted by heap destruction or by the garbage collector when no reference to the JS object is left. In both cases, the actual object deletion is called "finalization", and a special finalizer method/callback can be installed on a JS object to be called just before the object gets deallocated. That is done by the DuktapeObject::Couple() method (implicitly called when constructed directly with a JS object reference).

There is no way to force finalization on a JS object. So a DuktapeObject cannot tell Duktape to delete it's coupled object, that means a DuktapeObject should normally not be deleted from outside the Duktape context, at least not if still coupled to the JS object. Coupling and decoupling can only be done in the Duktape context.

The standard finalizer DuktapeObject::Finalizer() simply decouples, automatically deleting itself if the coupling was the last reference. This is a virtual method, so can be overridden as necessary.

The coupling operation additionally adds a hidden pointer to the DuktapeObject instance in the JS object. That allows to check for and retreive associated DuktapeObject instances from any JS object, which is provided by the GetInstance() call.

*_Concept #3: Registration_*

For asynchronous operations, it's normally very convenient to have a "fire & forget" API. Example from the documentation: VFS.Save({ path: "/sd/mydata/telemetry.json", data: Duktape.enc('jx', telemetry), fail: function(error) { print("Error saving telemetry: " + error); } }); I.e. you simply pass the operation arguments including the done/fail callbacks to an API method and don't need to care about storing a reference to some handle. In JS that normally means the object used won't have any reference left after the call, so would be deleted by the garbage collector on the next run.

To avoid garbage collection and lock the JS object in memory, we need to store a reference to it in a "public" place. Duktape provides a special public place for this, hidden from scripts, called the global stash. DuktapeObject maintains a dedicated global object registry in that stash.

Adding and removing the coupled object reference to/from that registry is done by the Register() and Deregister() methods.

So for asynchronous system operations, or system integrations that shall be persistent, you normally do a Register() call together with the coupling, unless some ressource isn't available. Deregistration is then normally done when all pending JS callbacks have been executed, or when the persistent system integration has been unbound.

Other API designs are possible here: if you'd rather like the script needing to store a reference to your operation handle, you don't need to do a registration. The object will then be deleted (finalized) by the garbage collector automatically after the script deletes the reference.

*_Concept #4: Callback invocation_*

Triggers on the system side, for example a finished or failed network operation, shall normally trigger a JS method execution.

JS callback methods are simply passed as part of the arguments object in modern JS APIs. This allows to pass simple function definitions inline, as well as to reference a separately defined general handler function. JS allows functions to be excuted in the context of any object, and callbacks normally are executed in the context of the API object. This adds even more convenience, as the callbacks can easily access the other API arguments still stored in the object, as well as additional data added by the call.

JS callbacks cannot be executed directly from any system context, they need to run in the Duktape context. So the DuktapeObject callback invocation mechanism includes a general method to request a callback execution by Duktape: RequestCallback()

Note: RequestCallback() is an asynchronous operation. A synchronous variant can be added if necessary (and probably will be for command execution from a console).

A pending callback automatically increments the reference count, so the object is locked in memory until the callback has been executed (or aborted) by the Duktape task.

The callback invocation API provides a void* for simple data (e.g. a fixed string) to be passed to the callback method, but for more complex data, you will normally fill some DuktapeObject member variables before invoking the callback.

In Duktape context, the callback invocation translates the data returned or provided by the system side into the Duktape callback arguments and then runs the callback (if the object actually has the requested callback set). The default implementation for this is DuktapeObject::CallMethod(), which can be used directly for simple callbacks without arguments. For more complex handling, override this with your custom implementation.

The callbacks are by default executed on the coupled JS object, so data can also be transported by setting properties on that object. The callback can then simply access them via "this".

To simplify callback invocation from code parts that may run outside or inside Duktape, it's convenient to allow calling CallMethod() without a Duktape context, and let CallMethod() translate that into a RequestCallback() call as necessary. Pattern:

duk_ret_t DuktapeHTTPRequest::CallMethod(duk_context *ctx, const char* method, void* data /*=NULL*/)   {   if (!ctx)     {     RequestCallback(method, data);     return 0;     }   …

A CallMethod() implementation isn't limited to executing a single callback. A common example is an API defining "done" & "fail" callbacks, as well as a general final "always" callback. DuktapeHTTPRequest::CallMethod() also serves as an example implementation for this.

Wow… that's become more to write & read than I expected. Please provide some feedback: is that explanation sufficient & clear? I'll refine it for the developer docs then.

Regards, Michael

Am 01.09.20 um 19:52 schrieb Michael Balzer:

...

Mark,

I'll have a look.

Regards, Michael

Am 01.09.20 um 07:30 schrieb Mark Webb-Johnson:

...

Michael,

Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works.

Here are some notes one what I have done so far:

1. Created a stub DuktapeConsoleCommand (derived from DuktapeObject) in ovms_duktape.{h,cpp}. This should hold enough to be able to call the javascript callback method for that object. It also stores the module filename (so the registration can be removed when the module is unloaded).

2. Provide a DuktapeCommandMap m_cmdmap in ovms_duktape.{h,cpp} OvmsDuktape class that stores a mapping from OvmsCommand to DuktapeConsoleCommand.

3. Created a OvmsDuktape::RegisterDuktapeConsoleCommand in ovms_duktape.{h,cpp) that (a) creates the OvmsCommand() object, (b) registers it, (c) creates the DuktapeConsoleCommand() object, and (d) updates a map from OvmsCommand->DuktapeConsoleCommand. There Is also a single callback DukOvmsCommandRegisterRun designed to be run by all.

4. Created hooks NotifyDuktapeModuleLoad, NotifyDuktapeModuleUnload, and NotifyDuktapeModuleUnloadAll in OvmsDuktape. The javascript module is identified by filename (path to module or script on vfs, usually, but may also be an internal module). The Unload functions look through the m_cmdmap and unregister commands for javascript modules being unloaded.

5. Provide an implementation for ovms_command DukOvmsCommandRegister to support registering commands from Javascript modules. This should extract the details, and then call OvmsDuktape::RegisterDuktapeConsoleCommand to do the actual registration. This has been implemented, except for the callback method (and somehow passing that method from Javascript in the OvmsCommand.Register javascript call).

6. Provide a stub implementation for DukOvmsCommandRegisterRun. This uses m_cmdmap to lookup the DuktapeConsoleCommand object for the command to be run. It should execute the callback method (but that part is not yet implemented).

I still need help with #5 and #6. What needs to be implemented in DuktapeConsoleCommand, and how is the parameter in OvmsCommand.Register used to store the callback (#5)? Then how to callback the command method from DukOvmsCommandRegisterRun (#6)? If you have time, it is probably much quicker for you to simply make those changes.

An alternative implementation would be to do something like the pubsub framework, where the mapping command->callback is done from within a javascript module. That I could do, but it seems your DuktapeObject can do it better. 

Thanks, Mark.

...

On 15 Jul 2020, at 3:34 PM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

yes, I needed that persistence for the HTTP and VFS classes, but I also needed to be able to couple a dynamic C++ instance with a JS object and have a mechanism to prevent garbage collection while the C++ side is still in use. If the C++ side is no longer needed, the JS finalizer also needs to imply the C++ instance can be deleted.

That is all implemented by DuktapeObject. DuktapeObject also provides JS method invocation on the coupled JS object and a mutex for concurrency protection.

We probably need some more framework documentation than the header comments (applies to all of our framework classes…):

/***************************************************************************************************  * DuktapeObject: coupled C++ / JS object  *  *  Intended for API methods to attach internal API state to a JS object and provide  *    a standard callback invocation interface for JS objects in local scopes.  *   *  - Override CallMethod() to implement specific method calls  *  - Override Finalize() for specific destruction in JS context (garbage collection)  *  - call Register() to prevent normal garbage collection (but not heap destruction)  *  - call Ref() to protect against deletion (reference count)  *  - call Lock() to protect concurrent access (recursive mutex)  *   *  - GetInstance() retrieves the DuktapeObject associated with a JS object if any  *  - Push() pushes the JS object onto the Duktape stack  *   *  Note: the DuktapeObject may persist after the JS object has been finalized, e.g.  *    if some callbacks are pending after the Duktape heap has been destroyed.  *    Use IsCoupled() to check if the JS object is still available.  *   *  Ref/Unref:  *    Normal life cycle is from construction to finalization. Pending callbacks extend  *    the life until the last callback has been processed. A subclass may extend the life  *    by calling Ref(), which increases the reference count. Unref() deletes the instance  *    if no references are left.  */

You normally just need to use Register/Deregister & Ref/Unref, and to implement the constructor and CallMethod. Coupling of the instances normally is done on construction, as a JS object is normally already needed for the parameters and can simply be attached to.

Have a look at DuktapeHTTPRequest, DuktapeVFSLoad and DuktapeVFSSave, these are the current subclasses using this.

For the command registration I would probably couple the OvmsCommand instance with a JS command object providing an execution method.

Tell me if you need more info.

Regards, Michael

Am 15.07.20 um 08:12 schrieb Mark Webb-Johnson:

...

@Michael this is probably for you.

I am trying to implement javascript command registration. The idea is that a javascript module can call something like:

OvmsCommand.Register(basecommand, name, title, callbackfn, usage, min, max)

Then we reflect that into MyCommandApp.RegisterCommand, and keep a track of which command is for which javascript callbackfn. When the command is executed, we pass it into duktape.

I also have tracking for javascript module loading and unloading, so I can DeregisterCommand() if duktape is reloaded (and also protected against commands being registered in short-lived scripts run from the command line).

To implement this, I need to store the callbackfn as a persistent reference to a duktape javascript function.

The issue with callback function references in duktape is summarised here:

https://wiki.duktape.org/howtonativepersistentreferences

/When a Duktape/C function is called, Duktape places the call arguments on the value stack. While the arguments are on the value stack, they're guaranteed to be reachable and the Duktape/C function can safely work with the arguments.

However, when the Duktape/C function returns, the value stack is unwound and references in the function's value stack frame are lost. If the last reference to a particular value was in the function's value stack frame, the value will be garbage collected when the function return is processed./

The standard approach is to store the reference back in the duktape duk_push_global_stash so it won’t get garbage-collected. But, that seems messy.

I see that Michael has already implemented something that seems similar in ovms_script.{h, cpp}, for the async http callbacks. Presumably to avoid this issue. But, the approach seems very different, and I am not sure if it is stopping _all_ garbage collection for the duration of the async query, or just that particular object being garbage collected. The work seems extensive (quite a few objects involved).

So @Michael, any suggestions for this? I don’t want to reinvent the wheel...

Regards, Mark.

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Am 11.09.20 um 08:53 schrieb Mark Webb-Johnson:

...

There is no need to extend OvmsCommand. The DuktapeObject is meant to be used as a general binding of arbitrary system objects to Duktape, so the system objects don't need to be extended.

I agree it is not necessary; my suggestion is purely to clean it up and enhance functionality. The problem at the moment is that OvmsCommand execute callbacks can only be to function callbacks (not objects). It doesn’t even use the c++ bind function callback mechanism (like notification, etc, for example). It is what it is, and changing now is very hard.

That slipped my attention, but upgrading OvmsCommand to accept any function type should be simply changing m_execute and m_validate to std::function, or do I miss something?

However, making the execute function virtual is relatively simple and allows the object to be virtualised. Alternatively, we could provide an alternative proper c++ function callback variant.

Without that, we need something kludgy like DukOvmsCommandRegisterRun that has to try to find the DuktapeConsoleCommand command in order to be able to call the execute callback in duktape. The current implementation needs to keep a map of OvmsCommand => DuktapeConsoleCommand in order to do this (which is messy).

I think the map wouldn't be necessary for the command invocation, but it would still be necessary to check/get the associated DuktapeConsoleCommand of a parent OvmsCommand pointer.

...

My API design would be like this:

OK

...

The forced unregistering limits registering commands to library plugins (which don't have an unload operation yet). I think this is an unnecessary limitation, I would like to be able to register a command by adding a registration call to "ovmsmain.js" or by executing a script. Of course that means commands registered that way need to be unregistered explicitly, but that's up to the script developers & users then. I don't think we should limit them here without good reason.

On a Duktape unload/reload operation, all JS objects get finalized anyway because of heap destruction, so deletion would happen automatically by the DuktapeConsoleCommand finalizer. If we add some module unload operation later on, modules may e.g. define a cleanup callback, or we can add the module filename to the DuktapeObject registry -- the latter option would also automatically apply to all DuktapeObject instances, eliminating the need for separate registries for other system bindings potentially to be added in the future.

Implicit OvmsCommand deletions can happen as the order of deletion is undefined. Inhibiting this should be a straight forward use of the reference count though: on registration of a sub command, simply Ref() all parent commands registered by Duktape as well. Unref() them after the sub command has been removed, the last Unref() on a parent then automatically deletes it (the first Ref() is done by the coupling).

OK. My only concern would be timing. Say there is a script that is run, registers and command, then exits. The command will be cleaned up in the next duktape memory clean. But before then, the command could be called. If everything is running in that one duktape thread, there should be no crash, I think, but it sounds scary to me.

I would keep the command registered until it's explicitly unregistered or deleted by a heap destruction. I also suggest keeping a Ref() on the DuktapeConsoleCommand until the command has been fully deregistered… Hm. Deregistration can still occur concurrently to the OvmsCommand deletion… I think we haven't taken care of concurrent command deregistration at all, there is no lock/mutex in OvmsCommandApp. So currently any subsystem deregistering it's commands can crash the system if one of those commands is just about to be executed. Maybe we need to address this in OvmsCommandApp or …Map. Regards, Michael

Overall, your suggested approach sounds ok.

Regards, Mark.

...

On 8 Sep 2020, at 4:13 PM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

for the synchronous callback execution, that's simply a variant of OvmsDuktape::DuktapeRequestCallback() using the already existing DuktapeDispatchWait() method along with passing the OvmsWriter* in dmsg.writer. Could be named "…ExecuteCallback" to reflect the synchronous nature.

There is no need to extend OvmsCommand. The DuktapeObject is meant to be used as a general binding of arbitrary system objects to Duktape, so the system objects don't need to be extended.

From my first check of your implementation, I would probably:

* Redefine the register API to expect an object with an "execute" (and later possibly also a "validate") callback, and bind the DuktapeCommandObject to that object. A "fail" callback could also make sense. * Delegate the full OvmsCommand life cycle and execution handling to the DuktapeCommandObject. * Remove the forced unregistering of commands on script file unloading. * Inhibit implicit sub command deletions by parent deletions.

My API design would be like this:

// Command registration: var cmd = OvmsCommand.Register({   parent: "test",   name: "dukcommand",   title: "Test/demonstrate Duktape command registration",   usage: "Pass 1 - 3 arguments",   min: 1,   max: 3,   execute: function(verbosity, argv) {},   fail: function(error) {} });

// Command deregistration: OvmsCommand.Unregister({ parent: "test", name: "dukcommand" }); // …or in case we still have the cmd object, simply: OvmsCommand.Unregister(cmd);

The OvmsCommand execution callback would be a bound method of the DuktapeCommandObject instance. The method passes the verbosity and arguments vector to ExecuteMethod() via the data pointer using a custom struct. CallMethod() then pushes these on the Duktape stack and calls the "execute" property.

The forced unregistering limits registering commands to library plugins (which don't have an unload operation yet). I think this is an unnecessary limitation, I would like to be able to register a command by adding a registration call to "ovmsmain.js" or by executing a script. Of course that means commands registered that way need to be unregistered explicitly, but that's up to the script developers & users then. I don't think we should limit them here without good reason.

On a Duktape unload/reload operation, all JS objects get finalized anyway because of heap destruction, so deletion would happen automatically by the DuktapeConsoleCommand finalizer. If we add some module unload operation later on, modules may e.g. define a cleanup callback, or we can add the module filename to the DuktapeObject registry -- the latter option would also automatically apply to all DuktapeObject instances, eliminating the need for separate registries for other system bindings potentially to be added in the future.

Implicit OvmsCommand deletions can happen as the order of deletion is undefined. Inhibiting this should be a straight forward use of the reference count though: on registration of a sub command, simply Ref() all parent commands registered by Duktape as well. Unref() them after the sub command has been removed, the last Unref() on a parent then automatically deletes it (the first Ref() is done by the coupling).

Regards, Michael

Am 08.09.20 um 09:06 schrieb Mark Webb-Johnson:

...

Michael,

Very clear, and very helpful. Only thing I would suggest would be to have a minimum example in the documentation. The bare minimum required for an implementation of an object.

Reading through what you write, it seems the correct approach is:

* Extend OvmsCommand to have a virtual ExecuteCommand method (same parameters as ‘m_execute’ callback). * Extend OvmsCommand::execute to check if m_execute is null, then call ‘ExecuteCommand’ instead. * Perhaps do the same for m_validate in OvmsCommand. * Then, the duktape implementation can be object (rather than callback function) based, as your DuktapeObject expects. The javascript would call a function to register a command, with a command object to be used for the callback. * Need to extend the DuktapeObject system to support a synchronous command (presumably implemented with a passed mutex like we do in several other parts of the system).

Is that correct, and what you were expecting? Or any other suggestions?

Regards, Mark.

...

On 7 Sep 2020, at 2:55 AM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

...

Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works.

I admit my documentation has some shortcomings. I'll try to fill that gap first:

*_Concept #1: Reference counting_*

A DuktapeObject is meant to be a C++/OS extension of a Javascript object, for example to hold some system binding associated with the JS object.

The primary goal is to provide asynchronous operations on the system side, initiating JS callbacks when finishing/failing. For example a HTTP operation can be started by a script, passing some "done" callback. The system then starts the network operation asynchronously, the JS task can continue processing other scripts. Once the network operation is done, the DuktapeObject sends a request to the Duktape task to execute the "done" callback on itself.

So the DuktapeObject is normally shared by multiple contexts and parallel operations. Asynchronous operation also means the JS object or context may already be gone when the operation finishes. So the DuktapeObject needs to stay locked in memory independent of the JS context.

That's implemented by counting the active references to the DuktapeObject (methods Ref() / Unref()). The last Unref() automatically deletes the DuktapeObject instance.

For example: JS requests a network operation. The initial JS binding (see coupling) sets the reference count to 1. The DuktapeObject starts the network request, increasing the ref count to 2. If the JS context now dies (decreasing the reference count), the DuktapeObject will still remain valid until the network operation returns.

As the Ref/Unref operations need a (recursive) mutex, that's also part of the DuktapeObject and exposed by the API for other uses: Lock() / Unlock().

*_Concept #2: Coupling_*

Javascript does not have a destructor concept. JS objects get deleted by heap destruction or by the garbage collector when no reference to the JS object is left. In both cases, the actual object deletion is called "finalization", and a special finalizer method/callback can be installed on a JS object to be called just before the object gets deallocated. That is done by the DuktapeObject::Couple() method (implicitly called when constructed directly with a JS object reference).

There is no way to force finalization on a JS object. So a DuktapeObject cannot tell Duktape to delete it's coupled object, that means a DuktapeObject should normally not be deleted from outside the Duktape context, at least not if still coupled to the JS object. Coupling and decoupling can only be done in the Duktape context.

The standard finalizer DuktapeObject::Finalizer() simply decouples, automatically deleting itself if the coupling was the last reference. This is a virtual method, so can be overridden as necessary.

The coupling operation additionally adds a hidden pointer to the DuktapeObject instance in the JS object. That allows to check for and retreive associated DuktapeObject instances from any JS object, which is provided by the GetInstance() call.

*_Concept #3: Registration_*

For asynchronous operations, it's normally very convenient to have a "fire & forget" API. Example from the documentation: VFS.Save({ path: "/sd/mydata/telemetry.json", data: Duktape.enc('jx', telemetry), fail: function(error) { print("Error saving telemetry: " + error); } }); I.e. you simply pass the operation arguments including the done/fail callbacks to an API method and don't need to care about storing a reference to some handle. In JS that normally means the object used won't have any reference left after the call, so would be deleted by the garbage collector on the next run.

To avoid garbage collection and lock the JS object in memory, we need to store a reference to it in a "public" place. Duktape provides a special public place for this, hidden from scripts, called the global stash. DuktapeObject maintains a dedicated global object registry in that stash.

Adding and removing the coupled object reference to/from that registry is done by the Register() and Deregister() methods.

So for asynchronous system operations, or system integrations that shall be persistent, you normally do a Register() call together with the coupling, unless some ressource isn't available. Deregistration is then normally done when all pending JS callbacks have been executed, or when the persistent system integration has been unbound.

Other API designs are possible here: if you'd rather like the script needing to store a reference to your operation handle, you don't need to do a registration. The object will then be deleted (finalized) by the garbage collector automatically after the script deletes the reference.

*_Concept #4: Callback invocation_*

Triggers on the system side, for example a finished or failed network operation, shall normally trigger a JS method execution.

JS callback methods are simply passed as part of the arguments object in modern JS APIs. This allows to pass simple function definitions inline, as well as to reference a separately defined general handler function. JS allows functions to be excuted in the context of any object, and callbacks normally are executed in the context of the API object. This adds even more convenience, as the callbacks can easily access the other API arguments still stored in the object, as well as additional data added by the call.

JS callbacks cannot be executed directly from any system context, they need to run in the Duktape context. So the DuktapeObject callback invocation mechanism includes a general method to request a callback execution by Duktape: RequestCallback()

Note: RequestCallback() is an asynchronous operation. A synchronous variant can be added if necessary (and probably will be for command execution from a console).

A pending callback automatically increments the reference count, so the object is locked in memory until the callback has been executed (or aborted) by the Duktape task.

The callback invocation API provides a void* for simple data (e.g. a fixed string) to be passed to the callback method, but for more complex data, you will normally fill some DuktapeObject member variables before invoking the callback.

In Duktape context, the callback invocation translates the data returned or provided by the system side into the Duktape callback arguments and then runs the callback (if the object actually has the requested callback set). The default implementation for this is DuktapeObject::CallMethod(), which can be used directly for simple callbacks without arguments. For more complex handling, override this with your custom implementation.

The callbacks are by default executed on the coupled JS object, so data can also be transported by setting properties on that object. The callback can then simply access them via "this".

To simplify callback invocation from code parts that may run outside or inside Duktape, it's convenient to allow calling CallMethod() without a Duktape context, and let CallMethod() translate that into a RequestCallback() call as necessary. Pattern:

duk_ret_t DuktapeHTTPRequest::CallMethod(duk_context *ctx, const char* method, void* data /*=NULL*/)   {   if (!ctx)     {     RequestCallback(method, data);     return 0;     }   …

A CallMethod() implementation isn't limited to executing a single callback. A common example is an API defining "done" & "fail" callbacks, as well as a general final "always" callback. DuktapeHTTPRequest::CallMethod() also serves as an example implementation for this.

Wow… that's become more to write & read than I expected. Please provide some feedback: is that explanation sufficient & clear? I'll refine it for the developer docs then.

Regards, Michael

Am 01.09.20 um 19:52 schrieb Michael Balzer:

...

Mark,

I'll have a look.

Regards, Michael

Am 01.09.20 um 07:30 schrieb Mark Webb-Johnson:

...

Michael,

Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works.

Here are some notes one what I have done so far:

1. Created a stub DuktapeConsoleCommand (derived from DuktapeObject) in ovms_duktape.{h,cpp}. This should hold enough to be able to call the javascript callback method for that object. It also stores the module filename (so the registration can be removed when the module is unloaded).

2. Provide a DuktapeCommandMap m_cmdmap in ovms_duktape.{h,cpp} OvmsDuktape class that stores a mapping from OvmsCommand to DuktapeConsoleCommand.

3. Created a OvmsDuktape::RegisterDuktapeConsoleCommand in ovms_duktape.{h,cpp) that (a) creates the OvmsCommand() object, (b) registers it, (c) creates the DuktapeConsoleCommand() object, and (d) updates a map from OvmsCommand->DuktapeConsoleCommand. There Is also a single callback DukOvmsCommandRegisterRun designed to be run by all.

4. Created hooks NotifyDuktapeModuleLoad, NotifyDuktapeModuleUnload, and NotifyDuktapeModuleUnloadAll in OvmsDuktape. The javascript module is identified by filename (path to module or script on vfs, usually, but may also be an internal module). The Unload functions look through the m_cmdmap and unregister commands for javascript modules being unloaded.

5. Provide an implementation for ovms_command DukOvmsCommandRegister to support registering commands from Javascript modules. This should extract the details, and then call OvmsDuktape::RegisterDuktapeConsoleCommand to do the actual registration. This has been implemented, except for the callback method (and somehow passing that method from Javascript in the OvmsCommand.Register javascript call).

6. Provide a stub implementation for DukOvmsCommandRegisterRun. This uses m_cmdmap to lookup the DuktapeConsoleCommand object for the command to be run. It should execute the callback method (but that part is not yet implemented).

I still need help with #5 and #6. What needs to be implemented in DuktapeConsoleCommand, and how is the parameter in OvmsCommand.Register used to store the callback (#5)? Then how to callback the command method from DukOvmsCommandRegisterRun (#6)? If you have time, it is probably much quicker for you to simply make those changes.

An alternative implementation would be to do something like the pubsub framework, where the mapping command->callback is done from within a javascript module. That I could do, but it seems your DuktapeObject can do it better. 

Thanks, Mark.

> On 15 Jul 2020, at 3:34 PM, Michael Balzer <dexter@expeedo.de > <mailto:dexter@expeedo.de>> wrote: > > Mark, > > yes, I needed that persistence for the HTTP and VFS classes, but > I also needed to be able to couple a dynamic C++ instance with a > JS object and have a mechanism to prevent garbage collection > while the C++ side is still in use. If the C++ side is no longer > needed, the JS finalizer also needs to imply the C++ instance > can be deleted. > > That is all implemented by DuktapeObject. DuktapeObject also > provides JS method invocation on the coupled JS object and a > mutex for concurrency protection. > > We probably need some more framework documentation than the > header comments (applies to all of our framework classes…): > > /*************************************************************************************************** >  * DuktapeObject: coupled C++ / JS object >  * >  *  Intended for API methods to attach internal API state to a > JS object and provide >  *    a standard callback invocation interface for JS objects in > local scopes. >  *  >  *  - Override CallMethod() to implement specific method calls >  *  - Override Finalize() for specific destruction in JS context > (garbage collection) >  *  - call Register() to prevent normal garbage collection (but > not heap destruction) >  *  - call Ref() to protect against deletion (reference count) >  *  - call Lock() to protect concurrent access (recursive mutex) >  *  >  *  - GetInstance() retrieves the DuktapeObject associated with > a JS object if any >  *  - Push() pushes the JS object onto the Duktape stack >  *  >  *  Note: the DuktapeObject may persist after the JS object has > been finalized, e.g. >  *    if some callbacks are pending after the Duktape heap has > been destroyed. >  *    Use IsCoupled() to check if the JS object is still available. >  *  >  *  Ref/Unref: >  *    Normal life cycle is from construction to finalization. > Pending callbacks extend >  *    the life until the last callback has been processed. A > subclass may extend the life >  *    by calling Ref(), which increases the reference count. > Unref() deletes the instance >  *    if no references are left. >  */ > > You normally just need to use Register/Deregister & Ref/Unref, > and to implement the constructor and CallMethod. Coupling of the > instances normally is done on construction, as a JS object is > normally already needed for the parameters and can simply be > attached to. > > Have a look at DuktapeHTTPRequest, DuktapeVFSLoad and > DuktapeVFSSave, these are the current subclasses using this. > > For the command registration I would probably couple the > OvmsCommand instance with a JS command object providing an > execution method. > > Tell me if you need more info. > > Regards, > Michael > > > Am 15.07.20 um 08:12 schrieb Mark Webb-Johnson: >> >> @Michael this is probably for you. >> >> I am trying to implement javascript command registration. The >> idea is that a javascript module can call something like: >> >> OvmsCommand.Register(basecommand, name, title, callbackfn, >> usage, min, max) >> >> >> Then we reflect that into MyCommandApp.RegisterCommand, and >> keep a track of which command is for which javascript >> callbackfn. When the command is executed, we pass it into duktape. >> >> I also have tracking for javascript module loading and >> unloading, so I can DeregisterCommand() if duktape is reloaded >> (and also protected against commands being registered in >> short-lived scripts run from the command line). >> >> To implement this, I need to store the callbackfn as a >> persistent reference to a duktape javascript function. >> >> The issue with callback function references in duktape is >> summarised here: >> >> https://wiki.duktape.org/howtonativepersistentreferences >> >> /When a Duktape/C function is called, Duktape places the >> call arguments on the value stack. While the arguments are >> on the value stack, they're guaranteed to be reachable and >> the Duktape/C function can safely work with the arguments. >> >> However, when the Duktape/C function returns, the value >> stack is unwound and references in the function's value >> stack frame are lost. If the last reference to a particular >> value was in the function's value stack frame, the value >> will be garbage collected when the function return is >> processed./ >> >> >> The standard approach is to store the reference back in the >> duktape duk_push_global_stash so it won’t get >> garbage-collected. But, that seems messy. >> >> I see that Michael has already implemented something that seems >> similar in ovms_script.{h, cpp}, for the async http callbacks. >> Presumably to avoid this issue. But, the approach seems very >> different, and I am not sure if it is stopping _all_ garbage >> collection for the duration of the async query, or just that >> particular object being garbage collected. The work seems >> extensive (quite a few objects involved). >> >> So @Michael, any suggestions for this? I don’t want to reinvent >> the wheel... >> >> Regards, Mark. >> >> _______________________________________________ >> OvmsDev mailing list >> OvmsDev@lists.openvehicles.com >> http://lists.openvehicles.com/mailman/listinfo/ovmsdev > > -- > Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal > Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 > _______________________________________________ > OvmsDev mailing list > OvmsDev@lists.openvehicles.com > <mailto:OvmsDev@lists.openvehicles.com> > http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Michael, Like Mark, I did not think the change would be simple. I've used functional objects before, but was not aware of std::function. I am still not quite sure how a vanilla function pointer matches this type; I need to think about that more carefully. Having the typedefs is a good idea even if we weren't using std:function. -- Steve On Fri, 11 Sep 2020, Michael Balzer wrote:

Mark,

Am 11.09.20 um 18:11 schrieb Michael Balzer:

...

...

I agree it is not necessary; my suggestion is purely to clean it up and enhance functionality. The problem at the moment is that OvmsCommand execute callbacks can only be to function callbacks (not objects). It doesn't even use the c++ bind function callback mechanism (like notification, etc, for example). It is what it is, and changing now is very hard.

That slipped my attention, but upgrading OvmsCommand to accept any function type should be simply changing m_execute and m_validate to std::function, or do I miss something?

Just did this locally, works perfectly. Shall I push the change or have you begun working on ovms_command?

If so, it's really just exchanging the function signatures for these types:

typedef std::function<void(int, OvmsWriter*, OvmsCommand*, int, const char* const*)> OvmsCommandExecuteCallback_t; typedef std::function<int(OvmsWriter*, OvmsCommand*, int, const char* const*, bool)> OvmsCommandValidateCallback_t;

Patch attached.

Regards, Michael

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

Michael, This looks good, and works for me. In for-3.3 branch, I am only working on the plugin code at the moment. Please commit this change to the branch. Thanks, Mark.

On 12 Sep 2020, at 2:26 PM, Michael Balzer <dexter@expeedo.de> wrote:

Steve,

std::function has constructors that accept function pointers, or NULL to create an empty function object.

std::function of course has a little overhead over a simple pointer. But that overhead isn't relevant for this use case. In fact, we use std::function already in places that are more performance & memory sensitive.

See: https://www.boost.org/doc/libs/1_45_0/doc/html/function/misc.html

Regards, Michael

Am 11.09.20 um 23:31 schrieb Stephen Casner:

...

Michael,

Like Mark, I did not think the change would be simple. I've used functional objects before, but was not aware of std::function. I am still not quite sure how a vanilla function pointer matches this type; I need to think about that more carefully.

Having the typedefs is a good idea even if we weren't using std:function.

-- Steve

On Fri, 11 Sep 2020, Michael Balzer wrote:

...

Mark,

Am 11.09.20 um 18:11 schrieb Michael Balzer:

...

...

I agree it is not necessary; my suggestion is purely to clean it up and enhance functionality. The problem at the moment is that OvmsCommand execute callbacks can only be to function callbacks (not objects). It doesn't even use the c++ bind function callback mechanism (like notification, etc, for example). It is what it is, and changing now is very hard. That slipped my attention, but upgrading OvmsCommand to accept any function type should be simply changing m_execute and m_validate to std::function, or do I miss something? Just did this locally, works perfectly. Shall I push the change or have you begun working on ovms_command?

If so, it's really just exchanging the function signatures for these types:

typedef std::function<void(int, OvmsWriter*, OvmsCommand*, int, const char* const*)> OvmsCommandExecuteCallback_t; typedef std::function<int(OvmsWriter*, OvmsCommand*, int, const char* const*, bool)> OvmsCommandValidateCallback_t;

Patch attached.

Regards, Michael

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

---

OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

Mark, the issue is, there are ambiguous overloads of these methods in their classes, and the compiler doesn't know which one to pick. - https://stackoverflow.com/questions/10111042/wrap-overloaded-function-via-st... - https://stackoverflow.com/questions/30393285/stdfunction-fails-to-distinguis... I prefer to keep my command stub names distinguished from their implementation counterparts, but apparently that doesn't apply to all of us ;-) Coming C++ versions may be smart enough to deduce the method to pick from the expected signature. For now, the compiler needs a hint here. I've added two preprocessor macros to make these hints readable. Regards, Michael Am 25.09.20 um 08:07 schrieb Mark Webb-Johnson:

@Michael,

I’ve tried merging this (along with all the other recent changes in master) into the for-v3.3 branch, but am now getting compilation errors. The two files with issues are:

components/retools_testerpresent/src/retools_testerpresent.cpp components/vehicle_mgev/src/vehicle_mgev.cpp

Both in the RegisterCommand function call with the static class member function as a parameter. Not sure if that new way is used elsewhere?

Example compiler output is:

CXX build/retools_testerpresent/src/retools_testerpresent.o .../components/retools_testerpresent/src/retools_testerpresent.cpp: In constructor 'OvmsReToolsTesterPresentInit::OvmsReToolsTesterPresentInit()': .../components/retools_testerpresent/src/retools_testerpresent.cpp:140:5: error: no matching function for call to 'OvmsCommand::RegisterCommand(const char [6], const char [48], <unresolved overloaded function type>, const char [23], int, int)'      );      ^

CXX build/vehicle_mgev/src/vehicle_mgev.o .../components/vehicle_mgev/src/vehicle_mgev.cpp: In constructor 'OvmsVehicleMgEv::OvmsVehicleMgEv()': .../components/vehicle_mgev/src/vehicle_mgev.cpp:143:5: error: no matching function for call to 'OvmsCommandApp::RegisterCommand(const char [8], const char [15], <unresolved overloaded function type>)'      );      ^mand(const char* name, const char* title,                   ^

I tried for a while, but can’t see what is wrong. Perhaps some missing header file? Anyway, I’ve committed the merge of master into for-v3.3 (even though it doesn’t compile) and just commented out those registrations in my local copy.

Can you help?

Thanks, Mark.

...

On 12 Sep 2020, at 5:12 AM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

Am 11.09.20 um 18:11 schrieb Michael Balzer:

...

...

I agree it is not necessary; my suggestion is purely to clean it up and enhance functionality. The problem at the moment is that OvmsCommand execute callbacks can only be to function callbacks (not objects). It doesn’t even use the c++ bind function callback mechanism (like notification, etc, for example). It is what it is, and changing now is very hard.

That slipped my attention, but upgrading OvmsCommand to accept any function type should be simply changing m_execute and m_validate to std::function, or do I miss something?

Just did this locally, works perfectly. Shall I push the change or have you begun working on ovms_command?

If so, it's really just exchanging the function signatures for these types:

typedef std::function<void(int, OvmsWriter*, OvmsCommand*, int, const char* const*)> OvmsCommandExecuteCallback_t; typedef std::function<int(OvmsWriter*, OvmsCommand*, int, const char* const*, bool)> OvmsCommandValidateCallback_t;

Patch attached.

Regards, Michael

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

<ovmscommand.patch>_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

I understood I should add the change to std::function in ovms_command only to the for-v3.3 branch, so Chris didn't take that into account yet. Regards, Michael Am 25.09.20 um 15:22 schrieb Mark Webb-Johnson:

Wow. That would have taken me hours to work out. Thanks.

Any idea why it compiled in master, but not in for-v3.3?

Regards, Mark.

...

On 25 Sep 2020, at 9:10 PM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

the issue is, there are ambiguous overloads of these methods in their classes, and the compiler doesn't know which one to pick.

- https://stackoverflow.com/questions/10111042/wrap-overloaded-function-via-st... - https://stackoverflow.com/questions/30393285/stdfunction-fails-to-distinguis...

I prefer to keep my command stub names distinguished from their implementation counterparts, but apparently that doesn't apply to all of us ;-)

Coming C++ versions may be smart enough to deduce the method to pick from the expected signature.

For now, the compiler needs a hint here. I've added two preprocessor macros to make these hints readable.

Regards, Michael

Am 25.09.20 um 08:07 schrieb Mark Webb-Johnson:

...

@Michael,

I’ve tried merging this (along with all the other recent changes in master) into the for-v3.3 branch, but am now getting compilation errors. The two files with issues are:

components/retools_testerpresent/src/retools_testerpresent.cpp components/vehicle_mgev/src/vehicle_mgev.cpp

Both in the RegisterCommand function call with the static class member function as a parameter. Not sure if that new way is used elsewhere?

Example compiler output is:

CXX build/retools_testerpresent/src/retools_testerpresent.o .../components/retools_testerpresent/src/retools_testerpresent.cpp: In constructor 'OvmsReToolsTesterPresentInit::OvmsReToolsTesterPresentInit()': .../components/retools_testerpresent/src/retools_testerpresent.cpp:140:5: error: no matching function for call to 'OvmsCommand::RegisterCommand(const char [6], const char [48], <unresolved overloaded function type>, const char [23], int, int)'      );      ^

CXX build/vehicle_mgev/src/vehicle_mgev.o .../components/vehicle_mgev/src/vehicle_mgev.cpp: In constructor 'OvmsVehicleMgEv::OvmsVehicleMgEv()': .../components/vehicle_mgev/src/vehicle_mgev.cpp:143:5: error: no matching function for call to 'OvmsCommandApp::RegisterCommand(const char [8], const char [15], <unresolved overloaded function type>)'      );      ^mand(const char* name, const char* title,                   ^

I tried for a while, but can’t see what is wrong. Perhaps some missing header file? Anyway, I’ve committed the merge of master into for-v3.3 (even though it doesn’t compile) and just commented out those registrations in my local copy.

Can you help?

Thanks, Mark.

...

On 12 Sep 2020, at 5:12 AM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

Am 11.09.20 um 18:11 schrieb Michael Balzer:

...

...

I agree it is not necessary; my suggestion is purely to clean it up and enhance functionality. The problem at the moment is that OvmsCommand execute callbacks can only be to function callbacks (not objects). It doesn’t even use the c++ bind function callback mechanism (like notification, etc, for example). It is what it is, and changing now is very hard.

That slipped my attention, but upgrading OvmsCommand to accept any function type should be simply changing m_execute and m_validate to std::function, or do I miss something?

Just did this locally, works perfectly. Shall I push the change or have you begun working on ovms_command?

If so, it's really just exchanging the function signatures for these types:

typedef std::function<void(int, OvmsWriter*, OvmsCommand*, int, const char* const*)> OvmsCommandExecuteCallback_t; typedef std::function<int(OvmsWriter*, OvmsCommand*, int, const char* const*, bool)> OvmsCommandValidateCallback_t;

Patch attached.

Regards, Michael

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

<ovmscommand.patch>_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

I think the map wouldn't be necessary for the command invocation, but it would still be necessary to check/get the associated DuktapeConsoleCommand of a parent OvmsCommand pointer.

My intention here is to avoid the risk of maps of pointers (where the object pointed to is destructed by something externally. I guess we can address this by removing from the map within the destructor of the object? But also need to be careful to mutex protect that. I was trying to find a way of avoiding that map altogether. For example if the DuktapeConsoleCommand object was also derived from OvmsCommand. Perhaps with the now std::function implementation we can register the command callback direct to a handler in the DuktapeConsoleCommand object, and keep a reference to the registered OvmsCommand object there? I’m not sure this has been much of an issue before, as commands don’t often get deregistered. Certainly not in the normal course of operation. But with javascript Duktape it could be fairly command (for example every time the scripts are reloaded). I do think that there is great opportunity here for Duktape - not just for commands and extensions, but also for simpler vehicle implementations (for example polling ones). The barrier to entry for development is certainly dramatically lower than with c++ espressif IDE, and the dynamic development environment should help. It is worth the effort to get this right. Regards, Mark.

On 12 Sep 2020, at 12:11 AM, Michael Balzer <dexter@expeedo.de> wrote:

Am 11.09.20 um 08:53 schrieb Mark Webb-Johnson:

...

...

There is no need to extend OvmsCommand. The DuktapeObject is meant to be used as a general binding of arbitrary system objects to Duktape, so the system objects don't need to be extended.

I agree it is not necessary; my suggestion is purely to clean it up and enhance functionality. The problem at the moment is that OvmsCommand execute callbacks can only be to function callbacks (not objects). It doesn’t even use the c++ bind function callback mechanism (like notification, etc, for example). It is what it is, and changing now is very hard.

That slipped my attention, but upgrading OvmsCommand to accept any function type should be simply changing m_execute and m_validate to std::function, or do I miss something?

...

However, making the execute function virtual is relatively simple and allows the object to be virtualised. Alternatively, we could provide an alternative proper c++ function callback variant.

Without that, we need something kludgy like DukOvmsCommandRegisterRun that has to try to find the DuktapeConsoleCommand command in order to be able to call the execute callback in duktape. The current implementation needs to keep a map of OvmsCommand => DuktapeConsoleCommand in order to do this (which is messy).

I think the map wouldn't be necessary for the command invocation, but it would still be necessary to check/get the associated DuktapeConsoleCommand of a parent OvmsCommand pointer.

...

...

My API design would be like this:

OK

...

The forced unregistering limits registering commands to library plugins (which don't have an unload operation yet). I think this is an unnecessary limitation, I would like to be able to register a command by adding a registration call to "ovmsmain.js" or by executing a script. Of course that means commands registered that way need to be unregistered explicitly, but that's up to the script developers & users then. I don't think we should limit them here without good reason.

On a Duktape unload/reload operation, all JS objects get finalized anyway because of heap destruction, so deletion would happen automatically by the DuktapeConsoleCommand finalizer. If we add some module unload operation later on, modules may e.g. define a cleanup callback, or we can add the module filename to the DuktapeObject registry -- the latter option would also automatically apply to all DuktapeObject instances, eliminating the need for separate registries for other system bindings potentially to be added in the future.

Implicit OvmsCommand deletions can happen as the order of deletion is undefined. Inhibiting this should be a straight forward use of the reference count though: on registration of a sub command, simply Ref() all parent commands registered by Duktape as well. Unref() them after the sub command has been removed, the last Unref() on a parent then automatically deletes it (the first Ref() is done by the coupling).

OK. My only concern would be timing. Say there is a script that is run, registers and command, then exits. The command will be cleaned up in the next duktape memory clean. But before then, the command could be called. If everything is running in that one duktape thread, there should be no crash, I think, but it sounds scary to me.

I would keep the command registered until it's explicitly unregistered or deleted by a heap destruction. I also suggest keeping a Ref() on the DuktapeConsoleCommand until the command has been fully deregistered…

Hm. Deregistration can still occur concurrently to the OvmsCommand deletion… I think we haven't taken care of concurrent command deregistration at all, there is no lock/mutex in OvmsCommandApp. So currently any subsystem deregistering it's commands can crash the system if one of those commands is just about to be executed. Maybe we need to address this in OvmsCommandApp or …Map.

Regards, Michael

...

Overall, your suggested approach sounds ok.

Regards, Mark.

...

On 8 Sep 2020, at 4:13 PM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

for the synchronous callback execution, that's simply a variant of OvmsDuktape::DuktapeRequestCallback() using the already existing DuktapeDispatchWait() method along with passing the OvmsWriter* in dmsg.writer. Could be named "…ExecuteCallback" to reflect the synchronous nature.

There is no need to extend OvmsCommand. The DuktapeObject is meant to be used as a general binding of arbitrary system objects to Duktape, so the system objects don't need to be extended.

From my first check of your implementation, I would probably: Redefine the register API to expect an object with an "execute" (and later possibly also a "validate") callback, and bind the DuktapeCommandObject to that object. A "fail" callback could also make sense. Delegate the full OvmsCommand life cycle and execution handling to the DuktapeCommandObject. Remove the forced unregistering of commands on script file unloading. Inhibit implicit sub command deletions by parent deletions. My API design would be like this:

// Command registration: var cmd = OvmsCommand.Register({ parent: "test", name: "dukcommand", title: "Test/demonstrate Duktape command registration", usage: "Pass 1 - 3 arguments", min: 1, max: 3, execute: function(verbosity, argv) {}, fail: function(error) {} });

// Command deregistration: OvmsCommand.Unregister({ parent: "test", name: "dukcommand" }); // …or in case we still have the cmd object, simply: OvmsCommand.Unregister(cmd);

The OvmsCommand execution callback would be a bound method of the DuktapeCommandObject instance. The method passes the verbosity and arguments vector to ExecuteMethod() via the data pointer using a custom struct. CallMethod() then pushes these on the Duktape stack and calls the "execute" property.

The forced unregistering limits registering commands to library plugins (which don't have an unload operation yet). I think this is an unnecessary limitation, I would like to be able to register a command by adding a registration call to "ovmsmain.js" or by executing a script. Of course that means commands registered that way need to be unregistered explicitly, but that's up to the script developers & users then. I don't think we should limit them here without good reason.

On a Duktape unload/reload operation, all JS objects get finalized anyway because of heap destruction, so deletion would happen automatically by the DuktapeConsoleCommand finalizer. If we add some module unload operation later on, modules may e.g. define a cleanup callback, or we can add the module filename to the DuktapeObject registry -- the latter option would also automatically apply to all DuktapeObject instances, eliminating the need for separate registries for other system bindings potentially to be added in the future.

Implicit OvmsCommand deletions can happen as the order of deletion is undefined. Inhibiting this should be a straight forward use of the reference count though: on registration of a sub command, simply Ref() all parent commands registered by Duktape as well. Unref() them after the sub command has been removed, the last Unref() on a parent then automatically deletes it (the first Ref() is done by the coupling).

Regards, Michael

Am 08.09.20 um 09:06 schrieb Mark Webb-Johnson:

...

Michael,

Very clear, and very helpful. Only thing I would suggest would be to have a minimum example in the documentation. The bare minimum required for an implementation of an object.

Reading through what you write, it seems the correct approach is:

Extend OvmsCommand to have a virtual ExecuteCommand method (same parameters as ‘m_execute’ callback). Extend OvmsCommand::execute to check if m_execute is null, then call ‘ExecuteCommand’ instead. Perhaps do the same for m_validate in OvmsCommand. Then, the duktape implementation can be object (rather than callback function) based, as your DuktapeObject expects. The javascript would call a function to register a command, with a command object to be used for the callback. Need to extend the DuktapeObject system to support a synchronous command (presumably implemented with a passed mutex like we do in several other parts of the system).

Is that correct, and what you were expecting? Or any other suggestions?

Regards, Mark.

...

On 7 Sep 2020, at 2:55 AM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Mark,

...

Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works.

I admit my documentation has some shortcomings. I'll try to fill that gap first:

Concept #1: Reference counting

A DuktapeObject is meant to be a C++/OS extension of a Javascript object, for example to hold some system binding associated with the JS object.

The primary goal is to provide asynchronous operations on the system side, initiating JS callbacks when finishing/failing. For example a HTTP operation can be started by a script, passing some "done" callback. The system then starts the network operation asynchronously, the JS task can continue processing other scripts. Once the network operation is done, the DuktapeObject sends a request to the Duktape task to execute the "done" callback on itself.

So the DuktapeObject is normally shared by multiple contexts and parallel operations. Asynchronous operation also means the JS object or context may already be gone when the operation finishes. So the DuktapeObject needs to stay locked in memory independent of the JS context.

That's implemented by counting the active references to the DuktapeObject (methods Ref() / Unref()). The last Unref() automatically deletes the DuktapeObject instance.

For example: JS requests a network operation. The initial JS binding (see coupling) sets the reference count to 1. The DuktapeObject starts the network request, increasing the ref count to 2. If the JS context now dies (decreasing the reference count), the DuktapeObject will still remain valid until the network operation returns.

As the Ref/Unref operations need a (recursive) mutex, that's also part of the DuktapeObject and exposed by the API for other uses: Lock() / Unlock().

Concept #2: Coupling

Javascript does not have a destructor concept. JS objects get deleted by heap destruction or by the garbage collector when no reference to the JS object is left. In both cases, the actual object deletion is called "finalization", and a special finalizer method/callback can be installed on a JS object to be called just before the object gets deallocated. That is done by the DuktapeObject::Couple() method (implicitly called when constructed directly with a JS object reference).

There is no way to force finalization on a JS object. So a DuktapeObject cannot tell Duktape to delete it's coupled object, that means a DuktapeObject should normally not be deleted from outside the Duktape context, at least not if still coupled to the JS object. Coupling and decoupling can only be done in the Duktape context.

The standard finalizer DuktapeObject::Finalizer() simply decouples, automatically deleting itself if the coupling was the last reference. This is a virtual method, so can be overridden as necessary.

The coupling operation additionally adds a hidden pointer to the DuktapeObject instance in the JS object. That allows to check for and retreive associated DuktapeObject instances from any JS object, which is provided by the GetInstance() call.

Concept #3: Registration

For asynchronous operations, it's normally very convenient to have a "fire & forget" API. Example from the documentation: VFS.Save({ path: "/sd/mydata/telemetry.json", data: Duktape.enc('jx', telemetry), fail: function(error) { print("Error saving telemetry: " + error); } }); I.e. you simply pass the operation arguments including the done/fail callbacks to an API method and don't need to care about storing a reference to some handle. In JS that normally means the object used won't have any reference left after the call, so would be deleted by the garbage collector on the next run.

To avoid garbage collection and lock the JS object in memory, we need to store a reference to it in a "public" place. Duktape provides a special public place for this, hidden from scripts, called the global stash. DuktapeObject maintains a dedicated global object registry in that stash.

Adding and removing the coupled object reference to/from that registry is done by the Register() and Deregister() methods.

So for asynchronous system operations, or system integrations that shall be persistent, you normally do a Register() call together with the coupling, unless some ressource isn't available. Deregistration is then normally done when all pending JS callbacks have been executed, or when the persistent system integration has been unbound.

Other API designs are possible here: if you'd rather like the script needing to store a reference to your operation handle, you don't need to do a registration. The object will then be deleted (finalized) by the garbage collector automatically after the script deletes the reference.

Concept #4: Callback invocation

Triggers on the system side, for example a finished or failed network operation, shall normally trigger a JS method execution.

JS callback methods are simply passed as part of the arguments object in modern JS APIs. This allows to pass simple function definitions inline, as well as to reference a separately defined general handler function. JS allows functions to be excuted in the context of any object, and callbacks normally are executed in the context of the API object. This adds even more convenience, as the callbacks can easily access the other API arguments still stored in the object, as well as additional data added by the call.

JS callbacks cannot be executed directly from any system context, they need to run in the Duktape context. So the DuktapeObject callback invocation mechanism includes a general method to request a callback execution by Duktape: RequestCallback()

Note: RequestCallback() is an asynchronous operation. A synchronous variant can be added if necessary (and probably will be for command execution from a console).

A pending callback automatically increments the reference count, so the object is locked in memory until the callback has been executed (or aborted) by the Duktape task.

The callback invocation API provides a void* for simple data (e.g. a fixed string) to be passed to the callback method, but for more complex data, you will normally fill some DuktapeObject member variables before invoking the callback.

In Duktape context, the callback invocation translates the data returned or provided by the system side into the Duktape callback arguments and then runs the callback (if the object actually has the requested callback set). The default implementation for this is DuktapeObject::CallMethod(), which can be used directly for simple callbacks without arguments. For more complex handling, override this with your custom implementation.

The callbacks are by default executed on the coupled JS object, so data can also be transported by setting properties on that object. The callback can then simply access them via "this".

To simplify callback invocation from code parts that may run outside or inside Duktape, it's convenient to allow calling CallMethod() without a Duktape context, and let CallMethod() translate that into a RequestCallback() call as necessary. Pattern:

duk_ret_t DuktapeHTTPRequest::CallMethod(duk_context *ctx, const char* method, void* data /*=NULL*/) { if (!ctx) { RequestCallback(method, data); return 0; } …

A CallMethod() implementation isn't limited to executing a single callback. A common example is an API defining "done" & "fail" callbacks, as well as a general final "always" callback. DuktapeHTTPRequest::CallMethod() also serves as an example implementation for this.

Wow… that's become more to write & read than I expected. Please provide some feedback: is that explanation sufficient & clear? I'll refine it for the developer docs then.

Regards, Michael

Am 01.09.20 um 19:52 schrieb Michael Balzer:

...

Mark,

I'll have a look.

Regards, Michael

Am 01.09.20 um 07:30 schrieb Mark Webb-Johnson: > Michael, > > Still struggling with this. It seems like your DuktapeObject will do this, but I can’t work out how it works. > > Here are some notes one what I have done so far: > > Created a stub DuktapeConsoleCommand (derived from DuktapeObject) in ovms_duktape.{h,cpp}. This should hold enough to be able to call the javascript callback method for that object. It also stores the module filename (so the registration can be removed when the module is unloaded). > > Provide a DuktapeCommandMap m_cmdmap in ovms_duktape.{h,cpp} OvmsDuktape class that stores a mapping from OvmsCommand to DuktapeConsoleCommand. > > Created a OvmsDuktape::RegisterDuktapeConsoleCommand in ovms_duktape.{h,cpp) that (a) creates the OvmsCommand() object, (b) registers it, (c) creates the DuktapeConsoleCommand() object, and (d) updates a map from OvmsCommand->DuktapeConsoleCommand. There Is also a single callback DukOvmsCommandRegisterRun designed to be run by all. > > Created hooks NotifyDuktapeModuleLoad, NotifyDuktapeModuleUnload, and NotifyDuktapeModuleUnloadAll in OvmsDuktape. The javascript module is identified by filename (path to module or script on vfs, usually, but may also be an internal module). The Unload functions look through the m_cmdmap and unregister commands for javascript modules being unloaded. > > Provide an implementation for ovms_command DukOvmsCommandRegister to support registering commands from Javascript modules. This should extract the details, and then call OvmsDuktape::RegisterDuktapeConsoleCommand to do the actual registration. This has been implemented, except for the callback method (and somehow passing that method from Javascript in the OvmsCommand.Register javascript call). > > Provide a stub implementation for DukOvmsCommandRegisterRun. This uses m_cmdmap to lookup the DuktapeConsoleCommand object for the command to be run. It should execute the callback method (but that part is not yet implemented). > > > I still need help with #5 and #6. What needs to be implemented in DuktapeConsoleCommand, and how is the parameter in OvmsCommand.Register used to store the callback (#5)? Then how to callback the command method from DukOvmsCommandRegisterRun (#6)? If you have time, it is probably much quicker for you to simply make those changes. > > An alternative implementation would be to do something like the pubsub framework, where the mapping command->callback is done from within a javascript module. That I could do, but it seems your DuktapeObject can do it better. > > Thanks, Mark. > >> On 15 Jul 2020, at 3:34 PM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote: >> >> Mark, >> >> yes, I needed that persistence for the HTTP and VFS classes, but I also needed to be able to couple a dynamic C++ instance with a JS object and have a mechanism to prevent garbage collection while the C++ side is still in use. If the C++ side is no longer needed, the JS finalizer also needs to imply the C++ instance can be deleted. >> >> That is all implemented by DuktapeObject. DuktapeObject also provides JS method invocation on the coupled JS object and a mutex for concurrency protection. >> >> We probably need some more framework documentation than the header comments (applies to all of our framework classes…): >> >> /*************************************************************************************************** >> * DuktapeObject: coupled C++ / JS object >> * >> * Intended for API methods to attach internal API state to a JS object and provide >> * a standard callback invocation interface for JS objects in local scopes. >> * >> * - Override CallMethod() to implement specific method calls >> * - Override Finalize() for specific destruction in JS context (garbage collection) >> * - call Register() to prevent normal garbage collection (but not heap destruction) >> * - call Ref() to protect against deletion (reference count) >> * - call Lock() to protect concurrent access (recursive mutex) >> * >> * - GetInstance() retrieves the DuktapeObject associated with a JS object if any >> * - Push() pushes the JS object onto the Duktape stack >> * >> * Note: the DuktapeObject may persist after the JS object has been finalized, e.g. >> * if some callbacks are pending after the Duktape heap has been destroyed. >> * Use IsCoupled() to check if the JS object is still available. >> * >> * Ref/Unref: >> * Normal life cycle is from construction to finalization. Pending callbacks extend >> * the life until the last callback has been processed. A subclass may extend the life >> * by calling Ref(), which increases the reference count. Unref() deletes the instance >> * if no references are left. >> */ >> >> You normally just need to use Register/Deregister & Ref/Unref, and to implement the constructor and CallMethod. Coupling of the instances normally is done on construction, as a JS object is normally already needed for the parameters and can simply be attached to. >> >> Have a look at DuktapeHTTPRequest, DuktapeVFSLoad and DuktapeVFSSave, these are the current subclasses using this. >> >> For the command registration I would probably couple the OvmsCommand instance with a JS command object providing an execution method. >> >> Tell me if you need more info. >> >> Regards, >> Michael >> >> >> Am 15.07.20 um 08:12 schrieb Mark Webb-Johnson: >>> >>> @Michael this is probably for you. >>> >>> I am trying to implement javascript command registration. The idea is that a javascript module can call something like: >>> >>> OvmsCommand.Register(basecommand, name, title, callbackfn, usage, min, max) >>> >>> Then we reflect that into MyCommandApp.RegisterCommand, and keep a track of which command is for which javascript callbackfn. When the command is executed, we pass it into duktape. >>> >>> I also have tracking for javascript module loading and unloading, so I can DeregisterCommand() if duktape is reloaded (and also protected against commands being registered in short-lived scripts run from the command line). >>> >>> To implement this, I need to store the callbackfn as a persistent reference to a duktape javascript function. >>> >>> The issue with callback function references in duktape is summarised here: >>> >>> https://wiki.duktape.org/howtonativepersistentreferences <https://wiki.duktape.org/howtonativepersistentreferences> >>> >>> When a Duktape/C function is called, Duktape places the call arguments on the value stack. While the arguments are on the value stack, they're guaranteed to be reachable and the Duktape/C function can safely work with the arguments. >>> >>> However, when the Duktape/C function returns, the value stack is unwound and references in the function's value stack frame are lost. If the last reference to a particular value was in the function's value stack frame, the value will be garbage collected when the function return is processed. >>> >>> The standard approach is to store the reference back in the duktape duk_push_global_stash so it won’t get garbage-collected. But, that seems messy. >>> >>> I see that Michael has already implemented something that seems similar in ovms_script.{h, cpp}, for the async http callbacks. Presumably to avoid this issue. But, the approach seems very different, and I am not sure if it is stopping _all_ garbage collection for the duration of the async query, or just that particular object being garbage collected. The work seems extensive (quite a few objects involved). >>> >>> So @Michael, any suggestions for this? I don’t want to reinvent the wheel... >>> >>> Regards, Mark. >>> >>> >>> _______________________________________________ >>> OvmsDev mailing list >>> OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> >>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev> >> >> -- >> Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal >> Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 >> _______________________________________________ >> OvmsDev mailing list >> OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> >> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev> > > > > _______________________________________________ > OvmsDev mailing list > OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> > http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev