(In reply to Rohit P. Tahiliani from comment #0)
> Created attachment 2614 [details]
> Fixes TCP Variant configuration in wifi-tcp.cc
> 
> Although wifi-tcp.cc in examples/wireless/ permits the user to select the
> TCP variant using a command line argument, it does not configure the chosen
> TCP variant internally.
> 
> The patch provided here fixes this.
> 
> Also, the example has been extended to work with recently added TCP variants
> in ns-3-dev.
> 
> Thanks,
> Rohit P. Tahiliani

Rohit, can you investigate a change to the patch that makes use of TypeId::LookupByName (std::string name) to avoid having to enumerate all possibilities?  Such as:

if (tcpVariant.compare ("TcpWestwoodPlus") == 0)
  { 
    // TcpWestwoodPlus is not an actual TypeId name; we need TcpWestwood here
    Config::SetDefault ("ns3::TcpL4Protocol::SocketType", TypeIdValue (TcpWestwood::GetTypeId ()));
    // the default protocol type in ns3::TcpWestwood is WESTWOOD
    Config::SetDefault ("ns3::TcpWestwood::ProtocolType", EnumValue (TcpWestwood::WESTWOODPLUS));
  }
else
  {
    Config::SetDefault ("ns3::TcpL4Protocol::SocketType", TypeIdValue (TypeId::LookupByName (tcpVariant)));
  }

Note that I deleted the setting of Westwood FilterType to TUSTIN since it is already the default.

If the above works out well, can you look around the other examples that may be enumerating the variants in the same way, and add a similar improvement to those other examples?

(In reply to Tom Henderson from comment #1)
> (In reply to Rohit P. Tahiliani from comment #0)
> > Created attachment 2614 [details]
> > Fixes TCP Variant configuration in wifi-tcp.cc
> > 
> > Although wifi-tcp.cc in examples/wireless/ permits the user to select the
> > TCP variant using a command line argument, it does not configure the chosen
> > TCP variant internally.
> > 
> > The patch provided here fixes this.
> > 
> > Also, the example has been extended to work with recently added TCP variants
> > in ns-3-dev.
> > 
> > Thanks,
> > Rohit P. Tahiliani
> 
> Rohit, can you investigate a change to the patch that makes use of
> TypeId::LookupByName (std::string name) to avoid having to enumerate all
> possibilities?  Such as:
> 
> if (tcpVariant.compare ("TcpWestwoodPlus") == 0)
>   { 
>     // TcpWestwoodPlus is not an actual TypeId name; we need TcpWestwood here
>     Config::SetDefault ("ns3::TcpL4Protocol::SocketType", TypeIdValue
> (TcpWestwood::GetTypeId ()));
>     // the default protocol type in ns3::TcpWestwood is WESTWOOD
>     Config::SetDefault ("ns3::TcpWestwood::ProtocolType", EnumValue
> (TcpWestwood::WESTWOODPLUS));
>   }
> else
>   {
>     Config::SetDefault ("ns3::TcpL4Protocol::SocketType", TypeIdValue
> (TypeId::LookupByName (tcpVariant)));
>   }
> 
> Note that I deleted the setting of Westwood FilterType to TUSTIN since it is
> already the default.
> 
> If the above works out well, can you look around the other examples that may
> be enumerating the variants in the same way, and add a similar improvement
> to those other examples?

Hi Tom Sir,

Thank you for the suggestion. It works perfect. However, I had one query on using the LookupByName. If the input name is not a valid TypeId name then the method would crash. Is that ok?

Moreover, I will look around the other examples enumerating the variants in the same way and will add a similar improvement to those.

Thank you,
Rohit

> 
> Hi Tom Sir,
> 
> Thank you for the suggestion. It works perfect. However, I had one query on
> using the LookupByName. If the input name is not a valid TypeId name then
> the method would crash. Is that ok?

Well, it asserts if not found, and it will print out to the user the reason for the failure:

assert failed. cond="uid != 0", msg="Assert in TypeId::LookupByName: ... not found", file=../src/core/model/type-id.cc, line=817

I don't think that the program should execute if the user passes an invalid string.   Your patch also exits with code 1, but without the assert error string.

There is a variant of the lookup called LookupByNameFailSafe which does not crash upon invalid string but instead returns false, so you could wrap this check such as:

TypeId tcpTid;
if (TypeId::LookupByNameFailSafe (tcpVariant, &tid) == false)
  {
    NS_ABORT_MSG ("TypeId " << tcpVariant << " not found");
    // or do whatever else you want to do here if the string is invalid
  }

or this can be written more compactly as:

TypeId tcpTid;
NS_ABORT_MSG_UNLESS (TypeId::LookupByNameFailSafe (tcpVariant, &tcpTid), "TypeId " << tcpVariant << " not found");

but you will get a similar error output and error exit as before.

One benefit of doing this type of check and explicitly aborting, instead of relying on the assert, is that problems will be caught in non-debug builds too, and it might be more readable to the end user.

Created attachment 2619 [details]
Avoid enumerating TCP variants

Thank you for a comprehensive reply. It was really useful. 

My concern was the same: I wanted the program to abort / exit safely and give a meaningful message to the end user, instead of relying on assert error string.

I have created a patch as per your suggestions. Please let me know if it holds good. I'll accordingly update other examples.

Thank you,
Rohit

> 
> I have created a patch as per your suggestions. Please let me know if it
> holds good. I'll accordingly update other examples.
> 
> Thank you,
> Rohit


This looks good to me; I will move to last call and commit it next week if no further comments.

Created attachment 2678 [details]
Revised patch

Thanks for the confirmation.

I'll make similar changes in other examples that iterate through the TCP variants and file a new bug for them.

Meanwhile, I also noticed that the spelling of "throughput" in the following line (last line of wifi-tcp.cc) is incorrect:

std::cout << "\nAverage throughtput: " << averageThroughput << " Mbit/s" << std::endl;

So I've attached a new patch which corrects the same.

Thank you.

Created attachment 2857 [details]
Latest patch

Hi Tom Sir,

I have uploaded a new patch based on the latest ns-3-dev. 

Thank you,
Rohit

committed in changeset 12908:4dbc790cd45e