Bug 2413

Hi Hossam,
I've thouroughly re-checked the UAN code and I've come to the following conclusion:
1) application: should be revised as per Tom's request, see other ticket.
2) modifications done to interface MAC - device are OK.
3) MAC - PHY interface is OK the way it is, the MAC should use SendPacket with mode number instead of creating a Tx mode and using it. We need to mantain this layer of indirection between the two classes. I.e. the MAC need to know that index "i" is associated to a specific tx charcteristic, (faster, safer etc..) instead of directly creating the tx mode it self.

In order to mantain current MAC / PHY interface I would do the following
3.a) add a uint32_t m_modeNum in UanMac class, and add set get method in this way the children MAC classes would inherit this method without code duplication. also allow the user to change this via object configuration.
3.b) the UanMacHeader packettype should keep the way it is. It is used by the MAC peer entities to tag control packets like RTS, CTS, ACK etc..
3.c) therefore you need to add a new uint16_t member to UanMacHeader class, in order to store the packet's network protocol number. along with new set/get
3.d) the current MAC upon sending the packet, would: save the protocolNumber into the header, and send down the Packet with m_modeNum. 
RC MAC should update m_modeNum for its dynamic configuration instead of using the old private variable, which should be removed.
3.e) on packet reception, MAC should extract the network procotol configuration from UanMacHeader and pass it up via forward up callback along with the packet and the address.

let me know if anything is not clear, or if you have any doubts

Federico

So the UanHeaderCommon format will be:

 0               7 8              15 
+-----------------+-----------------+ 
|   Source UAN    | Destination UAN | 
|     Address     |     Address     | 
+-----------------+-----------------+ 
|   packetType	  |                 | 
|                 |                 |    
+-----------------+-----------------+ 
|    	   protocolNumber           | 
|                 |                 |
+-----------------+-----------------+

By the way, we can combine the packetType and protocolNumber fields. There should be no overlap in the values.

If >= 2048 then it indicates a protocolNumber, else packetType, and the format will be

 0               7 8              15 
+-----------------+-----------------+ 
|   Source UAN    | Destination UAN | 
|     Address     |     Address     | 
+-----------------+-----------------+ 
|     packetType/protocolNumber     | 
|                                   |    
+-----------------+-----------------+

you will probably need both.
If you want to send data, the you would need to set the packetType to data and the protocolType to the one passed from the upperLayer,
unless you would put the following restrictions:
- CTRL packet cannot carry any data 

==> 

if >= 2048 the packet is MAC data 
else the packet is control and the packetType should be checked.

this also implies that an advanced MAC cannot perform any advanced piggyback (i.e. send ACK + DATA or other CTRL + DATA). this is quite unusual of course but it would be excluded by this solution.

furthermore you would need to modify also the MAC RC in order to make this work.

what about this?



+-----------------+-----------------+ 
|   Source UAN    | Destination UAN | 
|     Address     |     Address     | 
+-----------------+-----------------+ 
|          protocolNumber           | 
|                 |                 |    
+-----------------+-----------------+ 
| packetType      |                 | 
|                 |                 |
+-----------------+-----------------+

are those extra 8 bits a real issue?

are you going to send these packets over a real modem?

real FW would use a #pragma pack anyway in order to send the exact number of bytes over the link. 
is this supported by NS3?

I agree, let's go for a 5-byte UanHeader.

Regarding, the RC MAC scheme, it only supports one-way communication from the node to the gateway, and as a result ARP will no work, and the solution will be to send the packets to 255.255.255.255 which will be translated to a broadcast L2 address without ARP. Do you think we need to address this limitation?

Does ARP support static IP/MAC address resolution?
The correct way to make RC MAC work would be to manually configure the ARP of the nodes with:
- sink node: MAC to IP for each of the transmitter
- transmitter node: MAC to IP for the sink node.

(In reply to Federico Guerra from comment #5)
> Does ARP support static IP/MAC address resolution?
> The correct way to make RC MAC work would be to manually configure the ARP
> of the nodes with:
> - sink node: MAC to IP for each of the transmitter
> - transmitter node: MAC to IP for the sink node.

Yes, it does.

However, rather than modifying the UAN MAC headers to comply with IP, modify them to be logical and useful. If they'll be "usable" by IP or of they'll require a shim adaptation layer (like 6LoWPAN) is another point.
Given the length of the UAN packets, you'll probably need a shim layer anyway.

(In reply to Tommaso Pecorella from comment #6)

> However, rather than modifying the UAN MAC headers to comply with IP, modify
> them to be logical and useful. If they'll be "usable" by IP or of they'll
> require a shim adaptation layer (like 6LoWPAN) is another point.
> Given the length of the UAN packets, you'll probably need a shim layer
> anyway.

Hi Tommaso, thanks for the suggestion.
The issue regarding the specific MAC RC and ARP came up only because the sink node is not yet able to transmit data by design (i.e. it is designed only to receive data).
Static ARP should do the trick.

Regarding the adapt. layer:
A first example can be found here
http://www.eng.buffalo.edu/wnesl/papers/Yifan_WUWNet2013.pdf

it is responsible for header compression, packet fragmentation,
mesh routing and broadcast emulation

- header compression, can be easily emulated by shrinking the packets at the app layer, or really performed at the adapt layer.
- fragmentation, same as above
- routing: the adapt layer has the option to "route over", i.e. pass the packet to the network layer and let it perform its routing algorithm, or to "mesh under", i.e. to route from one node to the border router without the help of the IP layer.
- L3 broadcast: nodes serve as router proxies to pass router information to terminals. should this be done by the adapt layer or can we use the standard NS3 network layer?


Critical issues:

ARP in IPv4 (quoted from the paper)
IPv4 uses ARP for mapping a link layer address to an IP
address. ARP is designed for broadcast within a single IPhop,
and is not to be routed inter-network. Since broadcast
can involve excessive overhead, ARP is unsuited for underwater
wireless networks.
One of the key points of mesh routing is to create virtual
links between the border router and each node. Like typical
WiFi networks, each node only regards the router as its
neighbor and ignores the existence of the other nodes. Each
node, if it knows router information, serves as a router proxy
and can send ARP packets on behalf of the border router.
Other nodes that do not have router information will only
keep silent.
The ARP procedure is illustrated in Fig. 9. Alice sends
an ARP request. Bob does not know where the border router
is, so he keeps silent. Charlie, as a router proxy, replies with
an ARP reply on behalf of the border router. Thus, from the
network layer perspective, the ARP reply is from the border
router and Charlie is not a neighbor of Alice.


NDP in IPv6
generation of Interface identifier from MAC address cannot be performed since UAN Mac address is 8 bit number instead of the required 48 bits.


Bottom line:
1) all in all a minimum change in the UAN architecture (i.e. packet header) 
could be the first step to enable the TCP/IP stack, with some caveats of course.
2) Adapt layer could be useful to implement the feature as presented in the paper and could be later implemented as an enhancement.


What do you all think?

thanks

(In reply to Federico Guerra from comment #7)
> (In reply to Tommaso Pecorella from comment #6)
> 
> > However, rather than modifying the UAN MAC headers to comply with IP, modify
> > them to be logical and useful. If they'll be "usable" by IP or of they'll
> > require a shim adaptation layer (like 6LoWPAN) is another point.
> > Given the length of the UAN packets, you'll probably need a shim layer
> > anyway.
> 
> Hi Tommaso, thanks for the suggestion.
> The issue regarding the specific MAC RC and ARP came up only because the
> sink node is not yet able to transmit data by design (i.e. it is designed
> only to receive data).
> Static ARP should do the trick.
> 
> Regarding the adapt. layer:
> A first example can be found here
> http://www.eng.buffalo.edu/wnesl/papers/Yifan_WUWNet2013.pdf
> 
> it is responsible for header compression, packet fragmentation,
> mesh routing and broadcast emulation
> 
> [...]
> 
> What do you all think?

Do you want a simple statement ?
It's 6LoWPAN with minor variations (some of them questionable too).

About the NDP problem (i.e., to generate the address from the MAC), it's just a matter of deciding a scheme to expand an 8-bit address to an EUI64. Easy.

The paper is referring to the "old" RFC 4494 scheme, while 6282 is way more efficient.

There's no need to support dual stack... we can save one bit (or use it for more important stuff). We can assume that Underwater networks are homogeneous, isn't it ?

Mesh-under or route-over is debatable. I'm not convinced by the author's claims, but I don't have any counter proof.

I can be wrong, but there's one point that most people forgets. Most Internet mechanisms assumes silently or explicitly that there is a *bidirectional* channel between all the nodes.
Bidirectional doesn't means that the channel is symmetric or that packets have to use the same path. Just that both nodes can send and receive packets from the other one.
As an example, in most routing protocols it's stated that a node can use another one as a router only after checking the IP reachability (i.e., a PING request / reply).
This "little issue" should be considered, at least in the to-do list.

This was the foreword.

Now, about how to proceed, one can either include the compression system into the NetDevice or make it external (like 6LoWPAN). The result is more or less the same, but having it external is more flexible.
One could even simply try to use 6LoWPAN, it would need just a little modification to take into account 8-bit headers.

Cheers !

>Now, about how to proceed, one can either include the compression system into >the NetDevice or make it external (like 6LoWPAN). The result is more or less >the same, but having it external is more flexible.
>One could even simply try to use 6LoWPAN, it would need just a little > modification to take into account 8-bit headers.

I agree that whatever it will be developed should be as external/addon.
I'd like to have both the original and the future netdevice to coexist.

I have no experience with the NS3's 6LoWPAN model, I guess I'll check it out then :)

Hi Tommaso,

I'm able to run IPv4, IPv6, 6lowpan on top of UAN. However, I need to change the ICMPv6 timers (i.e. Icmpv6L4Protocol::RETRANS_TIMER)

What is the best solution to do this?

(In reply to Hossam Khader from comment #10)
> Hi Tommaso,
> 
> I'm able to run IPv4, IPv6, 6lowpan on top of UAN. However, I need to change
> the ICMPv6 timers (i.e. Icmpv6L4Protocol::RETRANS_TIMER)
> 
> What is the best solution to do this?

There's a nice trick... subclass Icmpv6L4Protocol. Then you can use Node::RegisterProtocolHandler to register your new protocol for the device you want. It will work on that device and all the others will be unaffected (nice if you need to do it on a gateway).

You might want to take a look at the 6loWPAN-ND rfc tho. I still need to complete its code... I don't have much time recently.

What about using the attribute system or having setter methods (similiar to ARP)?
I'm done with testing UAN with raw, IPv4, IPv6, 6lowpan, and I need a solution for the ICMPv6 ND before the patch can be merged.

Hossam, could you please provide status and open issues left on this patch?  There has been a lot of discussion in the tracker but the patch posted is still the original one, I believe.

Created attachment 2508 [details]
Add IPv4/6 support to UAN Module

The attached patch adds IPv4/IPv6 support to UAN Module.


ARP timers need to be tuned, for successful transmissions.

For examples:

(*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache ()->SetDeadTimeout (Hours (24));
      (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache ()->SetAliveTimeout (Hours (12));
      (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache ()->SetWaitReplyTimeout (Seconds (10));


ICMPv6 timers need to be tuned also, but we don't have this capability.

(In reply to Hossam Khader from comment #14)
> Created attachment 2508 [details]
> Add IPv4/6 support to UAN Module
> 
> The attached patch adds IPv4/IPv6 support to UAN Module.
> 
> 
> ARP timers need to be tuned, for successful transmissions.
> 
> For examples:
> 
> (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache
> ()->SetDeadTimeout (Hours (24));
>       (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache
> ()->SetAliveTimeout (Hours (12));
>       (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache
> ()->SetWaitReplyTimeout (Seconds (10));
> 
> 
> ICMPv6 timers need to be tuned also, but we don't have this capability.

Hi Hossam,
sorry for the late reply, I was on summer vacation :)

I need some clarifications on the current patch:
- the common header's packet type is used to carry the packet type info (IP etc...). it has been increased to 2 bytes to support this correct?
- however packettype is also used by RC MAC in order to encode the RTS/CTS/DATA type... 
- if I correctly understood your patch then MAC-RC is currently not working with IP packets, since it is discarding the protocol number and overriding it with its MAC type. 
If this is correct, then I think you could simply make it work by:
- RTS / CTS, leave them as they are
- DATA, we should find a proper value for DATA enum, in order to safely OR its number to the upper layer protocol number and remove it on correct DATA reception, or even better split the packet type and protocol number functionalities into two as we discussed early (i.e. 1 byte for packet type, MAC dependant, 2 byes for prtocol number). I would prefer this solution. 

Let me know what do you think

another thing.
This patch should also preserve some retrocompatibility.
I expect the old code to continue working, i.e. sending raw packets over MAC.
This would still be useful in small network scenario where IP is not needed.
Is this still possible with your patch?
Is the arp required = true causing any issue?

thanks again
Federico

(In reply to Federico Guerra from comment #15)
> (In reply to Hossam Khader from comment #14)
> > Created attachment 2508 [details]
> > Add IPv4/6 support to UAN Module
> > 
> > The attached patch adds IPv4/IPv6 support to UAN Module.
> > 
> > 
> > ARP timers need to be tuned, for successful transmissions.
> > 
> > For examples:
> > 
> > (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache
> > ()->SetDeadTimeout (Hours (24));
> >       (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache
> > ()->SetAliveTimeout (Hours (12));
> >       (*node)->GetObject<Ipv4L3Protocol> ()->GetInterface (1)->GetArpCache
> > ()->SetWaitReplyTimeout (Seconds (10));
> > 
> > 
> > ICMPv6 timers need to be tuned also, but we don't have this capability.
> 
> Hi Hossam,
> sorry for the late reply, I was on summer vacation :)
> 
> I need some clarifications on the current patch:
> - the common header's packet type is used to carry the packet type info (IP
> etc...). it has been increased to 2 bytes to support this correct?
> - however packettype is also used by RC MAC in order to encode the
> RTS/CTS/DATA type... 
> - if I correctly understood your patch then MAC-RC is currently not working
> with IP packets, since it is discarding the protocol number and overriding
> it with its MAC type. 
> If this is correct, then I think you could simply make it work by:
> - RTS / CTS, leave them as they are
> - DATA, we should find a proper value for DATA enum, in order to safely OR
> its number to the upper layer protocol number and remove it on correct DATA
> reception, or even better split the packet type and protocol number
> functionalities into two as we discussed early (i.e. 1 byte for packet type,
> MAC dependant, 2 byes for prtocol number). I would prefer this solution. 
> 
> Let me know what do you think

Actually mac-rc could assume, as said earlier if packettype < 2048
==> control , else data.
This should be included in the final patch.

by the way, 2048 should be used as a define or even better as a method to dynamically get this number from some ns3 class (I don't know if this is possible though)

Welcome back Federico,

The values should be

0x0800 IPv4
0x0806 ARP
0x86DD IPv6

They don't need to be encoded anywhere in the UAN code.

The existing raw communications over UAN should continue to function.

I will add one more field to the UanHeaderCommon for MAC-RC packet type.

Without the ARP and ICMPv6 timers tuning no communications can take place due to collisions.

(In reply to Hossam Khader from comment #18)
> Welcome back Federico,
> 
> The values should be
> 
> 0x0800 IPv4
> 0x0806 ARP
> 0x86DD IPv6
> 
> They don't need to be encoded anywhere in the UAN code.
> 
> The existing raw communications over UAN should continue to function.
> 
> I will add one more field to the UanHeaderCommon for MAC-RC packet type.
> 
> Without the ARP and ICMPv6 timers tuning no communications can take place
> due to collisions.

Hi Hossam, 
OK. 
Actually the packet type (1 byte)  would be useful also for other future MAC protocols which would use control packets (RTS CTS ACK Sync etc...)

Regarding ARP I was referring to raw communication scenario, i. e. packet over MAC without IP. 
This is actually the main scenario of many real UW deployment. When you have a small number of nodes you don't really need IP stack especially if they can hear each other. Actually there are also UW protocols that do some primitive routing without using the network stack,  just the MAC address. Collision avoidance is not a problem in this scenario since it's MAC duty to avoid collision over the channel, Mac RC is an example. 
Check university of Padua's Desert UW framework for ns2/ns-miracle in order to have a good number of MAC / Routing / dissemination protocols for UW networks

Federico

The ARP and ICMPv6 timers tuning is required also because of the high delay in UAN

For example, the ARP WaitReplyTimeout default value is 1 second, which is not adequate for UAN.

(In reply to Hossam Khader from comment #20)
> The ARP and ICMPv6 timers tuning is required also because of the high delay
> in UAN
> 
> For example, the ARP WaitReplyTimeout default value is 1 second, which is
> not adequate for UAN.

yes of course, network timers has to be adjusted. I was only saying that channel collision is usually a L2 prerogative.

So our problem now is the ICMPv6 timers. They are not configurable.

(In reply to Hossam Khader from comment #22)
> So our problem now is the ICMPv6 timers. They are not configurable.

understood, wating for Tom/Tommaso input then

Hi,

if you're referring to the ones in icmpv6-l4-protocol.cc, e.g.:
const uint8_t Icmpv6L4Protocol::MAX_INITIAL_RTR_ADVERT_INTERVAL = 16;
const uint8_t Icmpv6L4Protocol::MAX_INITIAL_RTR_ADVERTISEMENTS = 3;
...

then I'm totally for moving all of them to appropriate attributes.
As a side note, some of them are not even used.

If this is enough, I'd say to prepare another patch (and another enhancement "bug").

T.


(In reply to Federico Guerra from comment #23)
> (In reply to Hossam Khader from comment #22)
> > So our problem now is the ICMPv6 timers. They are not configurable.
> 
> understood, wating for Tom/Tommaso input then

Hi Tommaso,

I created bug 2482 for this.

(In reply to Hossam Khader from comment #25)
> Hi Tommaso,
> 
> I created bug 2482 for this.

Hi Hossam, 
do you have any new patch to submit?
If I remember correctly we agreed to change the header to 5 bytes?
thanks

Federico

Created attachment 2699 [details]
Add support for IPv4/6 over UAN (5 bytes UAN header)

Hi Hossam,
there is still one issue remaining with this patch.

And it's the SendPacket.

the current Interface is

  /**
   * Send a packet using a specific transmission mode.
   *
   * \param pkt  Packet to transmit.
   * \param modeNum  Index of mode in SupportedModes list to use for transmission.
   */
  virtual void SendPacket (Ptr<Packet> pkt, uint32_t modeNum) = 0;


So modeNum is the index of the supported mode.

With your patch instead you are using a UanTxMode, and then send its modulation number, which is wrong, since it phy will interpret this as its m_mode index.

therefore:

UanTxMode::ModulationType GetTxMode ();
void SetTxMode (UanTxMode::ModulationType txMode);

should be changed to 

uint32_t GetTxModeIndex ();
void SetTxModeIndex (uint32_t txMode);

and m_txModeIndex should be a uint32_t.

Created attachment 2716 [details]
Add support for IPv4/6 over UAN (5 bytes UAN header)

(In reply to Hossam Khader from comment #29)
> Created attachment 2716 [details]
> Add support for IPv4/6 over UAN (5 bytes UAN header)

HI Hossam,
sorry to bother, don't get mad at me... but:
1) I probably skipped too many steps, but what I meant was that 

m_txModeIndex and the two functions SetTxModeIndex GetTxModeIndex are probably best to be put in the parent class, so that you don't have to redefine them again and again in the child classes.

2) furthermore m_txModeIndex init is still wrong, it should be zero, and not related to the (UanTxMode::PSK) enum

do you agree?

Federico

Created attachment 2719 [details]
Add support for IPv4/6 over UAN (5 bytes UAN header)

Hi Hossam,
thanks for the latest patch. I'd say good job!

Tom, Tommaso, the latest patch satisfies all the requirements, while not breaking the existing architecture.
Let me know if you have any other issue/doubt/question before integrating this patch.

Federico

Hi Hossam,
I was re-checking the patch, the only thing that it is bugging me is that the header is now 5 bytes instead of the previous 3 bytes.
Now 40 bits is not that much over higher order modulations like qpsk or bpsk (4800 or 2000 bps) while it adds too much overhead for FH-FSK of 80bps

In this scenario we jump from a 24/80 = 30% overhead to 40/80 = 50% overhead and it might be a little too much, what do you think?

Is there any way we can compress the 

uint16_t m_protocolNumber; 

to uint8_t m_protocolNumber?

So that overhead would be 32/80 = 40%

Can we map the 16 bits to 8 bits with an enum or something? I don't think we need to support all the possible numbers right? Which by the way seems to be only 255 (1 byte) https://en.wikipedia.org/wiki/List_of_IP_protocol_numbers

Let me know your comments

The problem is that we need to support at least the below 3 ethertype values:

0x0800 	Internet Protocol version 4 (IPv4)
0x0806 	Address Resolution Protocol (ARP)
0x86DD 	Internet Protocol Version 6 (IPv6)

We definitely can get the UanNetDevice to do 16 to 8 bits mapping.

(In reply to Hossam Khader from comment #34)
> The problem is that we need to support at least the below 3 ethertype values:
> 
> 0x0800 	Internet Protocol version 4 (IPv4)
> 0x0806 	Address Resolution Protocol (ARP)
> 0x86DD 	Internet Protocol Version 6 (IPv6)
> 
> We definitely can get the UanNetDevice to do 16 to 8 bits mapping

Yep I agree that we should do the mapping and support only the required protocols (IPv4, ARP, IPv6, IMCPv4? ICMPv6? any other protocol? )

This would be great.

Further reduction, i.e. make uan common header's type and protocol number into a 8 bit bitfield

 uint8_t m_type;     //!< The type field.

into something like

struct UanProtocolBits
{
     uint8_t  m_type : 4;
     uint8_t  m_protocolNumber : 4; 
};

since 2^4 - 1 number of packet types and protocol number should be enough for the most practical user case. 
I also suggest to create a new Header for this use case, i.e. UanHeaderCommonCompressed or something like that.

Now, regarding the level where these compression should be done, I see two options:
1) 16 bit to 8 bit protocol number reduction at UanNetDevice in both directions. Further compression/decompression into bit field done at MAC level in both direction. the Compressed header should be used in this scenario.
2) both protocol number and bitfield compression/decompression should be done at MAC level.

In any case, In order not to copy / paste the header compression / decompression in each MAC, this functionality should be added to the parent class.


What do you think?

Created attachment 2806 [details]
Add IPv4/IPv6 support to the UAN module

The attached patch uses one byte for the protocol number/packet type (4 bits each)

Please let me know what is the desired way to do the mapping between the protocolNumber and 4 bits header field.

I'm currently doing it in UanHeaderCommon::GetProtocolNumber and UanHeaderCommon::SetProtocolNumber

Hi Hossam,
thanks for the hard work, the patch seems good to me!
The only thing left to do IHMO is to double check the doxygen part, 
For example I see that 
 uint32_t GetTxModeIndex ();
131	  void SetTxModeIndex (uint32_t txModeIndex);

are without doxygen comment.

The code on the other hand looks great.

thanks again

I will work on the Doxygen comments.

I just need your confirmation regarding the best way to do the mapping between the protocolNumber and 4 bits header field.

Our next steps will be to Implement RFC6775: Neighbor Discovery Optimization for IPv6 over Low-Power Wireless Personal Area Networks.

(In reply to Hossam Khader from comment #38)
> I will work on the Doxygen comments.
> 
> I just need your confirmation regarding the best way to do the mapping
> between the protocolNumber and 4 bits header field.
> 
> Our next steps will be to Implement RFC6775: Neighbor Discovery Optimization
> for IPv6 over Low-Power Wireless Personal Area Networks.

mapping seems fine to me, and putting it the header class seems appropriate.

Ok, I know for sure you're going to kill me, and you're right.

UanAddress::GetAsInt - Unnecessary. Provide the following instead:
- UanAddress::Serialize
- UanAddress::Deserialize
- UanAddress::GetSerializedSize

UanAddress::GetAsMacAddress - Misleading. Why in the first place is it needed?

I'd move uan-address.cc to network/utils. All the addresses are there (debatable in the UAN case, but it could be needed by ARP).

Moreover, I need a clarification. ARP is not a nice protocol, it clamps the address length to fixed sizes and there's no way to understand what was the sender address length from the ARP packet alone.
The good news is that you can always assume that the sender has your own MAC address length. It's not "right" to have two completely incompatible technologies on the same network.
I see you changed one line in the ARP protocol, but it's only in the Serialization part. What about the deserialization and address storing?


UanHeaderCommon::SetProtocolNumber - don't use numerical values, use the corresponding enums.
As an example, 0x0800 is Ipv4L3Protocol::PROT_NUMBER

UanHeaderCommon::SetProtocolNumber - add an NS_ASSERT_MSG in case the protocol is not supported.

UanAddress::GetAsInt - Unnecessary. Provide the following instead:
- UanAddress::Serialize
- UanAddress::Deserialize
- UanAddress::GetSerializedSize

Are we looking for something like:
uint8_t UanAddress::Serialize()
void UanAddress::Deserialize(uint8_t)
uint32_t UanAddress::GetSerializedSize()

or something else?


UanAddress::GetAsMacAddress - Misleading. Why in the first place is it needed?
Required for the IPv6 EUI-64 addressing. Returns the 1-byte UanAddress in a 6-bytes address format.


I'd move uan-address.cc to network/utils. All the addresses are there (debatable in the UAN case, but it could be needed by ARP).
Do we want the UanAddress class to inherit the Address class?


Moreover, I need a clarification. ARP is not a nice protocol, it clamps the address length to fixed sizes and there's no way to understand what was the sender address length from the ARP packet alone.
The good news is that you can always assume that the sender has your own MAC address length. It's not "right" to have two completely incompatible technologies on the same network.
I see you changed one line in the ARP protocol, but it's only in the Serialization part. What about the deserialization and address storing?

I just made one change to the NS_ASSERT by allowing a 1-byte hardware address. Just escaping the assert.

(In reply to Hossam Khader from comment #41)
> UanAddress::GetAsInt - Unnecessary. Provide the following instead:
> - UanAddress::Serialize
> - UanAddress::Deserialize
> - UanAddress::GetSerializedSize
> 
> Are we looking for something like:
> uint8_t UanAddress::Serialize()
> void UanAddress::Deserialize(uint8_t)
> uint32_t UanAddress::GetSerializedSize()

Same as EthernetHeader:
  virtual uint32_t GetSerializedSize (void) const;
  virtual void Serialize (Buffer::Iterator start) const;
  virtual uint32_t Deserialize (Buffer::Iterator start);


> or something else?
Tbh the Address class doesn't impose anything specific. It's the whole "GetAs" that I'm not comfortable with.

> UanAddress::GetAsMacAddress - Misleading. Why in the first place is it
> needed?
> Required for the IPv6 EUI-64 addressing. Returns the 1-byte UanAddress in a
> 6-bytes address format.

That's the problem. You don't need a double conversion, you need a direct one.
Check the functions Ipv6Address::MakeAutoconfiguredAddress(...) and Ipv6Address::MakeAutoconfiguredLinkLocalAddress (...).
You need to add 2 more of them.
For IPv4 addresses... well, there's no need to provide conversions.

> I'd move uan-address.cc to network/utils. All the addresses are there
> (debatable in the UAN case, but it could be needed by ARP).
> Do we want the UanAddress class to inherit the Address class?

Not really, it's a Mac1Address for real, and Mac48Address is not derived from anything.
It should be consistent with Mac??Address tho, as it's a... Mac address :)

> Moreover, I need a clarification. ARP is not a nice protocol, it clamps the
> address length to fixed sizes and there's no way to understand what was the
> sender address length from the ARP packet alone.
> The good news is that you can always assume that the sender has your own MAC
> address length. It's not "right" to have two completely incompatible
> technologies on the same network.
> I see you changed one line in the ARP protocol, but it's only in the
> Serialization part. What about the deserialization and address storing?
> 
> I just made one change to the NS_ASSERT by allowing a 1-byte hardware
> address. Just escaping the assert.

Ok. Just double check the NDISC part as well.

Created attachment 2933 [details]
Add IPv4/6 to UAN module

This patch implements the previous recommendations

@Tommaso I would like to have also your opinion on this patch if possible.
thanks to all,

Federico

Hi,

I'd suggest just some small changes.

1) UanAddress should be renamed Mac8Address and moved to network/utils. Otherwise, we'll have a hard dependency (for example) between sixlowpan and uan.
2) UanAddress::GetAsInt is not really a good name. GetAsU8int is more explicit. As a side note, it's a duplicate of CopyTo (uint8_t *pBuffer). I'd remove it in favor of CopyTo. Perhaps it's less "direct", but it's more in line with the other addresses use.
3) Some lines are not strictly adherent to the code style. Pass the patch through utils/check-style.py
4) The documentation is missing. We need an update to the module documentation, CHANGES.html, and RELEASE_NOTES.

Cheers,
T.


(In reply to Federico Guerra from comment #44)
> @Tommaso I would like to have also your opinion on this patch if possible.
> thanks to all,
> 
> Federico

Created attachment 2989 [details]
Add IPv4/6 support to UAN module

I renamed UanAddress to Mac8Address, and eliminated GetAsInt()
I fixed the coding style.

+1 for committing this, but the documentation is still missing.

T.

(In reply to Hossam Khader from comment #46)
> Created attachment 2989 [details]
> Add IPv4/6 support to UAN module
> 
> I renamed UanAddress to Mac8Address, and eliminated GetAsInt()
> I fixed the coding style.

Ok, 
thanks Tommaso.
I'll double check it again ASAP.

Federico

Hi Tommaso,

I'm planning to write an example for 6lowpan over UAN.
Appreciate your feedback on bug 2805 (RFC6775)

Regarding the documentation, not sure if we want to keep the AUV stuff in the UAN module documentation.

Hi Hossam,
I have two observations related to the code:
1) UanCommonHeader::setProcotolNumber does not allow a special value, that should mean "no transport used". This number could be for example 0, and it should be used in all of the UAN examples, which do not use any transport protocol at all. This is also implied by UanCommonHeader constructor, which instead allows any value. This is also implied by your MAC CW/RC CW code, where you set 0 for RTS/CTS.
bottom line: I would allow the protocol value = 0 which would means "no transport". I would also put a value check (NS ASSERT) in the constructor. (i.e allowed values: 0,1,2,3).
2) We have many patches related to visual studio warnings/errors currently open. bottom line, these are promoting c++ style static casting instead of the c-style ones. So if I understood it correctly, it would be better to change the (uint8_t) c-style casts that you are introducing, to the c++ style static_cast<uint8_t>() instead. Tommaso, please correct me if I'm wrong (i.e. if c-style casting is ok with visual studio for windows)

Regarding the AUV documentation, I'll leave that decision to Tommaso, since he already managed to refactor it in one of his latest papers.

Federico

Created attachment 2991 [details]
Add IPv4/6 support to UAN module

Hi Federico,

Attached is an updated patch

Hi Hossam,
the patch is fine IHMO.
thanks for the great work.

Tom, Tommaso, let me know how do you wish to proceed, regarding to integration and documentation + example missing. (resolved here or in a separate issue?)

Federico

Hi all,

we'd like to merge this patch in the next few days, if you're ok and you can do the very last bits.

If I do remember well, the missing things are an example and the documentation.
-> Please add them to this patch.

Moreover, I'll go through the code once more in the next days, but I'd like to ask one thing.
Is it possible to *disable* the added features ?
The use-case could be if someone would like to use the UAN raw packet, and squeeze the very last bits out of it.

Of course the coexistence of the two modes would be impossible, but that's ok. It's much like using a 6LoWPAN device and a "raw" LoWPAN devices together: they won't work.

Anyway, it would be nice to have an Attribute (true/false) to disable the extra header and its processing.

Would it be a nightmare to add it ?

thanks,

T.


(In reply to Federico Guerra from comment #52)
> Hi Hossam,
> the patch is fine IHMO.
> thanks for the great work.
> 
> Tom, Tommaso, let me know how do you wish to proceed, regarding to
> integration and documentation + example missing. (resolved here or in a
> separate issue?)
> 
> Federico

> Is it possible to *disable* the added features ?
> The use-case could be if someone would like to use the UAN raw packet, and 
> squeeze the very last bits out of it.

The patch has introduced some changes which are not back compatible.

At device level: UAN used the transport int to signal the MAC which tx mode to use for each packet sent. 

With this new patch the transport type is not changed to its correct meaning. 

If you do not want to use the transport, just set 0 here. 

This though still uses the new header which has extra bytes (2 If I remember correctly).

The Tx mode to use is now set via dedicated Set/get API, which is now constant for each packet, unless the MAC protocol does otherwise of course.

This can be easily changed by Hossam, by:
- using the Set/get Tx Api already implemented. 
  
a) If the user sets (-1) for example, then the lower layers are instructed to interpret the transport type really as a tx mode selector, and incidentally the old header can be used.

b) else, if the user sets a valid value with the APIs, then the transport type is interpreted as transport number and the new header will be used.

so basically the Api has a dual function, and probably its name should also be changed to match the new behavior.

what do you both think of this proposal? Hossam? Tommaso?

thanks

Created attachment 3056 [details]
uan-ip

The patch maintains backward compatibility, and allows for raw packets over UAN.
UAN MAC layer setting the TX mode doesn't make sense. We already discussed this and agreed on.
The UAN header is still 3 bytes.
I will open a new bug for the uan-examples, because I think it will be another long discussion.
Not sure about the module documentation. The only change is the reuse of the 3rd byte in the UanHeaderCommon.

Hi Hossam,
please check the latest attached patch, it has an extra file, namely 
/ipv6-address-helper.cc~

back to the discussion:
1) what does this latest patch change in respect to the previous one?

2) my bad I forgot that we agreed that the header remained 3 bytes.

3) so the only thing that is not back compatible is the fact that the protocol number was being used as a selector of the tx mode.

In order to restore this, we could simply use UanMac::SetTxModeIndex (uint32_t txModeIndex).

if is < 0 (or equal to a special negative number) then it would mean invalid ==> use old behavior, i.e. protocolNumber passed to the Enqueue should be used in m_phy->SendPacket(). 
if >= 0 then the protoclNumber behavior of this patch kicks in.

this way we would have full back compatible behavior. the header would be kept the new one since it is still 3 bytes.

4) regarding the current examples and tests, if we set m_txModeIndex factory value to the special invalid value then we won't need to change anything, am I missing something?

5) regarding documentation, I believe they meant they wanted to describe the new behavior/class changes in the UAN sphynx rst doc, and probably a new example that shows how to use the new feature.


Let me know your answer to each of the points above.

thanks

Federico

Created attachment 3059 [details]
uan-ip

The patch fixes a bug in the previous one. Namely, the assert in uan-header-common and MAC8Address handling in ipv6-address-helper.

I've added set/get TX mode to the UanNetDevice and updated the tests.
Passing the TX mode for every packet from the upper layers doesn't make sense.


Let's move the discussion about the examples and the module documentation to:
Bug 2882: Examples for running raw, IPv4, IPv6, 6lowpan over UAN

Hossam,
I'm perfectly aware that passing the tx mode from upper layers makes sense in only some very limited scenarios.
Nevertheless, my only concern here is backward compatibility.
If we change this behavior to the new one then we will need to cristal clear state that UAN users around the world will be forced to update all of their code and examples.
Which by the way, might be fine with me, but we need to make everybody aware of this, unless, we provide a way to mantain that backward compatibilty for some users that don't want to perform the mandatory code revision.

Federico

Changing the TX mode requires an out of band channel to signal the TX mode to the receivers for them to tune to the new TX mode, and requires time sync between the 2 nodes. So practically it doesn't work.

We already provided an API to do the same functionality.
Tommaso Backward compatibility was about sending raw packets.

Actually one thing to add, raw and IPv4/IPv6 can coexist on the same UAN network.

+1, go ahead and push.

Hold on, I found something weird in the patch...
Working on it.


(In reply to Tommaso Pecorella from comment #60)
> +1, go ahead and push.

Created attachment 3064 [details]
uan-ip w/o dependency from internet module

I just updated the module to avoid an explicit dependency on the internet module.

This meant re-defining the ARP, IPv4, and IPv6 protocol numbers in the UAN module. Sadly, this is unavoidable (and it's what we did in others modules as well).

Now it's a "feel free to push".

Can you please add a Doxyen note about where the IPv6 address format comes from (in MakeAutoconfiguredAddress (Mac8Address addr, Ipv6Address prefix))?  I think it is an RFC 2373 EUI-64 address whose last byte is the Mac8Address.

Otherwise, I think it can be pushed.

pushed in changeset:   13315:f5eefc5a4fd1