Skip to content
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

Jump to bottom

netdev_ieee802154: Use intermediate layer for mac header #9417

Closed
wants to merge 11 commits into from
Closed

netdev_ieee802154: Use intermediate layer for mac header #9417

wants to merge 11 commits into from

Conversation

bergzand
Copy link
Member

Contribution description

This PR splits the construction of the ieee802154 MAC header. gnrc_netif_ieee802154 constructs a generic header with only the MAC addresses. The netdev_ieee802154 code then reformats it to a "proper" ieee802.15.4 MAC frame with the information from the device driver.

Issues/PRs references

#9330 and part of the follow up work of #8198

WIP as the gomach and the lwmac still need to be adapted.

bergzand added 2 commits June 26, 2018 19:28
@bergzand
netdev: Add intermediate ieee802154 mac layer
1cd163d 
This commit adds the option to use an intermediate format in
communicating with netdev to isolate the device settings from the upper
layers.
@bergzand
gnrc_netif: Use netdev ieee802154 glue format
9353a39
@bergzand bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking GNRC labels Jun 26, 2018
@bergzand bergzand requested a review from miri64 June 26, 2018 17:41
@bergzand
Copy link
Member Author

bergzand commented Jun 26, 2018 •
edited
Loading

Effective impact on code size is an increase of around 184 bytes with gnrc_networking on a samr21-xpro

@miri64
Copy link
Member

miri64 commented Jun 26, 2018

For a better architecture, I think only ~200 bytes are a good sacrafice.

@miri64
Copy link
Member

miri64 commented Jun 26, 2018

But maybe put a data instead of mac somewhere in the name. Because implementing MAC with this layer is next to impossible (no way to send beacons, ACKs, etc.)

@bergzand
Copy link
Member Author

But maybe put a data instead of mac somewhere in the name. Because implementing MAC with this layer is next to impossible (no way to send beacons, ACKs, etc.)

Thanks, I was looking for a better name, but couldn't think of anything while implementing.

@bergzand
Copy link
Member Author

Part of the L2 statistics are still handled in the gnrc_netif_ieee802154 code.

miri64
miri64 requested changes Jun 26, 2018
View reviewed changes
drivers/include/net/netdev/ieee802154.h Outdated
uint8_t dst[IEEE802154_LONG_ADDRESS_LEN]; /**< Destination address */
uint8_t src_len; /**< Source address length */
uint8_t dst_len; /**< Destination address length */
} netdev_ieee802154_data_t;
Copy link
Member

Choose a reason for hiding this comment

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

data_hdr instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok!

miri64
miri64 requested changes Jun 26, 2018
View reviewed changes
sys/net/gnrc/netif/gnrc_netif_ieee802154.c Outdated
uint8_t flags = (uint8_t)(state->flags & NETDEV_IEEE802154_SEND_MASK);
le_uint16_t dev_pan = byteorder_btols(byteorder_htons(state->pan));
const uint8_t *dst, *src;
netdev_ieee802154_data_t l2data;
Copy link
Member

Choose a reason for hiding this comment

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

l2data_hdr?

Copy link
Member Author

Choose a reason for hiding this comment

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

And ok!

@bergzand
Copy link
Member Author

@miri64 And another set of renaming commits :)

miri64
miri64 reviewed Jun 26, 2018
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Some more comment. But I think lwmac and gomac may need some changing as well. So I wait for the rest until you out of WIP.

drivers/include/net/netdev/ieee802154.h Outdated
* header using the device specific information.
*
* @param[in] dev network device descriptor
* @param[in] iolist io vector list to send
Copy link
Member

Choose a reason for hiding this comment

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

Indentation?

drivers/include/net/netdev/ieee802154.h Outdated
* call data to upper network layers.
*/
typedef struct {
uint8_t src[IEEE802154_LONG_ADDRESS_LEN]; /**< source address */
Copy link
Member

Choose a reason for hiding this comment

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

Consistency of capitalization with comments below?

drivers/netdev_ieee802154/netdev_ieee802154.c Outdated
}
/* prepare iolist for netdev / mac layer */
iolist_t iolist = {
.iol_next = (iolist_t *)list->iol_next,
Copy link
Member

Choose a reason for hiding this comment

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

This cast shouldn't be necessary.

miri64
miri64 reviewed Jun 26, 2018
View reviewed changes
drivers/include/net/netdev/ieee802154.h Outdated
* @param[in] dev network device descriptor
* @param[out] buf buffer to write into or NULL
* @param[in] len maximum number of bytes to read
* @param[out] info status information for the received packet. Might
Copy link
Member

Choose a reason for hiding this comment

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

Okay, since you decided for left aligned below, this one needs adaption as well ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! :)

@bergzand bergzand mentioned this pull request Jun 26, 2018
Closed
@bergzand
Copy link
Member Author

@miri64 I've adapted both lwmac and gomach to also use the new netdev layer.

@smlng
Copy link
Member

smlng commented Jun 27, 2018

@bergzand so no WIP anymore, and thus ready for review? If so, please change title ...

@bergzand bergzand changed the title WIP: netdev_ieee802154: Use intermediate layer for mac header netdev_ieee802154: Use intermediate layer for mac header Jun 27, 2018
@bergzand
Copy link
Member Author

@bergzand so no WIP anymore, and thus ready for review? If so, please change title ...

Yup, ready for review, removed the tag from the title

miri64
miri64 requested changes Jun 28, 2018
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Tested for both "vanilla" GNRC and with both MAC protocols (using the gnrc_networking_mac example). Vanilla works, MAC doesn't (can't ping other node).

Also the layering isn't really in place, since the send and receive functions are called hard-coded instead of the respective layer's netdev_t driver operations.

drivers/include/net/netdev/ieee802154.h
* header using the device specific information.
*
* @param[in] dev network device descriptor
* @param[in] iolist io vector list to send
Copy link
Member

Choose a reason for hiding this comment

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

I wager this parameter is expected to carry a netdev_ieee802154_mac_t header as first element? Please document.

drivers/netdev_ieee802154/netdev_ieee802154.c
l2data_hdr->src_len = src_len;
l2data_hdr->dst_len = dst_len;
/* Remove the ieee802154 frame header */
memmove(pdu_start, pdu_start+mhr_len, res - mhr_len);
Copy link
Member

Choose a reason for hiding this comment

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

Style comment: spaces around "+"

drivers/netdev_ieee802154/netdev_ieee802154.c
int netdev_ieee802154_send(netdev_t *dev, const iolist_t *list)
{
netdev_ieee802154_t *netdev = (netdev_ieee802154_t *)dev;
netdev_ieee802154_data_hdr_t *data = list->iol_base;
Copy link
Member

Choose a reason for hiding this comment

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

data_hdr as a name helps to make the code below less confusing.

sys/net/gnrc/link_layer/lwmac/lwmac_internal.c
@@ -101,10 +93,10 @@ int _gnrc_lwmac_transmit(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt)
res = csma_sender_csma_ca_send(dev, &iolist, &netif->mac.csma_conf);
}
else {
res = dev->driver->send(dev, &iolist);
res = netdev_ieee802154_send(dev, &iolist);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be the same and dev pointing to the netdev_ieee802154_t layer?

sys/net/gnrc/link_layer/lwmac/lwmac.c
@@ -126,27 +125,23 @@ static gnrc_pktsnip_t *_recv(gnrc_netif_t *netif)
DEBUG("_recv_ieee802154: cannot allocate pktsnip.\n");
return NULL;
}
nread = dev->driver->recv(dev, pkt->data, bytes_expected, &rx_info);
nread = netdev_ieee802154_recv(dev, pkt->data, bytes_expected, &rx_info);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be the same and dev pointing to the netdev_ieee802154_t layer?

sys/net/gnrc/netif/gnrc_netif_ieee802154.c
gnrc_pktsnip_t *pkt = NULL;
int bytes_expected = dev->driver->recv(dev, NULL, 0, NULL);
int bytes_expected = netdev_ieee802154_recv(dev, NULL, 0, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be the same and dev pointing to the netdev_ieee802154_t layer?

sys/net/gnrc/netif/gnrc_netif_ieee802154.c
@@ -90,27 +81,23 @@ static gnrc_pktsnip_t *_recv(gnrc_netif_t *netif)
DEBUG("_recv_ieee802154: cannot allocate pktsnip.\n");
return NULL;
}
nread = dev->driver->recv(dev, pkt->data, bytes_expected, &rx_info);
nread = netdev_ieee802154_recv(dev, pkt->data, bytes_expected, &rx_info);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be the same and dev pointing to the netdev_ieee802154_t layer?

sys/net/gnrc/netif/gnrc_netif_ieee802154.c
@@ -157,7 +145,7 @@ static gnrc_pktsnip_t *_recv(gnrc_netif_t *netif)
gnrc_pktbuf_realloc_data(pkt, nread);
} else if (bytes_expected > 0) {
DEBUG("_recv_ieee802154: received frame is too short\n");
dev->driver->recv(dev, NULL, bytes_expected, NULL);
netdev_ieee802154_recv(dev, NULL, bytes_expected, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be the same and dev pointing to the netdev_ieee802154_t layer?

sys/net/gnrc/netif/gnrc_netif_ieee802154.c
@@ -235,7 +216,7 @@ static int _send(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt)
res = dev->driver->send(dev, &iolist);
}
#else
res = dev->driver->send(dev, &iolist);
res = netdev_ieee802154_send(dev, &iolist);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be the same and dev pointing to the netdev_ieee802154_t layer?

sys/net/link_layer/csma_sender/csma_sender.c
@@ -106,7 +107,7 @@ static int send_if_cca(netdev_t *device, iolist_t *iolist)
/* if medium is clear, send the packet and return */
if (hwfeat == NETOPT_ENABLE) {
DEBUG("csma: Radio medium available: sending packet.\n");
return device->driver->send(device, iolist);
return netdev_ieee802154_send(device, iolist);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this still be the same and dev pointing to the netdev_ieee802154_t layer?

@miri64
Copy link
Member

miri64 commented Jun 28, 2018

Tested for both "vanilla" GNRC and with both MAC protocols (using the gnrc_networking_mac example). Vanilla works, MAC doesn't (can't ping other node).

(I also can confirm that it is working in master).

@bergzand
Copy link
Member Author

@miri64 Thank you for your review.

My aim for this PR was to have a it as a preparation step for the layered approach. I first want to have gnrc_netif and netdev decoupled, as in, no direct access to netdev data by gnrc_netif. As a next step in a follow up I want to make the netdev_ieee802154 and netdev_eth a "proper" netdev layer with their own data structs containing a netdev_driver.
As the netdev layer doesn't have a netdev_driver (yet), I can't use it here, thus the hardcoded calls to netdev_ieee802154_send/recv

Tested for both "vanilla" GNRC and with both MAC protocols (using the gnrc_networking_mac example). Vanilla works, MAC doesn't (can't ping other node).

Whoops 😞

@miri64
Copy link
Member

miri64 commented Jun 28, 2018

As the netdev layer doesn't have a netdev_driver (yet), I can't use it here, thus the hardcoded calls to netdev_ieee802154_send/recv

Why not introduce it here. netdev_eth should be dealt with in a separate PR anyway.

@tcschmidt
Copy link
Member

@bergzand IMO it would be beneficial to show a clear description of your attempted software architecture (layering) incl. a drawing. As is it appears slightly unspecified.

@bergzand
Copy link
Member Author

@bergzand IMO it would be beneficial to show a clear description of your attempted software architecture (layering) incl. a drawing. As is it appears slightly unspecified.

Do the schematics from #8198 answer your request? Mostly the diagram and accompanying posts here.

This PRs and any PR related to this one such as #9330 slowly work to the architecture described there.

@tcschmidt
Copy link
Member

Well, #8198 does not display an architecture. There is some rational in issue #7736 (should be referenced) why netdev needs layering., but little reasoning about the specifics. It could be envisioned that others who might pick this up later will find it difficult to understand what and why.

@bergzand
Copy link
Member Author

Why not introduce it here. netdev_eth should be dealt with in a separate PR anyway.

Yes, possible. This would change the scope to not only this netdev_ieee802154_data_hdr_t, but would also require changes to the current set of 802154 radios.

An implementation of a netdev_driver_t struct for the netdev_ieee802154 layer requires a layered structure not only for the send/recv functions, but also for the get/set methods. At the moment these functions are layered in a reverse way (first the function from the radio is called, then the netdev generic function). This would have to be modified to match the send/recv functions (first netdev generic, then the radio).

I have to see if I also require #9329 for this or that I have to statically allocate the new driver structs in auto_init.

I'll open a new PR with the full changes for this in the next few days.

@bergzand
Copy link
Member Author

Well, #8198 does not display an architecture. There is some rational in issue #7736 (should be referenced) why netdev needs layering., but little reasoning about the specifics. It could be envisioned that others who might pick this up later will find it difficult to understand what and why.

I'm not sure I understand what kind of information you're looking for here. Is your question about the changes in this PR specific or more about the overall architecture that we're aiming at in the long run? If it is the last case, maybe we can move this discussion to #7736.

Are you looking for a graphical explanation on how the different networking layers work together? So how the gnrc_netif code calls the netdev layers and how the individual netdev layers interact?

@tcschmidt
Copy link
Member

@bergzand it's not about me, it is the demand for a crystal clear layout of what is concepted here so that current work and future contributions can be measured against. From the start, netdev is an interface and the goal of making it multilayered sounds weird.
Presenting a clear, well justified architecture

fundamental concepts or properties of a system in its environment embodied in its elements, relationships, and in the principles of its design and evolution (ISO/IEC/IEEE 42010)

may help - and netdev++ was lacking this anyway.

@bergzand bergzand mentioned this pull request Jun 29, 2018
Open
@bergzand
Copy link
Member Author

@tcschmidt I would like to continue this discussion in #7736 as it seems to be more about the overal changes to the netdev implementation than the specific changes in this PR

@miri64 miri64 mentioned this pull request Jul 2, 2018
Merged
@jnohlgard jnohlgard mentioned this pull request Jul 2, 2018
Closed
@tcschmidt
Copy link
Member

@bergzand could we do a thorough practical evaluation of the performance impacts prior to progressing (e.g., like in https://arxiv.org/abs/1801.02833) ? I'm saying this after @PeterKietzmann and I have spent life-month of disclosing and debugging insufficiencies of previous versions of netdev ...

@bergzand
Copy link
Member Author

@tcschmidt I really prefer to continue this discussion in #7736 as your questions appear to be more aimed at the overal idea of the layered approach to netdev. This PR is only a small part of the migration.

@tcschmidt
Copy link
Member

@bergzand fine with me - even though my concern is related to code: We've learned that nice neaties in the code can completely mess up performance ...

@bergzand
Copy link
Member Author

@miri64 I was thinking about how to remove the memmove from the receive path, the impact on the latency is not really negligible (17us for large frames).
My current best solution is to add a netdev_data_hdr_t::offset to indicate where the next layer data (6lowpan header) starts. In the receive path this would be set to ieee802154_get_frame_hdr_len(pdu_start) and in the send path it can be zero. The whole sizeof(header)+header->offset snip can then be removed by the gnrc_netif_ieee802154::recv code.

@miri64
Copy link
Member

miri64 commented Jul 18, 2018

I think, except for the a major API change (where recv would get a list as a parameter) this would be the cleanest approach for the moment...

@miri64
Copy link
Member

miri64 commented Jul 18, 2018

(but maybe call it mhr_len instead of offset ;-)).

@bergzand
Copy link
Member Author

(but maybe call it mhr_len instead of offset ;-)).

👍

@miri64 miri64 removed the GNRC label Oct 5, 2018
@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 22, 2018
@miri64
Copy link
Member

miri64 commented Nov 22, 2018

Marked as WIP, since you seem to steer this into a different direction according to #7736 (comment).

@miri64 miri64 mentioned this pull request Jun 19, 2019
Merged
@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 requested changes

Assignees
No one assigned
Labels
Area: network Area: Networking State: stale State: The issue / PR has no activity for >185 days State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bergzand @miri64 @smlng @tcschmidt