Bug 1409

this seems to be a trivial fix to repair what appears to be an omission.

However, it makes me wonder why this is handled separately from Node::m_id which is an attribute. Any objections to aligning this with 'Id' and making an attribute?

Specifically, I suggest to preserve the constructor that takes argument (for backward compatibility, but making sure that the attribute is correctly initialized by using ObjectBase::ConstructSelf()), and to make 'SystemId' into an attribute similar to 'Id' (but with ability to set and get value).  API then would look like:

node->SetAttribute ("SystemId", UintegerValue (1));

Also, this line in the doxygen is incorrect and should be removed:

   * Must be invoked by subclasses only.

Created attachment 1419 [details]
updated patchset

Thanks Tom.

(In reply to comment #1)
> this seems to be a trivial fix to repair what appears to be an omission.
> 
> However, it makes me wonder why this is handled separately from Node::m_id
> which is an attribute. Any objections to aligning this with 'Id' and making an
> attribute?

no objections. I agree.

> Specifically, I suggest to preserve the constructor that takes argument (for
> backward compatibility, but making sure that the attribute is correctly
> initialized by using ObjectBase::ConstructSelf()), and to make 'SystemId' into
> an attribute similar to 'Id' (but with ability to set and get value).  API then
> would look like:
> 
> node->SetAttribute ("SystemId", UintegerValue (1));

I've updated the patch.
but the attached patch (Attachment 1419 [details]) doesn't have a backward compatibility: Node (sid) doesn't correctly configure the system id.

I will work on fixing it.

> Also, this line in the doxygen is incorrect and should be removed:
> 
>    * Must be invoked by subclasses only.

To tell the truth, I don't understand the above comments :-
Do you mean both constructors should remove the above doxygen comment?
Or only the one takes argument (of systemid)?

Created attachment 1420 [details]
patch to configure attribute properly with backward compatibility.

I found that TypeId::ATTR_GET|TypeId::ATTR_SET should be the right parameter for that purpose (configure the attribute with the parameter of Constructor or SetAttribute()).

(In reply to comment #3)
> Thanks Tom.
> 
> (In reply to comment #1)
> > this seems to be a trivial fix to repair what appears to be an omission.
> > 
> > However, it makes me wonder why this is handled separately from Node::m_id
> > which is an attribute. Any objections to aligning this with 'Id' and making an
> > attribute?
> 
> no objections. I agree.
> 
> > Specifically, I suggest to preserve the constructor that takes argument (for
> > backward compatibility, but making sure that the attribute is correctly
> > initialized by using ObjectBase::ConstructSelf()), and to make 'SystemId' into
> > an attribute similar to 'Id' (but with ability to set and get value).  API then
> > would look like:
> > 
> > node->SetAttribute ("SystemId", UintegerValue (1));
> 
> I've updated the patch.
> but the attached patch (Attachment 1419 [details]) doesn't have a backward compatibility:
> Node (sid) doesn't correctly configure the system id.
> 
> I will work on fixing it.

The problem is that attributes are assigned values after the constructor is called, so any constructor-initialized attribute will be overwritten.

I believe that you can get around this by inserting this statement into the constructor body:

 ObjectBase::ConstructSelf (AttributeConstructionList ());

and then set the system id after this statement; this statement accelerates the assignment of attribute values.

class RttEstimator is an example of doing this.  However, this suggestion is untested.

Cleaner solutions exist for this problem, depending on whether we want to introduce new API and deprecate the old one.  For example, the above need to use ConstructSelf() could be eliminated by using CreateObjectWithAttributes() to set the system ID (once it is an attribute), and not a constructor taking regular arguments (which is an less common API for ns-3).  And, the system ID could be relocated to an MPI interface specific to the MPI module (since that is the only module that makes use of this parameter), but that might require introducing some new NodeContainer types that understood systemId, and the low-level CreateObject<Node> () would need to be augmented with MPI-setting statements or wrapped in a new helper such as CreateMpiNode (sId);

> 
> > Also, this line in the doxygen is incorrect and should be removed:
> > 
> >    * Must be invoked by subclasses only.
> 
> To tell the truth, I don't understand the above comments :-
> Do you mean both constructors should remove the above doxygen comment?
> Or only the one takes argument (of systemid)?

Yes, this issue can be handled separately.  I'll clean it out now in ns-3-dev.  This comment is a carry over from GTNetS which used node subclasses (but we do not in ns-3).

(In reply to comment #5)
> (In reply to comment #3)
> > > Specifically, I suggest to preserve the constructor that takes argument (for
> > > backward compatibility, but making sure that the attribute is correctly
> > > initialized by using ObjectBase::ConstructSelf()), and to make 'SystemId' into
> > > an attribute similar to 'Id' (but with ability to set and get value).  API then
> > > would look like:
> > > 
> > > node->SetAttribute ("SystemId", UintegerValue (1));
> > 
> > I've updated the patch.
> > but the attached patch (Attachment 1419 [details]) doesn't have a backward compatibility:
> > Node (sid) doesn't correctly configure the system id.
> > 
> > I will work on fixing it.
> 
> The problem is that attributes are assigned values after the constructor is
> called, so any constructor-initialized attribute will be overwritten.
> 
> I believe that you can get around this by inserting this statement into the
> constructor body:
> 
>  ObjectBase::ConstructSelf (AttributeConstructionList ());
> 
> and then set the system id after this statement; this statement accelerates the
> assignment of attribute values.
> 
> class RttEstimator is an example of doing this.  However, this suggestion is
> untested.

does the following statement solve your problem (in class RttEstimator) without calling ObjectBase::ConstructSelf ()?

     .AddAttribute ("InitialEstimation", 
                    "Initial RTT estimation",
+                  TypeId::ATTR_GET|TypeId::ATTR_SET,
                    TimeValue (Seconds (1.0)),
                    MakeTimeAccessor (&RttEstimator::m_initialEstimatedRtt),
                    MakeTimeChecker ())

where the additional line avoids to initialize the value during the constructor.
it is also implemented in my latest patch (attachment 1420 [details]).

> Cleaner solutions exist for this problem, depending on whether we want to
> introduce new API and deprecate the old one.  For example, the above need to
> use ConstructSelf() could be eliminated by using CreateObjectWithAttributes()
> to set the system ID (once it is an attribute), and not a constructor taking
> regular arguments (which is an less common API for ns-3).  And, the system ID
> could be relocated to an MPI interface specific to the MPI module (since that
> is the only module that makes use of this parameter), but that might require
> introducing some new NodeContainer types that understood systemId, and the
> low-level CreateObject<Node> () would need to be augmented with MPI-setting
> statements or wrapped in a new helper such as CreateMpiNode (sId);

I don't have much opinion for that at this moment.

> > 
> > > Also, this line in the doxygen is incorrect and should be removed:
> > > 
> > >    * Must be invoked by subclasses only.
> > 
> > To tell the truth, I don't understand the above comments :-
> > Do you mean both constructors should remove the above doxygen comment?
> > Or only the one takes argument (of systemid)?
> 
> Yes, this issue can be handled separately.  I'll clean it out now in ns-3-dev. 
> This comment is a carry over from GTNetS which used node subclasses (but we do
> not in ns-3).

Ah, now I understand what you meant. thanks.

any opinion? if not, I will push the patch.

Any reason not to commit this for upcoming build?

(In reply to comment #8)
> Any reason not to commit this for upcoming build?

fixed and pushed the following changset.

changeset 9288	3791287491cd