Bug 988

(In reply to comment #0)
> Mirko wrote:
> 
> i'd like take a look at the function MacRxMiddle::SequenceControlSmaller. The
> first question is: what this function does? 

I don't know. Let's put some doxygen documentation when we figure it out...


> I suppose that it is used to figure
> out if a packet is an "old" packet or not. Standard specifies that range of
> sequence number are splitted in two parts: one corresponds to packets that can
> be considered new and one to packets that can be considered old. 

You refer to section 9.10.3, right? But that's for block ack only, isn't it? 

> For simplicity
> we can consider a smaller range, for example, of 10 sequence numbers so with an
> initial sequence number equals to 7 suppose to have next situation:
> 
> 0  1  2  3  4  5  6  7  8  9
> *  *  |  |  |  |  |  *  *  *
> 
> * = new packet
> | = old packet
> 
> Now i used that function to discover if the packet number 2 is old or new in
> comparison to packet number 7. So i used that logic (modified accordingly) but
> the function returns the following results:
> 
> SequenceControlSmaller (7<<4, 2<<4) = true
> SequenceControlSmaller (2<<4, 7<<4) = false

can you explain why you use the "<<" operator in this case?

> 
> I think that the function should return always true because if 7 is an old
> packet 2 in a new one and vice versa. What do you think about?

well I think that the SequenceControlSmaller function was written with a purpose that was probably different than yours, so no surprise it does not do what you want.


> Second question: who uses this function?It seems that it never used...

it is used in src/devices/wifi/wifi-test.cc


> this
> function would be very useful to me... what do you think about moving it to
> qos-utils.h?

I think that in any case SequenceControlSmaller is in the wrong place. If you can use it as-is, I agree with putting it in a common place - what about a static WifiMacHeader::IsSequenceControlSmaller method?

If you need to modify the method, then I would move the existing SequenceControlSmaller method as-is to wifi-test.cc, and you could implement your method separately at the location that you find more appropriate.

(In reply to comment #1)
> (In reply to comment #0)
> > Mirko wrote:
> > 
> > i'd like take a look at the function MacRxMiddle::SequenceControlSmaller. The
> > first question is: what this function does? 
> 
> I don't know. Let's put some doxygen documentation when we figure it out...
> 

I'm waiting for a Mathieu's reply.
> 
> > I suppose that it is used to figure
> > out if a packet is an "old" packet or not. Standard specifies that range of
> > sequence number are splitted in two parts: one corresponds to packets that can
> > be considered new and one to packets that can be considered old. 
> 
> You refer to section 9.10.3, right? But that's for block ack only, isn't it? 

I think that this rule is always valid. Also for packets that aren't transmitted with a block ack mechanism.

> > For simplicity
> > we can consider a smaller range, for example, of 10 sequence numbers so with an
> > initial sequence number equals to 7 suppose to have next situation:
> > 
> > 0  1  2  3  4  5  6  7  8  9
> > *  *  |  |  |  |  |  *  *  *
> > 
> > * = new packet
> > | = old packet
> > 
> > Now i used that function to discover if the packet number 2 is old or new in
> > comparison to packet number 7. So i used that logic (modified accordingly) but
> > the function returns the following results:
> > 
> > SequenceControlSmaller (7<<4, 2<<4) = true
> > SequenceControlSmaller (2<<4, 7<<4) = false
> 
> can you explain why you use the "<<" operator in this case?

Because the function take two sequence Control fields as parameters and not two sequence Numbers.

> > 
> > I think that the function should return always true because if 7 is an old
> > packet 2 in a new one and vice versa. What do you think about?
> 
> well I think that the SequenceControlSmaller function was written with a
> purpose that was probably different than yours, so no surprise it does not do
> what you want.
> 

Maybe, but i'm not sure of this. However Mathieu will explain the role of the function.

> 
> > Second question: who uses this function?It seems that it never used...
> 
> it is used in src/devices/wifi/wifi-test.cc
> 

Yes but only for a testing purpose. Really this function is used nowhere.

> 
> > this
> > function would be very useful to me... what do you think about moving it to
> > qos-utils.h?
> 
> I think that in any case SequenceControlSmaller is in the wrong place. If you
> can use it as-is, I agree with putting it in a common place - what about a
> static WifiMacHeader::IsSequenceControlSmaller method?
> 
> If you need to modify the method, then I would move the existing
> SequenceControlSmaller method as-is to wifi-test.cc, and you could implement
> your method separately at the location that you find more appropriate.

My idea is to remove the method from MacRxMiddle and to move it in qos-utils.h. I think that is the right place for it. However i'd like to hear Mathieu's comments before.

(In reply to comment #0)
> we can consider a smaller range, for example, of 10 sequence numbers so with an
> initial sequence number equals to 7 suppose to have next situation:
> 
> 0  1  2  3  4  5  6  7  8  9
> *  *  |  |  |  |  |  *  *  *
> 
> * = new packet
> | = old packet
> 
> Now i used that function to discover if the packet number 2 is old or new in
> comparison to packet number 7. So i used that logic (modified accordingly) but
> the function returns the following results:
> 
> SequenceControlSmaller (7<<4, 2<<4) = true
> SequenceControlSmaller (2<<4, 7<<4) = false
> 
> I think that the function should return always true because if 7 is an old
> packet 2 in a new one and vice versa. What do you think about?

This function is right in saying that 7 <= 2 because 7 <= 2+10/2.

> Second question: who uses this function?It seems that it never used...this

It used to be used to assert that we don't receive out of order packets.

> function would be very useful to me... what do you think about moving it to
> qos-utils.h?

why not ?

(In reply to comment #3)
> (In reply to comment #0)
> > we can consider a smaller range, for example, of 10 sequence numbers so with an
> > initial sequence number equals to 7 suppose to have next situation:
> > 
> > 0  1  2  3  4  5  6  7  8  9
> > *  *  |  |  |  |  |  *  *  *
> > 
> > * = new packet
> > | = old packet
> > 
> > Now i used that function to discover if the packet number 2 is old or new in
> > comparison to packet number 7. So i used that logic (modified accordingly) but
> > the function returns the following results:
> > 
> > SequenceControlSmaller (7<<4, 2<<4) = true
> > SequenceControlSmaller (2<<4, 7<<4) = false
> > 
> > I think that the function should return always true because if 7 is an old
> > packet 2 in a new one and vice versa. What do you think about?
> 
> This function is right in saying that 7 <= 2 because 7 <= 2+10/2.

This is ok. But the function returns false when the first parameter is 2<<4 and the second one is 7<<4. Maybe the condition "if (absDelta < 2048)" should be changed to "if (absDelta <= 2048)". However i think that the following formula is perfect for us to discover if a packet with sequence number seqNumber is old in comparison to a packet with sequenceNumber startingSeq:

return ((seqNumber - startingSeq) + 4096) % 4096 >= 2048 

> 
> > Second question: who uses this function?It seems that it never used...this
> 
> It used to be used to assert that we don't receive out of order packets.

I found it only in wifi-test.cc.

> 
> > function would be very useful to me... what do you think about moving it to
> > qos-utils.h?
> 
> why not ?

Ok. What do you think about a function like the following?

bool
QosUtilsIsOldPacket (uint16_t startingSeq, uint16_t seqNumber)
{
  uint16_t distance = ((seqNumber - startingSeq) + 4096) % 4096;
  return distance >= 2048;
}

(In reply to comment #4)

> > This function is right in saying that 7 <= 2 because 7 <= 2+10/2.
> 
> This is ok. But the function returns false when the first parameter is 2<<4 and
> the second one is 7<<4. Maybe the condition "if (absDelta < 2048)" should be
> changed to "if (absDelta <= 2048)". However i think that the following formula

yes, this might make sense.

> is perfect for us to discover if a packet with sequence number seqNumber is old
> in comparison to a packet with sequenceNumber startingSeq:
> 
> return ((seqNumber - startingSeq) + 4096) % 4096 >= 2048 

If you can prove that this is equivalent to the previous version, modulo any bug that you have fixed, why not ? If I were you, I would write a couple of tests to check that it does what you want and I would document it thusly.

> Ok. What do you think about a function like the following?
> 
> bool
> QosUtilsIsOldPacket (uint16_t startingSeq, uint16_t seqNumber)
> {
    NS_ASSERT (startingSeq <= 4096);
    NS_ASSERT (seqNumber <= 4096);
>   uint16_t distance = ((seqNumber - startingSeq) + 4096) % 4096;

Why not ? But, really, I don't remember the details of that code. I suspect that you know more about it than me now.

Fixed in ns-3-dev, changeset 6600:e438f9b17c66