kicad-developers team mailing list archive

[Jean-Samuel Reynaud <js.reynaud@xxxxxxxxx>]

Hi Dick, Hi list

I'm currently modify the plugin to met your points.

=> now the plugin don't load python module each time
=> the lib path is now handled directly onto the python module and is
not used anymore as the python module path.

=> I had created a python sub-interpreter (associated into the adapter)
This sub interpreter is a property of the adapter. So there are one
interpreter per adapter instance.

So on the contructor I have:
...
    m_pythread = Py_NewInterpreter();
...

then when I use it I do
....
    PyEval_AcquireLock();
    PyThreadState_Swap(m_pythread);
// speak with python/module...
     PyThreadState_Swap(g_PythonMainTState);
     PyEval_ReleaseLock();

Do you think it is ok ?

I understand that this is the opposite way of your idea (python at top
and C++ at the bottom) but I don't know if it's possible (in current
state) to do this by an other way.

Regards

Le 09/03/2014 20:22, Dick Hollenbeck a écrit :
> On 03/09/2014 01:59 PM, Dick Hollenbeck wrote:
>> Hi Jean-Samuel,
>>
>> I am assuming that you would like this work to be committed as part kicad source?  If not
>> please let me know.  With my assumption in place, we'll have to achieve a minimum amount
>> of general usability, and that may extend beyond what you need it for.
>>
>> Please be aware of this.
>>
>>
>>> Hi,
>>>
>>> For starting I had implement your b) option. For that I start from a copy of guthub
>>> plugin. So I add options in all
>>> needed files (CMakeLists.txt..), I had create a directory on pcbnew and put the code
>>> inside and an example.
>>>
>>> Out of this directory I had done some small changes:
>>> scripting/python_scripting.h
>>> => Adding global value g_PythonMainTState as extern (needed for python locks)
>>> pcbnew/plugin.cpp
>>> => move python_footprint_plugin option to my plugin (as comment suggest). I hope it's ok.
>>> pcbnew/io_mgr.h
>>> pcbnew/io_mgr.cpp
>>> => Declare my plugin on lists
>>>
>>> On the new directory:
>>>
>>> pcbnew/fppython/fppython_example.py
>>> A plugin example.
>>> It's a really basic:
>>>  - 2 statics foot prints
>>>  - footprint himself are hardcoded on the example
>>>  - fake save/delete
>>>
>>> pcbnew/fppython/fppython_plugin.(cpp|h)
>>> The plugin himself
>>>
>>> To use is:
>>> - add a new footprint lib entry,
>>> - specify the directory where the plugin is as library path
>>
>> The Footprint*() library path argument should always only be used as a pointer to the
>> library, not as a pointer to the plugin.
>>
>>
>> A normal python environment variable should know where a python module is.  That should be
>> the fallback in any case, with possible augmentation as described below, which is only an
>> *optional* override for module loading.
>>
>> Additionally, you can look for an optional fp_lib_table property, one spelled identically
>> to a standard python environment variable name, which if present, can trump the
>> environment variable.
>>
>>
>>
>>> - specify python_footprint_plugin option as the plugin name (without .py, for example
>>> fppython_example)
>> good.
>>
>>
>>> Some limitations I found inside the core to allow more feature for plugins:
>>> - IsFootprintLibWritable don't have a PROPERTY parameter. So since the plugin module name
>>> is inside this, I'm unable to load
>>> the right python module to let it handle this method
>> Your python module loading should happen less frequently.
>>
>> 1) The the association between the C++ adapter instance and the python module can and
>> should be retentative up until
>> it cannot be.
>>
>> 2) The association between the module and the library can and should be retentative up
>> until it cannot be.
>>
>>
>> The easiest way to manage this is to have two member helpers in your adaptor:
>>
>> bool PYTHON_ADAPTOR::changedLibPath( const wxString& aCurrentLibPath )
>> {
>>     if( aCurrentLibPath != m_lib_path )
>>     {
>> 	m_lib_path = aCurrentLibPath;
>> 	return true;
>>     }
>>     return false;
>> }
>>
>> PLUS, a similar function for python module name.
>>
>> Then only change libraries, when changedLibPath() returns true.  Only change modules when
>> changedModule() returns true.
>>
>> This way your module and library can both be cached in the PYTHON_ADAPTER.  User's will
>> instantiate a new PYTHON_ADAPTER for any number of libraries.
>>
>> But 99% of the time, your python interpreter (or sub-interpreter) instantiation should
>> survive multiple calls to the adaptor, and the module should be left loaded, and the
>> library should be left open.
>>
>> We can add PROPERTIES argument to IsFootprintLibWritable() but if you follow my design
>> suggestions, you can survive without this additional argument for now.  You will have a
>> record of the m_module's last value already.
>>
>>
>>> - FootprintLibOptions: Same issue. So the plugin can't provide a list of parameters needed
>>> for his own usage. 
>> In the C++ why don't you refer people to the pydocs for the module.  I don't want to have
>> to load the python module in the OptionsEditor, where the plugin may instantiated just to
>> get the supported parameters.  Let's refer people to external documentation in the help
>> text for the certainly present python_footprint_plugin option.
>>
>> That way each python module can later query for the options it might expect.  The user can
>> supply additional options, even though they are not initially part of the supplied
>> choices, in the options editor.
>>
>>
>>
>>> To work arround this
>>> I hard code 4 parameters (fppython_parameter[1-4]) but it is not beautiful...
>> Those could be removed, if the user can add his own in the OptionEditor with more
>> meaningful names.
>>
>> I think we have to prove viability of this entire python plugin module first before we tip
>> the boat over.
>>
>>
>> Also, remember that its going to be impossible to have more than one running python
>> interpreter in a single process.  Python simply does not support that.  It does however
>> support "sub-interpreters".  As the top level Kicad GUI transforms into wxPython, you will
>> not be able to instantiate a brand new python interpreter in your PYTHON_ADAPTOR, for two
>> reasons:
>>
>> a) only one interpreter per process, and the main one is way above you.
>>
>> b) you should not assume that the user only has one PYTHON_ADAPTOR in the fp_lib_table,
>> and you are limited to only one interpreter per process.
>>
>> So if you are not already an expert on python "sub-interpreters", you need to make that
>> your highest priority, because your work will never get committed if you cannot conform to
>> the overall design constraints of the single python interpreter constraint.
>
> Jean-Samuel,
>
> Another thing to be aware of is that currently the FootprintLoad() function has to be
> thread safe.  This may not have been the case when I planted the seed for this idea you had.
>
> But FootprintLoad() is currently thread safe for all implementing PLUGINs.  For any
> instance of the PYTHON_ADAPTOR, you can be sure that only a single thread will call on
> each PYTHON_ADAPTOR instance at a time.
>
> The only place that I am aware of that causes this is
>
> bool FOOTPRINT_LIST::ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname )
>
> near line 185 in common/footprint_info.cpp.  This code is called when the user presses the
> "Load All" button in the footprint picker.
>
> Its possible that a static MUTLOCK will give you the protection you need in the
> PYTHON_ADAPTOR.  But this is getting hairy now, and I'm not going to have the time to help
> you make python multithreaded.  The MUTLOCK may be a way out, but I don't have the brain
> cycles to worry about something as obscure as this.
>
> My vision has python on the top, and C++ underneath.  Your's is upside down from that.
>
>
> Dick
>
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp