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

drivers: Introduce a general interface for network device drivers #1492

Merged
merged 3 commits into from
Oct 2, 2014

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jul 29, 2014

This is thought to merge the concepts of @rousselk's #925 and my #1448. It also addresses #1448 (comment)

Things to do:

  • make ieee802154_radio_driver_t subclass of net_dev_driver_t
  • rename to naming scheme where renaming is due
  • adapt already implemented drivers
  • add native support
  • provide tests

Dependency graph:

… …
| |
| * #1448
| |\
* | | #1733
| * | #1732
| | * #1731
| |/
|/
* #1492

@miri64
Copy link
Member Author

miri64 commented Aug 5, 2014

Can someone with a cc2420 board test if this works?

@miri64
Copy link
Member Author

miri64 commented Aug 6, 2014

@LudwigOrtmann you might want to review, too, now that I started to adapt nativenet for net_dev.

@LudwigKnuepfer
Copy link
Member

I'd like to but I'm mostly out of resources right now and the PR is so intimidating.

@miri64
Copy link
Member Author

miri64 commented Aug 7, 2014

When I'm done with native I'll write some tests :-)

@miri64
Copy link
Member Author

miri64 commented Aug 7, 2014

Rebased and squashed. Will provide tests tomorrow.

@miri64
Copy link
Member Author

miri64 commented Aug 10, 2014

Rebased and squashed

@miri64
Copy link
Member Author

miri64 commented Aug 11, 2014

Had to rebase and used the chance to squash some commits.

@miri64
Copy link
Member Author

miri64 commented Aug 14, 2014

I'm done \o/

@LudwigKnuepfer
Copy link
Member

No you're not, buildtest fails for tests/net_dev =(

@miri64
Copy link
Member Author

miri64 commented Aug 14, 2014

Orrr, MSP430-libc! Was kannst du eigentlich?!!?

@miri64
Copy link
Member Author

miri64 commented Aug 14, 2014

Now it should work.

@miri64
Copy link
Member Author

miri64 commented Aug 14, 2014

I do not really understand why the unittests fail oO. On my machine everything is running smoothly

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 14, 2014
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 14, 2014
@miri64
Copy link
Member Author

miri64 commented Sep 26, 2014

ping?

@miri64
Copy link
Member Author

miri64 commented Sep 29, 2014

Needed to rebasedue to Makefile-change in #1714

@miri64
Copy link
Member Author

miri64 commented Sep 29, 2014

Split into 4 PRs (including this one) to simplify review (hopefully). Dependency graph is as follows:

… …
| |
| * #1448
| |\
* | | #1733
| * | #1732
| | * #1731
| |/
|/
* #1492

@miri64 miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Sep 29, 2014
@rousselk
Copy link
Contributor

Maybe I'm going to say something stupid, but... doesn't this "one size fits all" API change make everything more complex? I kind of feel that the drivers/include/netdev/base.h file contains many elements that have few relations to each other (e.g.: very low-level details like hardware addresses in host byte order, along with high-level layers like TCP or CCN)...This "semantic mix" makes me uneasy.

Isn't there also a risk of increasing code size? Every layer will have to "cope" with elements they are not related to... It may cause a problem if one wants to use the whole network layer on limited devices (like Telosb for example)

However, since I couldn't attend netxork-related meetings, maybe were these issues already discussed and decisions taken?

@miri64
Copy link
Member Author

miri64 commented Sep 29, 2014

There is no semantic mixing IMHO. If a device does not support and option it can always return -ENOTSUP, as stated in the documentation (and thus does not increase code size that much). Yes, maybe it is complicated by giving you all these options, but keep in mind that this is the in most (if not all) use-cases just the backend for the network stack. Currently the later communicates with the devices through the multiplexing transceiver interface, but this just doesn't cut it because:

  1. it is historically grown from a cc1100-only interface.
  2. it supports only 1 device of each type.
  3. it just does not scale development-wise when we add more devices, since every new one will introduce another ifdef-path.

We know this is far from perfect, but I think this PR is a step in a better direction.

*
* @extends clist_node_t
*/
typedef struct __attribute__((packed)) netdev_hlist_t {
Copy link
Member

Choose a reason for hiding this comment

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

Why packed?

Copy link
Member Author

Choose a reason for hiding this comment

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

because otherwise there would be alignment problems if you put it in a static array (namly pktbuf in #1638 ;-))

@OlegHahm
Copy link
Member

OlegHahm commented Oct 1, 2014

tests/netdev on native:
Default netdev type and driver unknown!

@miri64
Copy link
Member Author

miri64 commented Oct 1, 2014

Yes, because there are no devices implemented in this PR. Please refer to #1732 or #1733.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 1, 2014

Well, okay, ACK.

@miri64
Copy link
Member Author

miri64 commented Oct 1, 2014

Squashed & rebased & waiting for Travis to be done

@OlegHahm
Copy link
Member

OlegHahm commented Oct 2, 2014

Kicked Travis.

@miri64
Copy link
Member Author

miri64 commented Oct 2, 2014

And go.

miri64 added a commit that referenced this pull request Oct 2, 2014
drivers: Introduce a general interface for network device drivers
@miri64 miri64 merged commit 0d33a00 into RIOT-OS:master Oct 2, 2014
@miri64 miri64 deleted the net-dev-base branch October 2, 2014 10:31
@miri64 miri64 mentioned this pull request Oct 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants