(In reply to Hossam Khader from comment #0)
> Created attachment 2429 [details]
> uan-application
> 
> Add a new feature to he UAN module

Hi Hossam,
please re-check the patch, the file names are not present.

I tried applying the patch, and it succeeded.

It should add 5 new files and update the UAN wscripts to include them.

These files are included in the AUV code to be reviewed, and should move under the UAN module.


Hossam

Hi Hossam,
I believe you that the patch is working :)
What I meant is that 
https://www.nsnam.org/bugzilla/attachment.cgi?id=2429&action=diff

is not showing the names of the added files.

also the raw 
https://www.nsnam.org/bugzilla/attachment.cgi?id=2429&action=diff&context=patch&collapsed=&headers=1&format=raw

is not showing them.

however I found out that this is a bugzilla issue.

I will check the patch

and let you know

Federico

I will report the bug to the Bugzilla community.

Hi Hossam,
the code seems well written.
I have some small concerns:
1) uan-application-example.cc 
  as Tom taught me :) it is always best to document:
 -  What is the point of the example?  What does this example intend to do?

 - What user-observable output (files left behind, messages to the terminal)
does the example produce?

2) uan-application-helper class. I'd like to change the function name to from InstallPriv to DoInstall which is more in line with NS3 names. What do you think?

3) uan-application.h
+/**
+ * \ingroup uan
+ * \brief a simple application to be used in UAN networks
+ */
please expand the description:
- what does it really do? (i.e. uses packet socket)
- what kind of addresses uses? (i.e. uses UANAddress (i.e. MAC address).
etc...

4) Is this application easily extendable?
Let me explain: currently the app is using MAC addresses as destination. Suppose now that you would want to use this an bigger network that supports IP addresses and custom routing algorithm based on the IP address. (no TCP involved, just UDP, and suppose that the MAC knows how to convert from IP to MAC/UANAddress)
Would it be easy to extend the class and implement this functionality or a totally brand new app would be needed?

I will update the code according to your recommendations 1, 2, and 3

Regarding 4:

UanApplication represents the layer 2 LLC API in the OSI model. So if you want to send an IP packet, you need to pass it (including the IP header) in the buffer parameter of the Send function. Now, if you need the layer 3 and 4 services that a networking stack provides (e.g. routing, UDP port multiplexing), you need to handle it in your code, that will sit on top of UanApplication and use its services.

What we really need is a simple networking stack for UAN. If you are keen, let's start a discussion about building a one.

Hi Hossam,
thanks for the reply

4) from my perspective the APP layer should always sit on top of the OSI layer and should address the node with its networking address.
A simple Networking stack would be similar to the one that we use in DESERT underwater, a NS-miracle framework that we developed at the university of Padua
http://nautilus.dei.unipd.it/desert-underwater

|__UAN APP__|  <=== traffic generator, bound to [dest_IP:dest_port] it could be UanApplication
|____UDP____|  <=== multiplexer, that forwards incoming traffic to the relevant APP (in NS-miracle it's easy to have N-apps sitting on top of a UDP module
|IP ROUTING_|  <=== optional layer, 
|__IP INT___|  <=== simple interface, it adds the source IP addr of outgoing packets and it discards incoming 
|__LLC/ARP__| <==== at the simplest level, it should simply map IP to UanAddress and viceversa
|____MAC____| <=== UAN MAC protocol, 
|____PHY____| <=== UAN or PHY protocol

|__CHANNEL__| <=== UAN or WOSS channel/propag etc...

please be aware that more elegant/sophisticated protocols could merge

|ROUTING| 
|IP_INT |
|LLC/ARP|
| MAC   | 

into one single layer. (again see DESERT routing/dissemination protocols)

so in my opinion UanApp should be also ready for this kind of architecture

the previous discussion brings up a new point

5) why a UAN user should prefer UANApplication instead of using a raw packet socket generator as done in the MAC UAN examples?
Is it only a matter of ease of use or is there any new feature added?
What I mean is that the raw packet socket generator does not bound the user to use UanAddress, and it can be easily used as traffic generator also in case of an hypothetical UAN networking stack.

Created attachment 2433 [details]
uan-ip-support

Hi Federico,

Thanks for the follow-up. 

I've renamed UanApplication to UanRawApplication, and introduced a new UDP-based UanApplication that runs on the NS-3 IP stack.
To be able to run the NS-3 IP stack on UAN, I needed to extend the type field of the UanHeaderCommon to 2 bytes. The protocolNumber parameter in the Enqueue function of the UAN MAC is used as a layer 3 protocol number, instead of being used for the Digital modulation type. A separate function to set the digital modulation type is introduced. UanNetDevice NeedsArp is changed to true.

The Aloha MAC and CW MAC support running IP while maintaining backwards compatibility with the existing setup. The RC MAC doesn't support running IP currently.

The UanRawApplication is a wrapper for the PacketSocket, and it just provides a more convenient way to send data over UAN.

Please see the attached patch.

Hi Hossam,
I'm sorry but I'm not following your latest patch.
1) modified functions have no doxygen updated so it's hard to understand the patch.
2) why are you introducing a new protocol number in the UAN header? Why can't you just use the standard NS3 IP header or common header (provided that it exists)
3) why do you need to bind a tx mode to the MAC?
 MAC and PHY should be independent. MAC should not care of the PHY tx mode.
 why can't the PHY add the modulation byte by itself?
4) why do you need to explicitly pass the network protocol number in the forwardUp from mac to uandevice?
IHMO MAC should not care about this at all. Its job should be to send a packet to the destination UanAddress, and to receive incoming packet with correct source UanAddress.
If you need the protocol number, then it should be the UanNetDevice that should extract this info from the IP header before passing it up in the m_forwardUp callback.
Is this a common a practice in the other models?

bottom line, I suggest to:
a) reduce the final to the first patch + requested changes (i.e. UanApplication to UanRawApplication, DoInstall() etc.. + documentation expanded).
b) move the IP modification to a separate patch or even ticket, since I expect a lot of work / discussion before committing it.

I agree. I will open a new ticket for this (IPv4 support on UAN). However, I’m just addressing your concerns.

2) I’m not introducing a new protocol field, I’m reusing the existing type field, and expanding it to 2 bytes. I think it should be renamed to protocolNumber, and updated in the docs.
The expansion is needed to fit the layer 3 protocol number (IPv4: 0x0800, ARP:0x0806). This field is equivalent to the EtherType field in the Ethernet frame. The other solution to avoid this expansion is to get the MAC layer to map this two-bytes values to a one-byte value in both the Rx and Tx, which is a non-standard approach.

4) This protocol number need to be passed to UanNetDevice, and then up, so the packet can be forwarded to the proper layer 3 protocol handler (e.g. IPv4 vs ARP). This is the job of the MAC layer, not the NetDevice (the layer 2 header is stripped by the MAC layer before passing it to the NetDevice). See the ReceiveCallback in net-device.h:

typedef Callback< bool, Ptr<NetDevice>, Ptr<const Packet>, uint16_t, const Address & > ReceiveCallback;

So UanNetDevice is not following the standard.

3) Passing the Tx mode to the MAC layer is the existing setup, which makes no sense. I just wanted to maintain it. Actually, in the current setup the Tx mode is passed initially by the PacketSocket, and down all the way to Physical layer. This is a non-standard approach, and it misuses a field that is supposed to be used for the protocol number. Do you think Tx mode should be eliminated?

In summary, once the UAN implementation is in-line with the NS-3 standards, running the NS-3 IP stack on top of it should be straightforward.

Hi Hossam,
thanks for the clarification.
as agreed let's split up the patches.

regarding this:
"Do you think Tx mode should be eliminated?"
I'll think about a possible refactory and let you know as soon as possible.

thanks again

Federico

refactory that should be discussed in the IP stack newest ticket :)

Created attachment 2434 [details]
uan-raw-application.patch

Hi Federico,

As agreed, this is a separate patch for uan-raw-application.

Let me know if any changes are required.

Hi Hossam,
thanks for the new patch.
I believe that this is ok and should be integrated.
Thanks again for your work.

(In reply to Hossam Khader from comment #13)
> Created attachment 2434 [details]
> uan-raw-application.patch
> 
> Hi Federico,
> 
> As agreed, this is a separate patch for uan-raw-application.
> 
> Let me know if any changes are required.

Hossam, I have two questions:

1) the application seems to do pretty much what packet-socket-client.cc/packet-socket-server.cc do.  Can you document (in the class doxygen for your new application) what distinguishes it from the generic packet socket applications?  In other words, document the use case(s) for which UanRawApplication is preferred over a generic packet socket application.

2) why does the application create a new socket for each call to Send()?  Why not try to use/maintain one socket?

Created attachment 2453 [details]
uan-raw-application

Hi Federico,

2) I consolidated both the send and the receive to use m_socket.

1) I updated the docs. 

Definitely, the user can use the PacketSocket client/server, but will need both of them to be installed in the node.

If you believe there is no need to merge this patch, then let's close the ticket, and focus on running standard applications using IPoUAN.

Hi Hossam,
thanks for the reply.
Please keep in mind that the previous request came from Tom, so let's wait for his final say on this topic.

I have some ideas on the IP stack too, I'll post them in the related ticket asap.

Federico

Hossam, I lean against merging this latest patch.  You were saying earlier that "The UanRawApplication is a wrapper for the PacketSocket, and it just provides a more convenient way to send data over UAN."

For instance,

+void
+UanRawApplication::Send (Address dst, const Buffer dataBuffer)

vs. existing methods

int
PacketSocket::SendTo (Ptr<Packet> p, uint32_t flags, const Address &address)

and

int 
Socket::SendTo (const uint8_t* buf, uint32_t size, uint32_t flags, const Address &address);


The app itself does not generate data; it relies on the example program (here generating an energy report at random times) to actually send data.

Why do we need to add this wrapper with slightly different API?  We already have existing variants taking a Ptr<Packet> and a char pointer.

I think that you identified a need in the UAN module to provide a simple example of how to get UAN nodes to exchange data directly over the UanNetDevice, but I'd suggest that if you want to add an application, make it an application in the usual sense (e.g. an EnergyReportingApplication that uses existing socket mechanisms).  If you have a problem using something like PacketSocket because it is not compatible and we cannot patch UanNetDevice to make it compatible, then I'd suggest rather that you make the minimal change to PacketSocket necessary (e.g. a subclass of PacketSocket), or simply hook directly to the UanNetDevice API if you do not want socket semantics.

The other thing you could try to do here if your EnergyReportingApplication is not intended to be a generic UAN app going forward (i.e. is just a toy example) is just name your program 'uan-energy-reporting-example.cc' and put everything you need into that example program (and make use of PacketSocketHelper instead if appropriate).

Hi Tom,

Thanks for your feedback. I've successfully patched UAN module, to support running IPv4, IPv6, 6lowpan, and the existing raw payload.

Running IPv4 requires the tuning of the ARP timers, which we already have in place.

Running IPv6/6lowpan requires the tuning of the ICMPv6 timers, which currently we don't have in place (and RFC6775 is not implemented yet in the code)

I would love to write multiple examples to demonstrate the use of all above options, once we are settled with the layer 3 issues.

I will also work on a generic EnergyReportingApplication.

Please let me know what do you think (See bug 2413 for more info).