search

PixInsight / PCL

PixInsight Class Library
http://pixinsight.com/developer/pcl/
Other
42 stars 21 forks source link

Some fixes for better javascript automation support #24

Closed kkretzschmar closed 8 years ago

kkretzschmar commented 8 years ago

Hi Juan, here is a fix to support connecting to INDI server and devices from javascript.

Best, Klaus

jconejero commented 8 years ago

Hi Klaus,

I haven't merged this pull request because I'm not sure it fits well with the changes I've made to implement interactive INDICCDFrame functionality (Start and Stop buttons in INDICCDFrameInterface).

I'm going to implement your changes to INDIDeviceControllerInstance (missing public inheritance attribute and the new console message in INDIDeviceControllerInstance.cpp), but, are you really sure you want to make INDIClient data members public (m_instance and m_scriptInstance)?

kkretzschmar commented 8 years ago

Hi Juan,

I haven't merged this pull request because I'm not sure it fits well with the changes I've >made to implement interactive INDICCDFrame functionality (Start and Stop buttons in >INDICCDFrameInterface).

Yes, I was aware of that merge conflicts could happen. I had to change the code since the INDICCDFrame instance needs access to soome methods of the INDIDeviceController instance when TheINDIDeviceControllerInterface == nullptr, i.e. when no GUI has been started, which is the case when creating a DeviceControllerInstance from javascript.

are you really sure you want to make INDIClient data members public (m_instance and >m_scriptInstance)

No, you are right, this is a hack, which I forgot to fix. There is already a interface, and I'll enhance the interface.

I'll try to find time on weekend to fix the merge conflicts and enhance the interface.

Thanks!

2016-04-15 17:58 GMT+02:00 Juan Conejero notifications@github.com:

Hi Klaus,

I haven't merged this pull request because I'm not sure it fits well with the changes I've made to implement interactive INDICCDFrame functionality (Start and Stop buttons in INDICCDFrameInterface).

I'm going to implement your changes to INDIDeviceControllerInstance (missing public inheritance attribute and the new console message in INDIDeviceControllerInstance.cpp), but, are you really sure you want to make INDIClient data members public (m_instance and m_scriptInstance)?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/PixInsight/PCL/pull/24#issuecomment-210520139

kkretzschmar commented 8 years ago

Hi Juan, 1.) I solved the merge conflicts and made the m_instance members private again ;)...

2.) In addition I solved some problems with the server connection in interactive mode (pressing disconnect button several times in sequence).

3.) I also tested javascript execution with and without "interactive" DeviceController instances in parallel. Here seems to be a little problem with the current architecture. There is only one global INDIClient instance (the autopointer) but there can be several INDICCFrame process or INDIDeviceController process instances. The INDICCDFrame process instances hold a reference to the global INDIClient instances which is fine. However all INDIDeviceController process instances have full control over the INDIClient instance which is not good. E.g. while a INDIDeviceController process instance is running a script instance can disconnect from the INDI server without notifying the process instance. This leads to inconsistencies ...

I see two possible solutions here:

i) Disallow a INDIDeviceController javascript instance to connect/disconnect from INDI server ii) Add for each process/script instance there a different corresponding INDIClient instance. So we need not only one INDIClient instance but several.

I think we should implement i) temporarily for the prerelease and then implement ii) later.

What do you think?

By the way did you receive my PJSR pull request? I am not sure whether I configured my git correctly.

kkretzschmar commented 8 years ago

Hi Juan, forgot to mention, that I checked your new event based AbstractINDICCDFrameExecution class. Yes its great. I'll use a similar class for INDIMount and the INDIDeviceController class could also benefit from it.

Best, Klaus

jconejero commented 8 years ago

Hi Klaus,

Glad to know you like my event-driven implementation. This is a general abstraction technique that I also use in the PixInsight Core application with good results.

As you'll see, I have committed more changes. I'm going to comment on them in a separate thread for the corresponding commit (so we can have discussion topics better organized).