Bug 2213

Instead of 
+StringValue
+McsHelper::DataRateForVhtMcs (int mcs)
+{
+  std::stringstream sstmp;
+  std::string strtmp, dataRate;
+
+  sstmp << mcs;
+  sstmp >> strtmp;
+  dataRate = "VhtMcs" + strtmp;
+  
+  return StringValue (dataRate);
+}

you could simply use
+StringValue
+McsHelper::DataRateForVhtMcs (int mcs)
+{
+  std::ostringstream oss;
+  oss << "VhtMcs" << mcs;
+  
+  return StringValue (oss.str ());
+}

Moreover, the Doxygen description is somewhat misleading: "Converts a VHT MCS value into a DataRate value". It's not exactly a DataRate (if I'm right).

Other than this, I'm ok with the proposed changes, but they will break a LOT of simulations.
A more kind (to the users) approach is to deprecate the old helpers and remove them in 3.26.

(In reply to Tommaso Pecorella from comment #1)
> Instead of 
> +StringValue
> +McsHelper::DataRateForVhtMcs (int mcs)
> +{
> +  std::stringstream sstmp;
> +  std::string strtmp, dataRate;
> +
> +  sstmp << mcs;
> +  sstmp >> strtmp;
> +  dataRate = "VhtMcs" + strtmp;
> +  
> +  return StringValue (dataRate);
> +}
> 
> you could simply use
> +StringValue
> +McsHelper::DataRateForVhtMcs (int mcs)
> +{
> +  std::ostringstream oss;
> +  oss << "VhtMcs" << mcs;
> +  
> +  return StringValue (oss.str ());
> +}
> 

Thanks, it seems better this way.

> Moreover, the Doxygen description is somewhat misleading: "Converts a VHT
> MCS value into a DataRate value". It's not exactly a DataRate (if I'm right).

You are right, I will adapt the description.

> 
> Other than this, I'm ok with the proposed changes, but they will break a LOT
> of simulations.
> A more kind (to the users) approach is to deprecate the old helpers and
> remove them in 3.26.

Indeed, users will have to adapt their simulation scripts. The same applies for bug 2116. However, I think it's the good moment to change it, it's already too long that we provide a too "complex" API to configure HT parameters.

Tom, any thought on this?

In general, I would like to reduce the number of wifi helpers that users have to provide in the common cases.  I left a comment in bug 2116 that IMO it would be preferable to allow users to specify a standard such as 802.11n or ac with a single statement and get a default configuration that enables the key features (e.g. aggregation).  

Regarding this patch in particular, can providing QoS be made default for 11n/ac and this QoS helper used to configure non-default QoS-related attributes if needed on a per-container basis?  

Rather than deleting the Ht helper, is there a need to keep around an Ht or Vht helper to provide per-container configuration of HT or VHT parameters (i.e. if we have a helper dedicated to QoS configuration, why wouldn't we have one for HT configuration?).

Regarding DataRateForHtMcs (int mcs);, I did not understand how this would work since data rate depends not only on mcs but also on guard interval and channel width.

Wouldn't it be better to have a single helper (so even no distinction qos and nqos), and based on what is passed as standard to the wifi helper, we could select the appropriate parameters?

DataRateForHtMcs simply does a conversion from an mcs (integer) to a HtMcsX (string).
But it has no real purpose, I assume this can maybe simply be removed.

(In reply to sebastien.deronne from comment #5)
> Wouldn't it be better to have a single helper (so even no distinction qos
> and nqos), and based on what is passed as standard to the wifi helper, we
> could select the appropriate parameters?

That would be fine with me, but let's try to sketch it out a bit before deciding.

It would probably be a good idea to prioritize how we would do this for both 11n and 11ac, and then we could see whether we want to spend the time to also support 11a/b/g and what additional wrinkles those cause.

> 
> DataRateForHtMcs simply does a conversion from an mcs (integer) to a HtMcsX
> (string).
> But it has no real purpose, I assume this can maybe simply be removed.

OK

Latest changes updated in the code review:
https://codereview.appspot.com/281230043/

new patch set added to the review

pushed in changeset 11854:7c60a9f8f271