ieee802154: clean-up (Intra-PAN behavior + broadcast) by miri64 · Pull Request #5897 · RIOT-OS/RIOT

miri64 · 2016-09-30T11:24:29Z

In #5685 (comment) we realized, that correcting our Intra-PAN behavior would also effect broadcasting behavior due to inconsistent handling of the broadcast/multicast flags from GNRC. This PR fixes that by getting rid of the IEEE802154_BCAST flag and replacing it with the IEEE802154_ADDR_BCAST constant. This way the broadcast address is handled like any other address in ieee802154_set_frame_hdr() since the GNRC glue code now translates the (GNRC-internal) broadcast/multicast flags into the broadcast address, and not that function.

Since this cleanup is tightly bound to the PAN_COMP clean-up (and it would have been more complicated to work around this changes, since I needed to touch that lines anyway) I decided to incorporate #5685 (due to the dropping of the bcast variable that change looks a little different though, so its more of an alternative) and #5759 in here. Hope @aeneby and @cgundogan won't mind ;).

Summary of the changes

Removal of the device state flag NETDEV2_IEEE802154_PAN_COMP + change of the header setting behavior for PANs so that it is independent of external state (except the value of the given PAN IDs ;-))
Removal of the need for an IEEE802154_BCAST flag. The IEEE 802.15.4 broadcast address is now a global constant and thus can be set outside ieee802154_set_frame_hdr() so that it can be handled like any other destination address.
(clean-up of the netdev2_ieee802154 flags)
(adaptation of the unittests)

smlng

Tested this one on boards SAMR21 and REMOTE-REVB, works but ACK_REQ is on for broadcast.

smlng · 2016-09-30T12:05:12Z

sys/net/link_layer/ieee802154/ieee802154.c

+            buf[1] |= IEEE802154_FCF_DST_ADDR_VOID;
+            break;
+        case 2:
+            if (memcmp(dst, ieee802154_addr_bcast,


This should be if(!memcmp(...)) ..., currently this PR introduces ACKs for broadcasts, which is unwise.

Me everytime with these kind of functions ^^"...

smlng · 2016-09-30T12:06:18Z

sys/include/net/ieee802154.h

+/**
+ * @brief   Static initializer for broadcast address
+ */
+#define IEEE802154_ADDR_BCAST               { 0xff, 0xff }


rename to IEEE802154_BCAST_ADDR

Why? This group defines special IEEE 802.15.4 addresses (IEEE802154_ADDR). So if other special addresses are later added this would look as unified IMHO.:

#define IEEE802154_BCAST_ADDR { 0xff, 0xff } #define IEEE802154_FOO_ADDR .... #define IEEE802154_BAR_ADDR ....

mhm, this is a matter of taste --and grouping things.

I was thinking IEEE802154_BCAST_ADDR and IEEE802154_BCAST_ADDR_LEN together, so its an broadcast address which has address length (not broadcast length).

But, what matters most is that we get this PR/ fix through, and I don't want to start a naming discussion - such issues are all over the place, if we would look deep enough 😬

smlng · 2016-09-30T12:06:35Z

sys/include/net/ieee802154.h

+ * @brief   Length in byte of @ref IEEE802154_ADDR_BCAST
 */
-#define IEEE802154_BCAST                    (0x80)
+#define IEEE802154_ADDR_BCAST_LEN           (IEEE802154_SHORT_ADDRESS_LEN)


rename to IEEE802154_BCAST_ADDR_LEN

See #5897 (comment)

aeneby · 2016-09-30T12:08:52Z

Hope @aeneby and @cgundogan won't mind ;).

No, I don't mind. Actually I haven't had much time to work on this so thanks @miri64 for taking the reins.

I still think that the PAN_COMP flag should be getting set before the call to ieee802154_set_frame_hdr() (i.e. in gnrc_netdev2->send()), but it seems that there is still some work to do before this is possible anyway. Perhaps we can re-visit this later. I'm very glad to see NETDEV2_IEEE802154_PAN_COMP disappearing as a device state flag though 👍

miri64 · 2016-09-30T12:21:01Z

I still think that the PAN_COMP flag should be getting set before the call to ieee802154_set_frame_hdr() (i.e. in gnrc_netdev2->send()), but it seems that there is still some work to do before this is possible anyway. Perhaps we can re-visit this later. I'm very glad to see NETDEV2_IEEE802154_PAN_COMP disappearing as a device state flag though 👍

That's the thing I don't understand: set where? There is no flag for this in ieee802154_set_frame_hdr() defined anymore. It's purely dependent on the configuration of the input PANs, as you originally intended.

smlng

you don't like my shorter version using !memcmp(), however: ACK.

miri64 · 2016-09-30T12:56:32Z

you don't like my shorter version using !memcmp(), however: ACK.

Because it is misleading. The return value of memcmp() is not a boolean value, so using a boolean operation is just lazy coding in my opinion ;-).

miri64 · 2016-09-30T12:58:03Z

Squashed.

aeneby · 2016-09-30T23:26:09Z

That's the thing I don't understand: set where? There is no flag for this in ieee802154_set_frame_hdr() defined anymore. It's purely dependent on the configuration of the input PANs, as you originally intended.

Okay. But if this is the case then I guess the API documentation for the flags parameter needs updating.

miri64 · 2016-10-01T00:02:10Z

sys/include/net/ieee802154.h

- *                      do ignore @p dst and @p dst_len and just set `ff:ff`
- *                      (broadcast) as destination address
+ *                      @ref IEEE802154_FCF_FRAME_PEND, and
+ *                      @ref IEEE802154_FCF_ACK_REQ.


That's the thing I don't understand: set where? There is no flag for this in ieee802154_set_frame_hdr() defined anymore. It's purely dependent on the configuration of the input PANs, as you originally intended.

Okay. But if this is the case then I guess the API documentation for the flags parameter needs updating.

@aeneby Already ahead of you ;-)

Yikes - missed that. Sorry for the noise.

PeterKietzmann · 2016-10-04T08:49:00Z

ACK, finally :-)! Unfortunately Murdock fails for qemu-i386 builds.

miri64 · 2016-10-04T08:50:20Z

Mh yeah... The file was provided by @Kijewski, however non of the people with access to the server sprang into action yet :-/.

PeterKietzmann · 2016-10-04T08:57:59Z

Ah, that story :/ ... How do we proceed, wait until someone found that file? I assume all builds are failing now.

miri64 · 2016-10-04T09:01:42Z

We have two solution:

the file "was found" (actually @Kijewski just rebuild) so we could upload it to downloads.riot-os.org's new home.
cu is reachable again, so we can move the URL back to that location.

Sadly, the people that could do either of those seem to be MIA.

miri64 · 2016-10-04T09:05:43Z

Alternatively for this PR we can just say that qemu-i586 is definitely not effected by this PR (because it isn't) and just merge if everything else is alright, but since I'm the author I don't want to make that call ;-).

PeterKietzmann · 2016-10-04T09:53:00Z

I scanned Murdocks output to verify it's just qemu that fails. Let's wait until early afternoon if someone is able to fix the missing file. Otherwise I'll merge as is.

PeterKietzmann · 2016-10-04T12:09:56Z

Ok let's do this. ACK (again) and go.

miri64 added 2 commits September 30, 2016 13:09

netdev2_ieee802154: remove NETDEV2_IEEE802154_PAN_COMP flag

9f29e56

netdev2_ieee802154: cleanup flag definitions

15c4cea

miri64 assigned PeterKietzmann and smlng Sep 30, 2016

This was referenced Sep 30, 2016

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

Closed

at86rf2xx: do not set src pan compression on init #5759

Closed

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 30, 2016

smlng suggested changes Sep 30, 2016

View reviewed changes

smlng approved these changes Sep 30, 2016

View reviewed changes

miri64 added 3 commits September 30, 2016 14:57

ieee802154: remove need for IEEE802154_BCAST flag

a0454b3

tests: adapt unittests for removal of user-controlled PAN_COMP

3b35dae

tests: adapt tests for changed broadcast settings

ed34390

miri64 force-pushed the ieee802154/enh/cleanup branch from d9b8648 to ed34390 Compare September 30, 2016 12:57

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 30, 2016

miri64 commented Oct 1, 2016

View reviewed changes

PeterKietzmann added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 4, 2016

PeterKietzmann merged commit 359f3b5 into RIOT-OS:master Oct 4, 2016

miri64 deleted the ieee802154/enh/cleanup branch October 4, 2016 12:15

OlegHahm added this to the Release 2016.10 milestone Oct 13, 2016

miri64 mentioned this pull request Oct 18, 2016

ieee802154: Intra-pan packet headers are constructed incorrectly #5684

Closed

PeterKietzmann mentioned this pull request Nov 14, 2016

Mrf24j40 #5792

Closed

Conversation

miri64 commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the changes

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeneby commented Sep 30, 2016

Uh oh!

miri64 commented Sep 30, 2016

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 30, 2016

Uh oh!

miri64 commented Sep 30, 2016

Uh oh!

aeneby commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Oct 4, 2016

Uh oh!

miri64 commented Oct 4, 2016

Uh oh!

PeterKietzmann commented Oct 4, 2016

Uh oh!

miri64 commented Oct 4, 2016

Uh oh!

miri64 commented Oct 4, 2016

Uh oh!

PeterKietzmann commented Oct 4, 2016

Uh oh!

PeterKietzmann commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

miri64 commented Sep 30, 2016 •

edited

Loading