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.

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

gnrc_netdev2: link-layer retransmissions outside the transceiver driver #4795

Closed
wants to merge 14 commits into from

Conversation

OlegHahm
Copy link
Member

This PR introduces two features:
1.) Link layer retransmissions inside netdev2. Therefore, link layer retransmissions are disabled in the driver.
2.) netstats as a mostly generic module to collect statistics per layer. A first implementation for at86rf2xx's netdev2 driver is added.

Depends on #4645

@OlegHahm OlegHahm added Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Feb 11, 2016
@OlegHahm
Copy link
Member Author

I know it's not a beauty, but can become quite handy if you want to measure what's really happening inside the stack.

@miri64
Copy link
Member

miri64 commented Feb 11, 2016

This seems to be at first glance more of a gnrc_netdev2 change than a netdev2 change. Please clarify in the description. Why is it called netdev_retrans and not netdev2_retrans? More thorough review will follow when dependencies got merged (or when I find the time to differentiate your code from mine ^^)

@OlegHahm
Copy link
Member Author

Well, ideally, it would go into netdev2, but I need something to queue the packets while waiting for retransmission and therefore the pktbuf is quite handy.

@miri64
Copy link
Member

miri64 commented Feb 11, 2016

Why netdev2? Isn't this a very radio-only feature?

@OlegHahm
Copy link
Member Author

Why radio-only?

@kaspar030
Copy link
Contributor

@OlegHahm Would you mind factoring out the netstats?

Regarding the retransmissions, I think a combined approach is best:

  • gnrc's powerful pktsnips show that packet queueing is best done in the network stack
  • netdev2 still needs to communicate e.g., a busy medium

I propose that netdev2 devices do, if not configured to do some kind of internal retransmissions, return a defined error when the medium is busy when sending, e.g., "-EBUSY" or "-EAGAIN" (we should use one for the device actually being busy and the other for "medium is busy".

In gnrc, we devise a simple intermediate handler that overrides _send (and possibly _get/_set), on which radio devices (gnrc_iee802154/gnrc_cc110x) can be stacked.

Why radio-only?
Maybe it's not radio-only, but for some netdev's (e.g., ethernet), it doesn't make sense to support media access control. If that handling is inside gnrc_netdev2, it will be compiled in (and executed) for all types of devices.

@OlegHahm
Copy link
Member Author

@OlegHahm Would you mind factoring out the netstats?

Yes, makes sense, will do. Although without the retransmission mechanism the stats won't be accurate.

@OlegHahm
Copy link
Member Author

The problem that I'm current facing is that I lose interrupts on the SAMR21. If I send fragmented 6LoWPAN packets and the last fragment is rather small (e.g. <= 20 bytes of payload), I will only get one TX_END IRQ.

@OlegHahm OlegHahm changed the title netdev2: link-layer retransmissions outside the transceiver driver gnrc_netdev2: link-layer retransmissions outside the transceiver driver Feb 12, 2016
@OlegHahm
Copy link
Member Author

Problem fixed. Thanks @haukepetersen for the advice.

@OlegHahm OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 12, 2016
@OlegHahm
Copy link
Member Author

@kaspar030, here you go: #4801

@OlegHahm
Copy link
Member Author

Surprisingly it doesn't seem to affect the performance when running pinging (tested on SAMR21-xpro with 100 pings a 1008 bytes, i.e. 10 fragments per datagram).

@miri64
Copy link
Member

miri64 commented Mar 6, 2016

Needs rebase. I did not look into the code, but if I understand the problem correctly: Can't this be solved with a tx_info struct, similar as I did with rx_info in #4648?

@OlegHahm
Copy link
Member Author

OlegHahm commented Mar 6, 2016

What can be solved?

@miri64
Copy link
Member

miri64 commented Mar 7, 2016

Getting the number of retransmissions done by the driver.

@OlegHahm
Copy link
Member Author

OlegHahm commented Mar 7, 2016

I don't understand how. The transceiver and therefore the driver doesn't know anything about the number of retransmissions.

@miri64
Copy link
Member

miri64 commented Mar 7, 2016

Huh? If not the transceiver, who else?

@OlegHahm
Copy link
Member Author

OlegHahm commented Mar 7, 2016

In this PR: gnrc_netdev2. That's basically the whole point of this PR. ;)

@miri64
Copy link
Member

miri64 commented Mar 7, 2016

Ahhhh sorry I missunderstood the PR. I thought you introduced a feature to get the number of automatic retransmissions of the device... not that you take out the retransmissions to the MAC layer... Okay, then I wait until #4646 is merged for my final review :-)

@OlegHahm
Copy link
Member Author

rebased to current master after #4646 got merged.

@miri64
Copy link
Member

miri64 commented Mar 24, 2016

Isn't netstats introduced in #4801?

@OlegHahm
Copy link
Member Author

Yes.

@miri64
Copy link
Member

miri64 commented Mar 24, 2016

So why did you rebase to master then?

@miri64
Copy link
Member

miri64 commented Oct 30, 2016

Might also be of interest for @zhuoshuguo and @daniel-k especially considering that they are working on a priority queue for MAC too.

@miri64
Copy link
Member

miri64 commented Oct 31, 2016

Postponed due to feature freeze

@miri64 miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016
@miri64
Copy link
Member

miri64 commented Nov 7, 2016

Please see if you can unify this with #5941 and #5950 somehow (also needs rebase).

@miri64
Copy link
Member

miri64 commented Jan 10, 2017

@OlegHahm ping?

@miri64
Copy link
Member

miri64 commented Jan 10, 2017

(both PRs got merged in the meantime so rebasing is and unification with those is now required)

@OlegHahm
Copy link
Member Author

I can certainly rebase, but I'm not sure I will find the time for extensive testing.

@miri64
Copy link
Member

miri64 commented Jan 24, 2017

Ok, then let's postpone

@miri64 miri64 modified the milestones: Release 2017.04, Release 2017.01 Jan 24, 2017
@miri64
Copy link
Member

miri64 commented Mar 16, 2017

@OlegHahm What is the status of this?

@miri64
Copy link
Member

miri64 commented Jun 27, 2017

Needs rebase.

@kYc0o kYc0o mentioned this pull request Oct 30, 2017
15 tasks
@miri64
Copy link
Member

miri64 commented Nov 15, 2017

The code looks pretty stale and I think it would be better to have this done in a matter as proposed in #7736, so that other stacks can benefit from that.

@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Nov 15, 2017
@miri64 miri64 closed this Nov 15, 2017
@benpicco benpicco mentioned this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: archived State: The PR has been archived for possible future re-adaptation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants