Mathieu's Object::Stop() patch, cleaned up and documented

Mathieu's Object::Stop() patch, cleaned up and documented

[Vedran Miletić, 2012-09-20 11:08:21]

As said on mailing list, Reset() method is missing, but that's trivial.

http://codereview.appspot.com/6530051/diff/1/src/core/model/object.cc
File src/core/model/object.cc (right):

http://codereview.appspot.com/6530051/diff/1/src/core/model/object.cc#newcode282
src/core/model/object.cc:282: Iterate (&ReallyDoStart, true);
Putting "true" here makes Start() method go over all Ptr<> attributes, which can
be a problem. For example, there can be an object having a pointer to other
object which is not a "sub-Object" and should be started and stopped
independently.

I see two possible solutions:
1) We should put this to false by default, and make it an opt-in kind of option
where model authors can override this method in their class, or
2) We should somehow label "sub-Objects" which should be started or stopped with
main object, and Iterate() method should ignore the rest.

[Tom Henderson, 2012-10-15 13:28:26]

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h
File src/core/model/object.h (right):

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h#newco...
src/core/model/object.h:159: * an object, unlike DoDispose.
This method starts the Object and all of its aggregates and attributes of type
PointerValue, and sets a state variable to indicate that it is already started. 
Calling Start() on an already started object will have no effect.  Subclasses
that wish to implement their own object-specific start functionality should do
so in the virtual DoStart() method.   Start() can be called multiple times over
the lifetime of the object, unlike Dispose().

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h#newco...
src/core/model/object.h:169: * an object, unlike DoDispose.
This method stops the Object and all of its aggregates and attributes of type
PointerValue, and sets a state variable to indicate that it is already stopped.
Calling Stop() on an already stopped object will have no effect. Subclasses that
wish to implement their own object-specific stop functionality should do so in
the virtual DoStop() method. Stop() can be called multiple times over the
lifetime of the object, unlike Dispose().

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h#newco...
src/core/model/object.h:181: */
This method returns true if the object is started.

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h#newco...
src/core/model/object.h:209: * to their parent's implementation once they are
done.
(provide an example of what it means to _chain up_)

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h#newco...
src/core/model/object.h:289: void Iterate (Visitor visitor, bool attributes);

Is attributes ever set to true?  Is it being left here for testing?

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h#newco...
src/core/model/object.h:298: static bool ReallyDoStart (Object *object);
I'm searching for another name than "Really..." which seems to obfuscate things
(Start/DoStart/ReallyDoStart).   Perhaps DoStartVisitor()?  Suggestions welcome.

[Tom Henderson, 2012-10-15 13:29:56]

correct my Doxygen suggestion

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h
File src/core/model/object.h (right):

http://codereview.appspot.com/6530051/diff/6002/src/core/model/object.h#newco...
src/core/model/object.h:159: * an object, unlike DoDispose.
On 2012/10/15 13:28:26, Tom Henderson wrote:
> This method starts the Object and all of its aggregates and attributes of type
> PointerValue,

oops, I meant to delete the "and attributes of type PointerValue" once I
understood that this was not occurring

 and sets a state variable to indicate that it is already started. 
> Calling Start() on an already started object will have no effect.  Subclasses
> that wish to implement their own object-specific start functionality should do
> so in the virtual DoStart() method.   Start() can be called multiple times
over
> the lifetime of the object, unlike Dispose().

[Tom Henderson, 2013-01-09 15:06:53]

This patch makes good progress.  It introduces src/core/utils directory.  

Can you suggest an incremental merge plan or do you want to push for getting
more done and then a big merge?   Incremental merges are more work but we could
potentially start to get this into ns-3-dev earlier.

https://codereview.appspot.com/6530051/diff/34001/doc/manual/source/object-mo...
File doc/manual/source/object-model.rst (right):

https://codereview.appspot.com/6530051/diff/34001/doc/manual/source/object-mo...
doc/manual/source/object-model.rst:254: example, one can consider hypotetical
class :cpp:class:`SimpleApplication` that
"hypothetical" (also below)

https://codereview.appspot.com/6530051/diff/34001/src/applications/test/onoff...
File src/applications/test/onoff-application-test-suite.cc (right):

https://codereview.appspot.com/6530051/diff/34001/src/applications/test/onoff...
src/applications/test/onoff-application-test-suite.cc:72: Ptr<OnOffApplication>
app = apps.Get (0)->GetObject<OnOffApplication> ();
I suggest also some tests similar to object test suite, performed on the raw app
pointer (rather than just through the helper) to make sure this is behaving as
expected (and also test the state time tracker results)

https://codereview.appspot.com/6530051/diff/34001/src/core/model/object.h
File src/core/model/object.h (right):

https://codereview.appspot.com/6530051/diff/34001/src/core/model/object.h#new...
src/core/model/object.h:239: * calls Object::Stop multiple times, DoStart is
called only the
DoStop

https://codereview.appspot.com/6530051/diff/34001/src/core/model/object.h#new...
src/core/model/object.h:324: * type atributes
attributes

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
File src/core/utils/started-stopped-state-changer.cc (right):

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
src/core/utils/started-stopped-state-changer.cc:55:
MakeUintegerChecker<uint32_t> ())
I'm not sure about the magic numbers in the defaults.  In particular, why 100
for number of state changes?  This seems prone to soft errors by users who
forget these.  Suggest to set defaults to values that effectively disable
operation unless configured explicitly by users.

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
File src/core/utils/started-stopped-time-tracker.cc (right):

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
src/core/utils/started-stopped-time-tracker.cc:68: NS_LOG_ERROR ("Cannot attach
StartedStoppedTimeTracker " << this << " to object " << obj << ", it is already
attached to object " << m_object);
should this be a hard or soft error?  I suggest to assert or error out here.

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
src/core/utils/started-stopped-time-tracker.cc:126: return
(m_totalTimeStarted.GetSeconds () * 100) / totalTime.GetSeconds ();
protect against divide by zero?

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/state-change...
File src/core/utils/state-change-connector.h (right):

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/state-change...
src/core/utils/state-change-connector.h:39: */
I didn't notice where this is being used.

[Vedran Miletić, 2013-01-18 19:47:52]

Fixed all of those, thanks a lot for your comments. Updated patchset coming.

I would propose the following: merge this
(Node/Application/OnOffApplication/PointToPoint{NetDevice,Channel}) + tests and
eventual documentation (in manual) for core/utils, eventually
Csma{NetDevice,Channel}.

Then we can proceed according to plan (mobility, battery etc.). Since Start/Stop
is optional, we don't need to implement it everywhere at once; I suggest we let
the model maintainers dictate the pace.

I will update the wiki to current state and outstanding work in the next few
days.

https://codereview.appspot.com/6530051/diff/34001/doc/manual/source/object-mo...
File doc/manual/source/object-model.rst (right):

https://codereview.appspot.com/6530051/diff/34001/doc/manual/source/object-mo...
doc/manual/source/object-model.rst:254: example, one can consider hypotetical
class :cpp:class:`SimpleApplication` that
On 2013/01/09 15:06:54, Tom Henderson wrote:
> "hypothetical" (also below)

Fixed.

https://codereview.appspot.com/6530051/diff/34001/src/applications/test/onoff...
File src/applications/test/onoff-application-test-suite.cc (right):

https://codereview.appspot.com/6530051/diff/34001/src/applications/test/onoff...
src/applications/test/onoff-application-test-suite.cc:72: Ptr<OnOffApplication>
app = apps.Get (0)->GetObject<OnOffApplication> ();
On 2013/01/09 15:06:54, Tom Henderson wrote:
> I suggest also some tests similar to object test suite, performed on the raw
app
> pointer (rather than just through the helper) to make sure this is behaving as
> expected (and also test the state time tracker results) 

+1 on both. Fixed.

https://codereview.appspot.com/6530051/diff/34001/src/core/model/object.h
File src/core/model/object.h (right):

https://codereview.appspot.com/6530051/diff/34001/src/core/model/object.h#new...
src/core/model/object.h:239: * calls Object::Stop multiple times, DoStart is
called only the
On 2013/01/09 15:06:54, Tom Henderson wrote:
> DoStop

Fixed.

https://codereview.appspot.com/6530051/diff/34001/src/core/model/object.h#new...
src/core/model/object.h:324: * type atributes
On 2013/01/09 15:06:54, Tom Henderson wrote:
> attributes

Removed entirely, we gave up on this idea.

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
File src/core/utils/started-stopped-time-tracker.cc (right):

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
src/core/utils/started-stopped-time-tracker.cc:68: NS_LOG_ERROR ("Cannot attach
StartedStoppedTimeTracker " << this << " to object " << obj << ", it is already
attached to object " << m_object);
On 2013/01/09 15:06:54, Tom Henderson wrote:
> should this be a hard or soft error?  I suggest to assert or error out here.

assert it is.

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/started-stop...
src/core/utils/started-stopped-time-tracker.cc:126: return
(m_totalTimeStarted.GetSeconds () * 100) / totalTime.GetSeconds ();
On 2013/01/09 15:06:54, Tom Henderson wrote:
> protect against divide by zero?

Fixed.

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/state-change...
File src/core/utils/state-change-connector.h (right):

https://codereview.appspot.com/6530051/diff/34001/src/core/utils/state-change...
src/core/utils/state-change-connector.h:39: */
On 2013/01/09 15:06:54, Tom Henderson wrote:
> I didn't notice where this is being used.

Nowhere yet. It's a (bad as it turned out) prototype of something I needed for
my research and after messing with it decided to solve it in a different way.

[Mathieu Lacage, 2013-01-20 16:27:18]

There are really too many things in this patch:

1) I see changes in the simulator itself, I do not really see why. 

2) It would be nice to get a _single_ minimal patch that introduces the minimal
API for starting and stopping nodes, devices, and applications together with an
implementation for the default devices/applications.

3) we need an introduction to this patch that documents what the intended
semantics of starting/stopping a node, a device, and an application should be.
i.e., what happens for network stacks ? What are they expected to do ?

https://codereview.appspot.com/6530051/diff/44001/src/core/model/default-simu...
File src/core/model/default-simulator-impl.cc (right):

https://codereview.appspot.com/6530051/diff/44001/src/core/model/default-simu...
src/core/model/default-simulator-impl.cc:57: m_main = SystemThread::Self();
is there a reason for that change ?

https://codereview.appspot.com/6530051/diff/44001/src/core/model/default-simu...
src/core/model/default-simulator-impl.cc:191: Object::Start ();
Do you need this to catch 'simulation start' events ?

https://codereview.appspot.com/6530051/diff/44001/src/network/model/node.cc
File src/network/model/node.cc (left):

https://codereview.appspot.com/6530051/diff/44001/src/network/model/node.cc#o...
src/network/model/node.cc:172: for (std::vector<Ptr<NetDevice> >::iterator i =
m_devices.begin ();
that kind of totally gratuitious change does not make patch review any easier.

[Vedran Miletić, 2013-01-20 18:27:13]

Hi Mathieu,

thanks for your comments. Fixed patch coming soon.

https://codereview.appspot.com/6530051/diff/44001/src/core/model/default-simu...
File src/core/model/default-simulator-impl.cc (right):

https://codereview.appspot.com/6530051/diff/44001/src/core/model/default-simu...
src/core/model/default-simulator-impl.cc:57: m_main = SystemThread::Self();
On 2013/01/20 16:27:18, Mathieu Lacage wrote:
> is there a reason for that change ?

Can't remember, probably not. Will revert it.

https://codereview.appspot.com/6530051/diff/44001/src/core/model/default-simu...
src/core/model/default-simulator-impl.cc:191: Object::Start ();
On 2013/01/20 16:27:18, Mathieu Lacage wrote:
> Do you need this to catch 'simulation start' events ?

Yes, this is a replacement for m_stop bool variable in accordance to new
Start/Stop interface. (Simulator is an Object after all.)

Do you have a better idea?

https://codereview.appspot.com/6530051/diff/44001/src/network/model/node.cc
File src/network/model/node.cc (left):

https://codereview.appspot.com/6530051/diff/44001/src/network/model/node.cc#o...
src/network/model/node.cc:172: for (std::vector<Ptr<NetDevice> >::iterator i =
m_devices.begin ();
On 2013/01/20 16:27:18, Mathieu Lacage wrote:
> that kind of totally gratuitious change does not make patch review any easier.


I will remove these, updated patch coming soon.