raffaello-camoriano / RBDemo

Reactive Behaviors Demo
1 stars 0 forks source link

Reddite quae sunt Caesaris Caesari et quae sunt Dei Deo #16

Open alecive opened 10 years ago

alecive commented 10 years ago

(The title is meant to keep the level of the issues high)

Right now, you're having a module and a thread inside it, and that's ok. What I don't understand is why there are some variables that are arguably declared and instantiated inside the module, and passed as reference in the thread. This despite, at least apparently, they're used only by the thread. Why don't you put everything inside the thread? The code will be significantly better to read and to handle.

GiuliaP commented 10 years ago

I followed closely this document: http://wiki.icub.org/wiki/Summary_of_iCub_Software_Development_Guidelines but I agree with you. What about replacing the thread class (which actually isn't necessary because in this way I use two threads to do a very simple job) with another class (with the following methods: open(), init(), run(), close()) that plays the role of the thread? In this way I keep the module separated from the code, without using another thread.

alecive commented 10 years ago

As I said before, for a couple of reasons it's not that bad to have a module and a thread inside it, even if the module is doing nothing. But, as you said, for simple modules this structure is overly complex. My suggestion was to choose either one solution (i.e. a module and everything inside its updateModule() method), or the other (i.e. what you're using now). I wouldn't go for something in the middle: if you want create a class, why not a thread in itself? :)

pattacini commented 10 years ago

To recap Bear in mind that RFModule is a masked RateThread that does something more: ctrl+c catching and reading of configuration switches and data. If your algorithm is simple and fits this structure (i.e. just one periodic thread) don't instantiate further threads; otherwise you're forced to do so.

GiuliaP commented 10 years ago

Thank you! Then I finally chose to avoid instantiating another thread for my module. To keep the RFModule separated from the code, as I have been told that this is a best practice in general, I created a simple class that contains the variables needed for the computation, the ports and the interfaces, together with some methods that are called by the correspondent ones of the RFModule (open(), loop(), interrupt() and close()). However, if you believe that, in this simple case, all the code could be put directly inside the RFModule, I can put all variables inside it and paste the code of the mentioned methods inside the RFModule methods.