Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aeneby wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sys/net/link_layer/ieee802154/ieee802154.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ size_t ieee802154_set_frame_hdr(uint8_t *buf, const uint8_t *src, size_t src_len
uint8_t type = (flags & IEEE802154_FCF_TYPE_MASK);
uint8_t bcast = (flags & IEEE802154_BCAST);

if (dst_pan.u16 == src_pan.u16 &&
(src_len != 0 && dst_len != 0)) {
Copy link
Copy Markdown
Member

@smlng smlng Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with this comparison of dst_len != 0 is that for multicast messages such as RS the length actually is 0 see 1. The result is the for RS messages we have SRC_PAN==DST_PAN but dst_len=0 and the flag will be unset which makes Linux drop those RS and never send an RA.

This could be fixed, for instance in 2, by not not passing netif_hdr->dst_l2addr_lendirectly but rather set this like with src_len. That is, if the packet is broadcast or multicast (see 3) then set dst_len = 2.

Copy link
Copy Markdown
Member

@miri64 miri64 Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with this comparison of dst_len != 0 is that for multicast messages such as RS the length actually is 0 see 1. The result is the for RS messages we have SRC_PAN==DST_PAN but dst_len=0 and the will not be unset which makes Linux drops those RS and never send an RA.

Just an explanation for this behavior: since netif_hdr_t is L2 agnostic and the format of a multicast address can be very L2 specific, multicast is just handled by setting a special multicast flag and not setting the destination address in the netif_hdr_t. The L2 then sets the multicast address to its liking according to e.g. the IPv6 destination address (or in IEEE 802.15.4 by just using the broadcast address).

Copy link
Copy Markdown
Member

@smlng smlng Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miri

The L2 then sets the multicast address to its liking according to e.g. the IPv6 destination address

where does this happen in the standard case of 6lo over 802154?

Copy link
Copy Markdown
Member

@smlng smlng Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miri64, ah forget about it, I found it: https://github.com/RIOT-OS/RIOT/blob/master/sys/net/link_layer/ieee802154/ieee802154.c#L60

Copy link
Copy Markdown
Member

@miri64 miri64 Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few lines below

Copy link
Copy Markdown
Member

@smlng smlng Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miri64 I don't understand why src address and length are set in _send here but the destination is not set the same way when it is multicast? I mean the code in gnrc_netdev2_ieee802154.c already knows about L2 so it should be able to set the address like here, or not?

Copy link
Copy Markdown
Member

@smlng smlng Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and what's the reasoning behind unset the broadcast flag here?

Copy link
Copy Markdown
Member

@miri64 miri64 Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miri64 I don't understand why src address and length are set in _send here but the destination is not set the same way when it is multicast? I mean the code in gnrc_netdev2_ieee802154.c already knows about L2 so it should be able to set the address like here, or not?

Because the driver is not "supposed" to know the length of the broadcast address. That's this function's job.

and what's the reasoning behind unset the broadcast flag here?

The flag is not part of the IEEE 802.15.4 specs so adding it to the MAC header seems like a bad idea ;-).

Copy link
Copy Markdown
Member

@smlng smlng Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miri64, this is no driver code (I thought)?! I was talking about these 2 files

  1. /sys/net/link_layer/ieee802154/ieee802154.c
  2. /sys/net/gnrc/link_layer/netdev2/gnrc_netdev2_ieee802154.c

both are not driver, but src is set in the GNRC (2.) part but dst if broad/multicast is set in the other file (2.). I argue for consistency, i.e., set broadcast address in GNRC too, and further get rid of this broadcast flag setting and later unsetting again.

Copy link
Copy Markdown
Member

@miri64 miri64 Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to confess with "Because the driver is not 'supposed' to know […]" I was mixing terms myself. I meant "the glue code" gnrc_netdev2_ieee802154 ^^".

I argue for consistency, i.e., set broadcast address in GNRC too, and further get rid of this broadcast flag setting and later unsetting again.

Might indeed be a good idea!

flags |= IEEE802154_FCF_PAN_COMP;
}
else {
flags &= ~IEEE802154_FCF_PAN_COMP;
}

buf[0] = flags & (~IEEE802154_BCAST);
buf[1] = IEEE802154_FCF_VERS_V1;

Expand Down