The problem is due to setting an attribute that is restricted to GET operations:


    .AddAttribute ("Velocity", "The current velocity of the mobility model.",
                   TypeId::ATTR_GET,
                   VectorValue (Vector (0.0, 0.0, 0.0)), // ignored initial value.
                   MakeVectorAccessor (&MobilityModel::GetVelocity),
                   MakeVectorChecker ())


The below code will not raise an error (fails, silently, to set the Velocity):

	MobilityHelper ueMob;	
	ueMob.SetMobilityModel ("ns3::ConstantVelocityMobilityModel",
					"Position", Vector3DValue(Vector3D (500, 900, 0)),
					"Velocity", Vector3DValue(Vector3D(25, 0, 0)));

A couple of issues are raised by this, which need further discussion:

1) should the above code cause an execution error (ASSERT or ABORT) with an attribute with only ATTR_GET flag set?  There does not seem to be any test code in src/core/test/attribute-test-suite.cc for this.

Mathieu, how do you want src/core, in general, to behave in this case?

2) how to support this for the ConstantVelocity mobility model?

Some options:
- leave attribute as ATTR_GET, but fix src/core so that it will error out in the provided helper code above.  Document that constant velocity mobility model needs to set its velocity through an explicit call to SetVelocity()

- make attribute settable, and figure out the appropriate behavior for other mobility models.  I'm not sure if setting the value at initialization time to (0,0,0) has any side effect on the other mobility models.

This is a case in which setting this attribute would only typically be done for one of the subclasses (ConstantVelocity) but getting this attribute value would be valid for all.  If it could be redefined to have SET flag active only in the ConstantVelocity subclass, that would seem to be the best solution, but not sure the attribute system can handle such an overload.

(In reply to comment #1)
> The problem is due to setting an attribute that is restricted to GET
> operations:
> 
> 
>     .AddAttribute ("Velocity", "The current velocity of the mobility model.",
>                    TypeId::ATTR_GET,
>                    VectorValue (Vector (0.0, 0.0, 0.0)), // ignored initial
> value.
>                    MakeVectorAccessor (&MobilityModel::GetVelocity),
>                    MakeVectorChecker ())
> 
> 
> The below code will not raise an error (fails, silently, to set the Velocity):
> 
>     MobilityHelper ueMob;    
>     ueMob.SetMobilityModel ("ns3::ConstantVelocityMobilityModel",
>                     "Position", Vector3DValue(Vector3D (500, 900, 0)),
>                     "Velocity", Vector3DValue(Vector3D(25, 0, 0)));
> 
> A couple of issues are raised by this, which need further discussion:
> 
> 1) should the above code cause an execution error (ASSERT or ABORT) with an
> attribute with only ATTR_GET flag set?  There does not seem to be any test code
> in src/core/test/attribute-test-suite.cc for this.
> 
> Mathieu, how do you want src/core, in general, to behave in this case?

We should be able to detect that the attribute setting failed at construction time and report that. An assert that reports useful information would be probably useful. i.e., ObjectBase::ConstructSelf should detect when an explicit initial value was given for an attribute but it was unable to set it. Maybe by keeping track of which attributes in the input AttributeConstructionList have been used or not ? I have added this to my TODO but anyone is free to beat me to it.

> 
> 2) how to support this for the ConstantVelocity mobility model?
> 
> Some options:
> - leave attribute as ATTR_GET, but fix src/core so that it will error out in
> the provided helper code above.  Document that constant velocity mobility model
> needs to set its velocity through an explicit call to SetVelocity()

This seems to be a good idea whatever we do for the mobility model.

> 
> - make attribute settable, and figure out the appropriate behavior for other
> mobility models.  I'm not sure if setting the value at initialization time to
> (0,0,0) has any side effect on the other mobility models.
> 
> This is a case in which setting this attribute would only typically be done for
> one of the subclasses (ConstantVelocity) but getting this attribute value would
> be valid for all.  If it could be redefined to have SET flag active only in the
> ConstantVelocity subclass, that would seem to be the best solution, but not
> sure the attribute system can handle such an overload.

Yes, the ideal behavior would be to allow the subclass to extend the attribute definition for itself to allow initialization+generic setting. I am not sure how this could be done but I will think about it.


thanks,

I'd rather introduce a new rw attribute InitialVelocity for ConstantVelocityMobilityModel keeping current velocity read-only.

(In reply to comment #3)
> I'd rather introduce a new rw attribute InitialVelocity for
> ConstantVelocityMobilityModel keeping current velocity read-only.


OK, seems like the suggested path forward is:

1) ObjectBase::ConstructSelf should detect when an explicit
initial value was given for an attribute but it was unable to set it. Maybe by
keeping track of which attributes in the input AttributeConstructionList have
been used or not ? I have added this to my TODO but anyone is free to beat me
to it.

2) Add new rw attribute InitialVelocity to ConstantVelocityMobilityModel

We'll try to beat you to it.

> 
> 2) Add new rw attribute InitialVelocity to ConstantVelocityMobilityModel
> 
> We'll try to beat you to it.

#2 has a complication; MobilityModel's Position attribute has a side effect of setting Velocity to zero.  We do not want to make attributes depend on one another (initialization order issues) so we seem to have new options to consider here.

- proceed with adding the InitialVelocity attribute, but cache its value in a member variable and perform a lazy initialization (such at Object::Start time).  This is a little bit breaking the rules about dependencies among attributes; it is effectively ensuring that InitialVelocity is set after Position attribute.

- forget about setting the velocity through the attribute system; make users set velocity explicitly after object is fully constructed.

- change the behavior of ConstantVelocityHelper::SetPosition().  Currently, it sets velocity to zero, but it could instead set m_paused to true and leave velocity untouched.  It would probably require more explicity handling of Pause() and Unpause() by clients if we were to fully decouple the setting of these parameters from the pausing and unpausing of the motion.

Considering that an user could set the position in an arbitrary simulation instant (and the attributes are there also for this reason), I'd go for the third option.

I'm not sure about the m_paused, tho. If an user is wise (or dumb) anough to set the position manually, then he/she should be also responsible for (eventually) pause and un-pause the mobility. If this is the intended behaviour.
If the user want to have a node jumping around like Crazy Ivan, so it be.

In other words: if the user is going to set the position explicitly and wants *also* to set exactly when the node will start moving, then he'll have to call pause/unpause. Else the node will start moving right away after the position setup.

Honestly I don't see why we shoud pause the node after a forced position change. Maybe I'm missing some internals problem of the model, tho.

(In reply to comment #4)
> (In reply to comment #3)
> > I'd rather introduce a new rw attribute InitialVelocity for
> > ConstantVelocityMobilityModel keeping current velocity read-only.
> 
> 
> OK, seems like the suggested path forward is:
> 
> 1) ObjectBase::ConstructSelf should detect when an explicit
> initial value was given for an attribute but it was unable to set it. Maybe by
> keeping track of which attributes in the input AttributeConstructionList have
> been used or not ? I have added this to my TODO but anyone is free to beat me
> to it.
> 
> 2) Add new rw attribute InitialVelocity to ConstantVelocityMobilityModel
> 
> We'll try to beat you to it.

For step 1), should this situation result in an assert or an abort?

> 
> For step 1), should this situation result in an assert or an abort?


I suggest NS_FATAL_ERROR in keeping with ObjectBase::SetAttribute() exception handling.

Created attachment 1639 [details]
Patch to detect unsettable attributes in ConstructSelf()

This patch makes ConstructSelf() detect unsettable attributes and give an error message in necessary.

(In reply to comment #9)
> Created attachment 1639 [details]
> Patch to detect unsettable attributes in ConstructSelf()
> 
> This patch makes ConstructSelf() detect unsettable attributes and give an error
> message in necessary.

This patch fixes step 1).

Mathieu, do you have time to review it?

(In reply to Mitch Watrous from comment #10)
> (In reply to comment #9)
> > Created attachment 1639 [details]
> > Patch to detect unsettable attributes in ConstructSelf()
> > 
> > This patch makes ConstructSelf() detect unsettable attributes and give an error
> > message in necessary.
> 
> This patch fixes step 1).
> 
> Mathieu, do you have time to review it?

looks good to me. +1 for merging

This has never been pushed, despite the +1.

We should consider it again, and apply it (I guess it's still relevant).

Pushed "step 1" in changeset: 10710:1c733dcc50b2

Leaving the bug open for step 2.