at86rf2xx: do not set src pan compression on init by cgundogan · Pull Request #5759 · RIOT-OS/RIOT

cgundogan · 2016-08-18T12:38:30Z

Depends on ~~#5525~~ #5685

Currently, the at86rf2xx driver sets the src pan compression on initialization. Doing this when building the 802.15.4 header instead (like e.g. proposed by @aeneby in ~~#5525~~ #5685 ) is more dynamic, so that the flag is still set/unset correctly iff the pan id changes.

According to section 7.5.6.1 of 802.15.4-2003, packets destined for the same PAN from which they were sent must have their intra-PAN flag set, and the source PAN omitted from the transmitted frame.

miri64 · 2016-08-18T12:51:16Z

Do you mean #5685? Also: What happens if the flag is set?

cgundogan · 2016-08-18T13:04:53Z

yes #5685

Also: What happens if the flag is set?

Can you rephrase? The flag is set by default currently and this leads to the src pan being compressed (elided from the header). This is no problem for now (and correct), because there is no case where ieee802154_set_Frame_hdr() is called with different src and dst pans (https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/link_layer/netdev2/gnrc_netdev2_ieee802154.c#L185).

iff ieee802154_set_Frame_hdr() gets called with different pan ids in the future, then this flag must be unset (this is still missing in #5685 )

miri64 · 2016-08-18T14:53:47Z

You unset it, yes. But what happens now, if someone sets it later on?

cgundogan · 2016-08-18T15:22:34Z

You unset it, yes. But what happens now, if someone sets it later on?

If you set it later, then you have the same functionality as before.

cgundogan · 2016-08-18T16:14:57Z

as a side note, NETDEV2_IEEE802154_PAN_COMP does not correspond to a register of the device.

aeneby · 2016-08-19T02:34:40Z

I don't really understand how PAN compression is relevant to the driver at all... is the NETDEV2_IEEE802154_PAN_COMP flag even necessary?

miri64 · 2016-08-19T06:22:41Z

I don't really understand how PAN compression is relevant to the driver at all... is the NETDEV2_IEEE802154_PAN_COMP flag even necessary?

If src_pan == dst_pan but the compression shouldn't be used eg. I already had the problem, that there is a bug in lwIP that it just ignores that bit and always skips the source PAN.

aeneby · 2016-08-19T06:27:13Z

If src_pan == dst_pan but the compression shouldn't be used

That would be an invalid frame though, according to the 802.15.4 spec.

miri64 · 2016-08-19T07:28:16Z

Yes, but for Interop its sometimes necessary to provide behavior beyond the spec until a bug is fixed ;-) (I don't say that its not wrong, just that lwIP is doing it wrong atm).

aeneby · 2016-08-19T07:37:48Z

I don't really know enough about lwIP to comment, but if lwIP is doing it wrong then it needs to be fixed in lwIP, not broken in RIOT - two wrongs don't make a right :)

thomaseichinger · 2016-08-19T08:22:05Z

If src_pan == dst_pan but the compression shouldn't be used

That would be an invalid frame though, according to the 802.15.4 spec.

@aeneby Could you point me to where in the 802.15.4 spec this is specified? Or which version of 802.15.4? As far as I read point 5.2.1.1.5 it is possible to have the two PAN IDs set to the same value without the need to use compression.

aeneby · 2016-08-19T09:24:14Z

@thomaseichinger, you have a point. In the 2003 spec it was quite clear, since it was called the "Intra-PAN" field. With this definition, it simply doesn't make sense to have src and dst PANs the same without this bit being set (and therefore the src PAN removed). However in the 2011 spec, the name has changed to "PAN ID Compression" field, even though the functional definition remains basically the same. I guess this would allow Intra-PAN packets to be sent with both src and dst PANs specified, according to the semantic definition of this field...?

Now I'm not 100% sure, I will think over it.

miri64 · 2016-08-19T11:07:08Z

@aeneby

I don't really know enough about lwIP to comment, but if lwIP is doing it wrong then it needs to be fixed in lwIP, not broken in RIOT - two wrongs don't make a right :)

I agree with you and I finally came around to open an issue on the lwIP side, but I know enough about lwIP to say that changes to mainline are quite slow (6lowpan took 1.5 years to get to master, my issue about the incorrect fragmentation [a very pressing issue] is still open after ~6 month and the last non-RC release was nearly 4 years ago). So as a workaround I would still have this kind of option provided (in case they provide the fragmentation fix first ^^).

@thomaseichinger, you have a point. In the 2003 spec it was quite clear, since it was called the "Intra-PAN" field. With this definition, it simply doesn't make sense to have src and dst PANs the same without this bit being set (and therefore the src PAN removed). However in the 2011 spec, the name has changed to "PAN ID Compression" field, even though the functional definition remains basically the same. I guess this would allow Intra-PAN packets to be sent with both src and dst PANs specified, according to the semantic definition of this field...?

Now I'm not 100% sure, I will think over it.

To me it reads that the compression is basically optional. Yes it makes sense to compress the source PAN away if it is identical, but I wouldn't say its completely wrong to include it regardless.

thomaseichinger · 2016-08-19T11:07:36Z

Referencing the 2011 802.15.4 spec

5.2.1.1.5 PAN ID Compression field
The PAN ID Compression field specifies whether the MAC frame is to be sent containing only one of the PAN identifier fields when both source and destination addresses are present. If this field is set to one and both the source and destination addresses are present, the frame shall contain only the Destination PAN Identifier field, and the Source PAN Identifier field shall be assumed equal to that of the destination. If this field is set to zero, then the PAN Identifier field shall be present if and only if the corresponding address is present.

I think the argumentation is just the other way around. If the bit is set there must only be one PAN ID while there are no rules constraining the usage of PAN IDs if the bit is not set. At least in the 2011 version. (I also didn't get my hands on the most recent 2015 version yet.)

The interesting part I think is however, as, at least the at86rf231, to my knowledge implements IEEE 802.15.4 2006, how behavior of the hardware frame filters looks like in that case. (I don't have the 2006 version around right now.)

miri64 · 2016-08-19T11:08:45Z

(to be clear: the faulty behavior of lwIP in my eyes is that they completely ignore the compression bit, not that they include the source PAN, even if it is identical to the destination PAN).

thomaseichinger · 2016-08-19T11:18:50Z

@miri64 This is undisputed and the discussion not necessarily prevents from moving forward with this PR as AFAIK RIOT should be able to deal with two same PAN IDs anyways.

miri64 · 2016-08-19T11:36:14Z

@miri64 This is undisputed and the discussion not necessarily prevents from moving forward with this PR as AFAIK RIOT should be able to deal with two same PAN IDs anyways.

But #5685 takes away this option.

thomaseichinger · 2016-08-19T13:55:12Z

@miri64 I think it's totally standard conform if RIOT does PAN ID compression, my note was more about the receiving side. RIOT should be able to process a frame that doesn't have the compression bit set and two identical PAN IDs.

aeneby · 2016-08-19T21:27:52Z

After doing some careful reading, here is my interpretation (I'll try to stay relevant to this PR):

According to the 802.15.4-2003 spec, any frame with identical source and destination PANs is invalid
In the 802.15.4-2011 spec, the meaning of the field has changed; it is valid, albeit wasteful, for a frame to contain identical source and destination PANs
In the 802.15.4-2015 spec... who knows, because I don't enjoy reading standards documents enough to pay ~$400 for the privilege. Does anyone have access to this?

What does this mean for this PR?

It does in fact make some sense to have the NETDEV2_IEEE802154_PAN_COMP flag since, in theory at least, it's valid to send intra-PAN packets without using PAN compression
In practice though, I don't see why you would ever want to do this. Doesn't that mean this should be left as true by default?
While ieee802154: Set intra-PAN flag for packets where src/dst PANs match #5685 does enforce the only really sensible behaviour, strictly speaking it is perhaps wrong to do so - we should not be modifying this flag based on src and dst PANs, but rather using this flag to determine what to do if the src and dst PANs are the same (compress or not compress)

The main issue is that, since each version of the 802.15.4 supersedes the ones before it, 802.15.4-2015 should be the one we are following. So there is still a missing piece of the puzzle here.

aeneby · 2016-08-19T22:05:47Z

RIOT should be able to process a frame that doesn't have the compression bit set and two identical PAN IDs.

According to my interpretation of the 2011 spec, yes, this is correct. Interesting to note though that Linux will drop such frames.

aeneby · 2016-08-22T03:54:44Z

See #5685 (comment).

So in short, it looks like frames with identical src and dst PAN IDs are invalid again in the 2015 spec. Further, it's clear that the MAC layer should be automatically handling PAN ID compression. So actually, provided the linked text is accurate, I don't see any reason for the NETDEV2_IEEE802154_PAN_COMP flag to exist at all.

miri64 · 2016-08-30T19:43:02Z

drivers/at86rf2xx/at86rf2xx.c

    at86rf2xx_set_txpower(dev, AT86RF2XX_DEFAULT_TXPOWER);
    /* set default options */
-    at86rf2xx_set_option(dev, NETDEV2_IEEE802154_PAN_COMP, true);
+    at86rf2xx_set_option(dev, NETDEV2_IEEE802154_PAN_COMP, false);


If this option is ignored anyway, shouldn't the code for at86rf2xx_set_option be disabled anyway?

For this option I mean.

@miri64 I think you're right. There is no explicit implementation behind this option, so simply removing this line would probably be the most logical thing?

PeterKietzmann · 2016-09-01T08:33:53Z

How about me use this PR just for removing at86rf2xx_set_option(dev, NETDEV2_IEEE802154_PAN_COMP, false); from the driver and handle the rest in #5685

miri64 · 2016-09-26T15:01:03Z

@PeterKietzmann +1

miri64 · 2016-09-26T15:01:41Z

sys/net/link_layer/ieee802154/ieee802154.c


+    if (dst_pan.u16 == src_pan.u16) {
+        flags |= IEEE802154_FCF_PAN_COMP;
+    }


See #5759 (comment)

miri64 · 2016-09-30T11:28:47Z

Alternative in #5897

cgundogan · 2016-09-30T15:02:36Z

No objection to close once #5897 is merged.

PeterKietzmann · 2016-10-04T08:51:11Z

Closing in favour of #5897.

aeneby and others added 2 commits August 18, 2016 14:22

ieee802154: Set intra-PAN flag for packets where src/dst PANs match

cc0bddc

According to section 7.5.6.1 of 802.15.4-2003, packets destined for the same PAN from which they were sent must have their intra-PAN flag set, and the source PAN omitted from the transmitted frame.

at86rf2xx: do not set src pan compression on init

4c7d8c0

cgundogan added Area: network Area: Networking Area: drivers Area: Device drivers labels Aug 18, 2016

cgundogan assigned miri64 and PeterKietzmann Aug 18, 2016

cgundogan added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 18, 2016

aeneby mentioned this pull request Aug 19, 2016

ieee802154: Set intra-PAN flag for packets where src/dst PANs match #5685

Closed

miri64 reviewed Aug 30, 2016
View reviewed changes

miri64 reviewed Sep 26, 2016

View reviewed changes

miri64 mentioned this pull request Sep 30, 2016

ieee802154: clean-up (Intra-PAN behavior + broadcast) #5897

Merged

PeterKietzmann closed this Oct 4, 2016

cgundogan deleted the pr/at86rf2xx/src_comp branch October 4, 2016 09:05

Conversation

cgundogan commented Aug 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 18, 2016

Uh oh!

cgundogan commented Aug 18, 2016

Uh oh!

miri64 commented Aug 18, 2016

Uh oh!

cgundogan commented Aug 18, 2016

Uh oh!

cgundogan commented Aug 18, 2016

Uh oh!

aeneby commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016

Uh oh!

aeneby commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016

Uh oh!

aeneby commented Aug 19, 2016

Uh oh!

thomaseichinger commented Aug 19, 2016

Uh oh!

aeneby commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016

Uh oh!

thomaseichinger commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016

Uh oh!

thomaseichinger commented Aug 19, 2016

Uh oh!

miri64 commented Aug 19, 2016

Uh oh!

thomaseichinger commented Aug 19, 2016

Uh oh!

aeneby commented Aug 19, 2016

Uh oh!

aeneby commented Aug 19, 2016

Uh oh!

aeneby commented Aug 22, 2016

Uh oh!

miri64 Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

miri64 Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Sep 1, 2016

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Sep 1, 2016

Uh oh!

miri64 commented Sep 26, 2016

Uh oh!

miri64 Sep 26, 2016

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 30, 2016

Uh oh!

cgundogan commented Sep 30, 2016

Uh oh!

PeterKietzmann commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cgundogan commented Aug 18, 2016 •

edited

Loading