IMO, the best way is to call CalcRxPower by specifying the frequency of the tx signal.
CalcRxPower thus needs to be extended for each model.

Indeed, we could imagine that both 2.4 and 5GHz signals are transmitted on the wireless channel. By calling CalcRxPower with the correct frequency, we are sure that the computed signal attenuation is done w.r.t. the signal frequency.

Created attachment 2061 [details]
proposed patch

I propose to set the correct frequency on the flight (when needed) directly in YansWifiChannel (see attached patch).

Good point:
-> since there is dependencies on propagation loss models, it avoids changes in other modules.
-> multiple type of signals can "share" the same channel.
-> users no longer need to explicitly set the frequency used in the simulation. Or even better, it avoids wrong results when users do not set the frequency in the propagation loss model!

Bad points:
-> computation at each reception.

Any idea of improvement?
Any objection?

Note that the proposed fix would no longer allow users to customize frequency-related parameters in the propagation model, but this is for a good reason.
I do not imagine a user would specify frequency Y while the wifi signal frequency would be X...

Hello Sebastien.

I have some comments and questions.

If I understand correctly you are setting the frequency at the Send function so as to allow devices using different frequencies to use the same channel and the same propagation model.
I think this is ok.

What I do not understand is why for the LogDistance models you do the calculation in the channel and not in the propagation model.
Do you think it generate to many changes to add a SetFrequency to those models?
My proposal is to add the SetFrequency function to these models, do the refLoss calculation in this new function and take out the attribute ReferenceLoss.

With your changes, if the user modify this attribute when creating the model we are overwriting it, so in either case I think we should remove it.

Matias,

Thanks for your comment.
I will check whether it can be done, it sounds indeed better.

The users will not be allowed to overwrite the frequency with my changes, but IMO it has no sense to set a different frequency for the propagation channel than the frequency being used by the propagating signal.

I agree that SetFrequency should be added to those models (or directly in the parent class?).

It may happen that the user want to specify different reference loss/distance values than the default one, and those values should not be overwritten in that case. 
They should be overwritten only if the user do not specify anything.

Is it ok to add a check like this?
if (m_foo != defaut_value)
  {
    ...
  }

(In reply to sebastien.deronne from comment #6)
> I agree that SetFrequency should be added to those models (or directly in
> the parent class?).
We should consider that there could be models that don't use the frequency for the calculation.
> 
> It may happen that the user want to specify different reference
> loss/distance values than the default one, and those values should not be
> overwritten in that case. 
> They should be overwritten only if the user do not specify anything.
> 
> Is it ok to add a check like this?
> if (m_foo != defaut_value)
>   {
>     ...
>   }

I don't completely understand what you are comparing.
The problem I see is that the default reference loss value depends on the frequency and the distance.

(In reply to Matías Richart from comment #7)
> (In reply to sebastien.deronne from comment #6)
> > I agree that SetFrequency should be added to those models (or directly in
> > the parent class?).
> We should consider that there could be models that don't use the frequency
> for the calculation.

Indeed, but in that case it should just be ignored.
The good point to have SetFrequency() defined in the parent class is that we can have this simple call in YansWifiChannel:
m_loss->SetFrequency (...)

Otherwise, you need to get the model(s) that are attached to the channel, and then set the frequency separately for each of them if needed.

> > 
> > It may happen that the user want to specify different reference
> > loss/distance values than the default one, and those values should not be
> > overwritten in that case. 
> > They should be overwritten only if the user do not specify anything.
> > 
> > Is it ok to add a check like this?
> > if (m_foo != defaut_value)
> >   {
> >     ...
> >   }
> 
> I don't completely understand what you are comparing.
> The problem I see is that the default reference loss value depends on the
> frequency and the distance.

Indeed, that is why we need to be sure that the user is aware of this.

If the user intentionally tuned the reference loss/distance, we need to not overwrite it (it is at his own risk, and will be used even if different signals are propagating).

On the contrary, if the user did not set those values, it should be overwritten with the correct frequency.

So, in order to know whether the user did overwrite the value(s), I suggest to add a check like this:

if (m_foo != defaut_value)
  {
    ...
  }

(In reply to sebastien.deronne from comment #8)
> (In reply to Matías Richart from comment #7)
> > (In reply to sebastien.deronne from comment #6)
> > > I agree that SetFrequency should be added to those models (or directly in
> > > the parent class?).
> > We should consider that there could be models that don't use the frequency
> > for the calculation.
> 
> Indeed, but in that case it should just be ignored.
> The good point to have SetFrequency() defined in the parent class is that we
> can have this simple call in YansWifiChannel:
> m_loss->SetFrequency (...)
> 
> Otherwise, you need to get the model(s) that are attached to the channel,
> and then set the frequency separately for each of them if needed.

Good point. I agree.
> 
> > > 
> > > It may happen that the user want to specify different reference
> > > loss/distance values than the default one, and those values should not be
> > > overwritten in that case. 
> > > They should be overwritten only if the user do not specify anything.
> > > 
> > > Is it ok to add a check like this?
> > > if (m_foo != defaut_value)
> > >   {
> > >     ...
> > >   }
> > 
> > I don't completely understand what you are comparing.
> > The problem I see is that the default reference loss value depends on the
> > frequency and the distance.
> 
> Indeed, that is why we need to be sure that the user is aware of this.
> 
> If the user intentionally tuned the reference loss/distance, we need to not
> overwrite it (it is at his own risk, and will be used even if different
> signals are propagating).
> 
> On the contrary, if the user did not set those values, it should be
> overwritten with the correct frequency.
> 
> So, in order to know whether the user did overwrite the value(s), I suggest
> to add a check like this:
> 
> if (m_foo != defaut_value)
>   {
>     ...
>   }

Ok, I understand. What do you say to do if the user only tune the distance? Should we calculate the "correct" reference loss?

(In reply to Matías Richart from comment #9)
> (In reply to sebastien.deronne from comment #8)
> > (In reply to Matías Richart from comment #7)
> > > (In reply to sebastien.deronne from comment #6)
> > > > I agree that SetFrequency should be added to those models (or directly in
> > > > the parent class?).
> > > We should consider that there could be models that don't use the frequency
> > > for the calculation.
> > 
> > Indeed, but in that case it should just be ignored.
> > The good point to have SetFrequency() defined in the parent class is that we
> > can have this simple call in YansWifiChannel:
> > m_loss->SetFrequency (...)
> > 
> > Otherwise, you need to get the model(s) that are attached to the channel,
> > and then set the frequency separately for each of them if needed.
> 
> Good point. I agree.
> > 
> > > > 
> > > > It may happen that the user want to specify different reference
> > > > loss/distance values than the default one, and those values should not be
> > > > overwritten in that case. 
> > > > They should be overwritten only if the user do not specify anything.
> > > > 
> > > > Is it ok to add a check like this?
> > > > if (m_foo != defaut_value)
> > > >   {
> > > >     ...
> > > >   }
> > > 
> > > I don't completely understand what you are comparing.
> > > The problem I see is that the default reference loss value depends on the
> > > frequency and the distance.
> > 
> > Indeed, that is why we need to be sure that the user is aware of this.
> > 
> > If the user intentionally tuned the reference loss/distance, we need to not
> > overwrite it (it is at his own risk, and will be used even if different
> > signals are propagating).
> > 
> > On the contrary, if the user did not set those values, it should be
> > overwritten with the correct frequency.
> > 
> > So, in order to know whether the user did overwrite the value(s), I suggest
> > to add a check like this:
> > 
> > if (m_foo != defaut_value)
> >   {
> >     ...
> >   }
> 
> Ok, I understand. What do you say to do if the user only tune the distance?
> Should we calculate the "correct" reference loss?

Yes, it is what I suggest.
This should also be made clear in the documentation then.

I'm not in favor of making YansWifiChannel have to be aware of the type of loss model, as in:

+  if (m_loss->GetObject<FriisPropagationLossModel> () != 0)
+    {
+      m_loss->GetObject<FriisPropagationLossModel> ()->SetFrequency(frequency * 1000000);

this approach will break over time.  I'd rather see that the loss model is passed the frequency as an argument to CalcRxPower().

also, 
> Is it ok to add a check like this?
> if (m_foo != defaut_value)
>   {
>    ...
>  }

I don't think we have any facility to remember whether the user touched a default value as in Config::SetDefault(..).  If you are checking whether the instance value matches the current default value, that could be done.  But it makes me wonder whether the need to do this is an indication that the overall design is not correct...

(In reply to Tom Henderson from comment #11)
> I'm not in favor of making YansWifiChannel have to be aware of the type of
> loss model, as in:
> 
> +  if (m_loss->GetObject<FriisPropagationLossModel> () != 0)
> +    {
> +      m_loss->GetObject<FriisPropagationLossModel>
> ()->SetFrequency(frequency * 1000000);
> 
> this approach will break over time.  I'd rather see that the loss model is
> passed the frequency as an argument to CalcRxPower().

This could indeed be a good option.
Does this seem cleaner than adding a SetFrequency in the parent class?

> 
> also, 
> > Is it ok to add a check like this?
> > if (m_foo != defaut_value)
> >   {
> >    ...
> >  }
> 
> I don't think we have any facility to remember whether the user touched a
> default value as in Config::SetDefault(..).  If you are checking whether the
> instance value matches the current default value, that could be done.  But
> it makes me wonder whether the need to do this is an indication that the
> overall design is not correct...

Unfortunately, I do not see any other option for the moment.

Hi,

I agree that it would be better to have SetFrequency method in parent class.
All subclasses should decide how to deal with frequency property by themselves.
Some may ignore this if they do not need this.
Some shall do calculation further with this property.
So the SetReference and SetReferenceLoss in the proposed patch will become private in the different classes.
Besides that, it can also make YansWifiChannel::Send method implemented easier than current proposed path.

Junling

*** Bug 2648 has been marked as a duplicate of this bug. ***

I would like to handle this one in the coming weeks.
Some new wifi standards are using different frequency bands (1 GHz, 60 GHz, ...), so this will become more and more critical.

I would really like to make changes to "propagation" module instead and change its API to accept frequency each time. And also change "spectrum" module to use propagation loss models from the "propagation" model. Currently we have friis-spectrum-propagation-loss.cc simply because "propagation" API is bad, and each time I need to use some propagation loss model with the spectrum channel, I have to copy it from propagation module and adopt it similarly to friis-spectrum-propagation-loss.cc, with lots of duplicate code.

Besides that, proposed patch does a lot of GetObject calls inside the Send that are expensive. And adding a new propagation loss model requires adding another "if" branch.

I think breaking API is worth it in this case.

(In reply to Alexander Krotov from comment #16)
> 
> I think breaking API is worth it in this case.

I agree; do you have any cycles to work on a new patch?

I suggest to keep the existing API and Frequency attributes (and Set/GetFrequency) for a release cycle or two with NS_DEPRECATED and add the new API taking frequency argument.

I started implementing it, but it seems we need to deal with https://www.nsnam.org/bugzilla/show_bug.cgi?id=2001 first.

Created attachment 3157 [details]
Proposed patch to change CalcRxPower API

I think your idea is indeed better.
I see you made changes to the propagation module, do you plan to also adapt wifi and other wireless modules?

> I see you made changes to the propagation module, do you plan to also adapt wifi and other wireless modules?

The patch is not finished yet.

I need to make changes to everything else: spectrum model (remove m_propagationLoss from MultiModelSpectrumChannel, make a template wrapper for normal propagation loss models), change wi-fi etc.

> I suggest to keep the existing API and Frequency attributes (and Set/GetFrequency) for a release cycle or two with NS_DEPRECATED and add the new API taking frequency argument.

I am trying to do it, but there will be incompatibilities anyway, because I will have to change channel models.

Moved to https://gitlab.com/nsnam/ns-3-dev/-/issues/174