kicad-developers team mailing list archive

[miles mccoo <mail@xxxxxxxxxx>]

Thanks all for your replies.

I like the plugin mechanism. It does some nice things for python folks.
Refresh, undo, garbage collection (I think). I want to see it succeed.

>From reading Orson's mail, I think I wasn't entirely clear, so let me try
to fix that first.


The plugin mechanism tries to track the changes of a python plugin. In the
c++ world, developers are expected to inform BOARD_COMMIT that something
has changed. The plugin infrastructure does this for you. One could compare
it to garbage collection. My interpretation of how this is done is simple:
before a plugin runs, make a list of all design objects. zones, tracks,
modules, nets... after the plugin completes, do a "diff".

This is mostly fine with the exception of removed items.

The problem with removed items is how do you do a diff on a deleted, memory
managed, cleaned up object? Perhaps that memory now contains a different
kind of object. Perhaps it's been reallocated to a similar object as before.



*as it stands now, any plugin that removes items from a board item
container will likely fail*. It's not quite a crash, but it has a similar
feel to the user.



Solutions. I can think of four.

solution 1. Why is the plugin mechanism in the business of tracking
changes? Shouldn't BOARD_COMMIT just magically happen whenever a c++
instance is modified. I brought this up in a previous thread[1] and various
reasons were given why this isn't desirable.

solution 2. Same as #1 except BC magic only happens on python API calls.
The plugin mechanism would no longer do diffs. Just add BC checkpoints.
This is probably a lot of work. I might be willing to do this work but it
would take time. [2]

solution 3. easy to implement. turn off deletion on calls to remove if
currently running a plugin

   - plugin gets a boolean: isPluginActive. set/unset around the call to
   actual python run code.
   - add pcbnew.isPluginActive() to python api. (I could imagine this could
   have other uses)
   - the remove code looks at isPluginActive to decide whether to set
   isown. (that code is actually python, not swig)

I have #3 implemented and my plugins no longer crash. *See attached patch
for if folks agree it is an acceptable stopgap.*

solution 4. probably not the direction I would go but a way to enable
python memory management and do the plugin diff thing. Basically, give each
object some sort of unique id. (could be useful for other things). In
addition to holding a list of object pointers, plugin infrastructure would
hold a list of object ids.


*Given the ease with which a plugin can hit this issue and given how long
it would take to get #2 right, I guess I'm recommending the changes of that
attached patch for #3.*

Miles

[1] https://lists.launchpad.net/kicad-developers/msg32063.html
[2] I think my personal opinion is that #1 is superior over #2. Python
people shouldn't have to care but why is it that c++ should or want to?
Yes, I read the arguments from the previous thread but I'm not convinced.
I'm also just a kicad spectator, so there's a lot I don't know.





On Fri, Mar 2, 2018 at 2:47 PM, Wayne Stambaugh <stambaughw@xxxxxxxxx>
wrote:

> LOL, I just replied to Miles.  Thanks Orson for helping out!
>
> On 3/2/2018 8:36 AM, Maciej Sumiński wrote:
> > Hi Miles,
> >
> > I suppose the silence in the thread indicates there are not many
> > developers knowing the Python scripting interface inside out. Since you
> > are both studying the scripting interface and developing own scripts, it
> > is quite possible you are the most competent person to give us an advice
> > on how to proceed. See some comments below, but I am neither a Python
> > script developer nor a scripting interface maintainer, so I might be
> > lacking some basic knowledge here.
> >
> > On 02/28/2018 05:12 PM, miles mccoo wrote:
> >> So I'm plugin-ifying my python scripts (the mechanism is awesome). One
> of
> >> the plugins deletes some stuff and that is causing trouble.
> >>
> >>
> >>
> >> I'm not sure how to fix the root cause. Hence this mail.
> >>
> >>
> >>
> >> The plugin just deletes Edge.Cuts[1]:
> >> for d in board.GetDrawings():
> >>             if (d.GetLayerName() == 'Edge.Cuts'):
> >>                 board.Remove(d)
> >>
> >>
> >>
> >> in board_item_container.i, I see this (with stuff deleted):
> >> %rename(RemoveNative)    BOARD_ITEM_CONTAINER::Remove;
> >>     def Remove(self,item):
> >>         self.RemoveNative(item)
> >>         item.thisown=1
> >>
> >>
> >> Setting thisown tells, python "you own it". Delete it when you're done.
> >> Which it does.
> >>
> >>
> >> The problem this causes is that the plugin mechanism saves a list of all
> >> objects before running the plugin and then checks if any of them has a
> null
> >> list after (ie is it still in the design).
> >
> > Is this mechanism implemented to handle memory leaks? If so, would not
> > it be sufficient to stick to 'thisown' flag on Remove() calls or is
> > there another way objects might be destroyed using Python scripts?
> >
> >> Since the object has been deleted by python, the plugin stuff gets
> confused.
> >>
> >>
> >> *So, the question is how to fix this?*
> >>
> >>
> >> It appears that the plugin infrastructure will delete for you (that's
> what
> >> I'm guessing), so the thisown setting shouldn't be done.
> >>
> >>
> >> On the other hand, if running code from within a standalone script (ie
> from
> >> regular python shell), now thisown'ing it would yield a memory leak.
> >>
> >>
> >>
> >> Perhaps the plugin stuff should have some sort of flag indicating
> "you're
> >> in a plugin". Then the thisown setting could be conditional.
> >
> > If the object listing mechanism is required for other reasons, then I
> > suppose it is second best idea. Generally speaking, I would like to make
> > the scripting interface convenient for the users, so they do not need to
> > worry about whether their scripts are run standalone or as a plugin.
> > Let's hide the dirty magic from them and make the coding process
> enjoyable.
> >
> > Regards,
> > Orson
> >
> >> But I'm just a spectator. *I'm happy to put in the time to fix this but
> >> need guidance on what approach to take.*
> >>
> >>
> >> Miles
> >>
> >>
> >>
> >> [1] full plugin text
> >> import pcbnew
> >>
> >> class RemoveBoundaryPlugin(pcbnew.ActionPlugin):
> >>     def defaults(self):
> >>         self.name = "Remove boundary"
> >>         self.category = "A descriptive category name"
> >>         self.description = "This plugin reads a dxf file and converts
> it to
> >> a zone"
> >>
> >>     def Run(self):
> >>         board = pcbnew.GetBoard()
> >>
> >>         for d in board.GetDrawings():
> >>             print("{}".format(str(d)))
> >>             #print("on layer {} {} {}".format(d.GetLayerName(),
> >>             #                                 str(d.GetStart()),
> >>             #                                 str(d.GetEnd())))
> >>             if (d.GetLayerName() == 'Edge.Cuts'):
> >>                 board.Remove(d)
> >>
> >> RemoveBoundaryPlugin().register()
> >>
> >>
> >>
> >> _______________________________________________
> >> 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
> >>
> >
> >
> >
> >
> > _______________________________________________
> > 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
> >
>
>
> _______________________________________________
> 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
>

From 28033cac3bb46bcaf748662b5e287d2513895150 Mon Sep 17 00:00:00 2001
From: Miles McCoo <mail@xxxxxxxxxx>
Date: Tue, 6 Mar 2018 09:28:57 +0100
Subject: [PATCH] Fix for crash due to pcbnew_action_plugin object tracking
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.7.4"

This is a multi-part message in MIME format.
--------------2.7.4
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


pcbnew_action_plug tracks items that were modified within a plugin. In the case of
deletion, the old object is no longer valid. This commit turns off the call to delete
if a plugin is active.
---
 pcbnew/action_plugin.cpp                 | 15 +++++++++++++++
 pcbnew/action_plugin.h                   | 16 +++++++++++++++-
 pcbnew/swig/board_item_container.i       |  5 +++--
 pcbnew/swig/pcbnew_action_plugins.cpp    |  2 ++
 pcbnew/swig/pcbnew_scripting_helpers.cpp |  7 +++++++
 pcbnew/swig/pcbnew_scripting_helpers.h   |  5 +++++
 6 files changed, 47 insertions(+), 3 deletions(-)


--------------2.7.4
Content-Type: text/x-patch; name="0001-Fix-for-crash-due-to-pcbnew_action_plugin-object-tra.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-Fix-for-crash-due-to-pcbnew_action_plugin-object-tra.patch"

diff --git a/pcbnew/action_plugin.cpp b/pcbnew/action_plugin.cpp
index 7996745..daeeb6e 100644
--- a/pcbnew/action_plugin.cpp
+++ b/pcbnew/action_plugin.cpp
@@ -44,6 +44,9 @@ void ACTION_PLUGIN::register_action()
 std::vector<ACTION_PLUGIN*> ACTION_PLUGINS::m_actionsList;
 
 
+bool ACTION_PLUGINS::m_actionRunning = false;
+
+
 ACTION_PLUGIN* ACTION_PLUGINS::GetAction( int aIndex )
 {
     return m_actionsList[aIndex];
@@ -147,3 +150,15 @@ bool ACTION_PLUGINS::deregister_object( void* aObject )
 
     return false;
 }
+
+
+bool ACTION_PLUGINS::IsActionRunning()
+{
+    return ACTION_PLUGINS::m_actionRunning;
+}
+
+
+void ACTION_PLUGINS::SetActionRunning(bool running)
+{
+    ACTION_PLUGINS::m_actionRunning = running;
+}
diff --git a/pcbnew/action_plugin.h b/pcbnew/action_plugin.h
index 9552e1a..fb315aa 100644
--- a/pcbnew/action_plugin.h
+++ b/pcbnew/action_plugin.h
@@ -101,7 +101,7 @@ private:
      * ACTION_PLUGIN system wide static list
      */
     static std::vector<ACTION_PLUGIN*> m_actionsList;
-
+    static bool m_actionRunning;
 public:
     /**
      * Function register_action
@@ -168,6 +168,20 @@ public:
      * @return the number of actions available into the system
      */
     static int GetActionsCount();
+
+
+    /**
+     * Function IsActionRunning
+     * @return Is an action running right now
+     */
+    static bool IsActionRunning();
+
+    
+    /**
+     * Function SetActionRunning
+     * @return Set whether an action is running now.
+     */
+    static void SetActionRunning(bool running);
 };
 
 #endif /* PCBNEW_ACTION_PLUGINS_H */
diff --git a/pcbnew/swig/board_item_container.i b/pcbnew/swig/board_item_container.i
index aadc069..cabac64 100644
--- a/pcbnew/swig/board_item_container.i
+++ b/pcbnew/swig/board_item_container.i
@@ -32,8 +32,9 @@
         Remove(self, BOARD_ITEM)
         """
         self.RemoveNative(item)
-        item.thisown=1
-
+        if (not IsActionRunning()):
+            item.thisown=1
+        
     def Delete(self,item):
         """
         Remove a BOARD_ITEM from this BOARD_ITEM_CONTAINER, set the thisdown flag so that
diff --git a/pcbnew/swig/pcbnew_action_plugins.cpp b/pcbnew/swig/pcbnew_action_plugins.cpp
index ef18cde..316efc6 100644
--- a/pcbnew/swig/pcbnew_action_plugins.cpp
+++ b/pcbnew/swig/pcbnew_action_plugins.cpp
@@ -230,7 +230,9 @@ void PCB_EDIT_FRAME::OnActionPlugin( wxCommandEvent& aEvent )
         itemsList.ClearItemsList();
 
         // Execute plugin himself...
+        ACTION_PLUGINS::SetActionRunning(true);
         actionPlugin->Run();
+        ACTION_PLUGINS::SetActionRunning(false);
 
         currentPcb->m_Status_Pcb = 0;
 
diff --git a/pcbnew/swig/pcbnew_scripting_helpers.cpp b/pcbnew/swig/pcbnew_scripting_helpers.cpp
index 3dbedc3..065c029 100644
--- a/pcbnew/swig/pcbnew_scripting_helpers.cpp
+++ b/pcbnew/swig/pcbnew_scripting_helpers.cpp
@@ -40,6 +40,7 @@
 #include <macros.h>
 #include <stdlib.h>
 #include <pcb_draw_panel_gal.h>
+#include <action_plugin.h>
 
 static PCB_EDIT_FRAME* s_PcbEditFrame = NULL;
 
@@ -135,3 +136,9 @@ void UpdateUserInterface()
     if( s_PcbEditFrame )
         s_PcbEditFrame->UpdateUserInterface();
 }
+
+
+bool IsActionRunning()
+{
+    return ACTION_PLUGINS::IsActionRunning();
+}
diff --git a/pcbnew/swig/pcbnew_scripting_helpers.h b/pcbnew/swig/pcbnew_scripting_helpers.h
index e0a0ae9..6233b3a 100644
--- a/pcbnew/swig/pcbnew_scripting_helpers.h
+++ b/pcbnew/swig/pcbnew_scripting_helpers.h
@@ -69,4 +69,9 @@ void    WindowZoom( int xl, int yl, int width, int height );
  */
 void UpdateUserInterface();
 
+/**
+ * Are we currently in an action plugin?
+ */
+bool IsActionRunning();
+
 #endif      // __PCBNEW_SCRIPTING_HELPERS_H

--------------2.7.4--