Skip to content

sys: add improved network device API#3210

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
kaspar030:add_netdev2
Aug 26, 2015
Merged

sys: add improved network device API#3210
miri64 merged 1 commit intoRIOT-OS:masterfrom
kaspar030:add_netdev2

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Jun 16, 2015

@haukepetersen and I tried to find a generally usable but maximally minimized "network device interface".

This is our work in progress. Please comment.

@kaspar030 kaspar030 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Jun 16, 2015
@kaspar030 kaspar030 mentioned this pull request Jun 16, 2015
Closed
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 16, 2015

Can we just call it ng_netdev and replace the old one as soon as this gets working state? Two versions of netdev and the transceiver API already confuse new people. Three versions of netdev will only increase this.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Jun 16, 2015

@authmillenon of course, that's the plan. I went for netdev2 because I didn't want the name clash while developing. Also ng_* seems gnrc_* related to me...

Kijewski
Kijewski reviewed Jun 16, 2015
View reviewed changes
sys/include/net/netdev_events.h Outdated
Copy link
Copy Markdown
Contributor

@Kijewski Kijewski Jun 16, 2015

Choose a reason for hiding this comment

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

Why is this a file on its own?

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Jun 16, 2015

Choose a reason for hiding this comment

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

the old ng_netdev has the same events. I didn't want to copy them as long as both interfaces exist.

@haukepetersen haukepetersen mentioned this pull request Jun 16, 2015
Merged
@kaspar030 kaspar030 force-pushed the add_netdev2 branch 4 times, most recently from 2dc75cb to 3706aa0 Compare June 18, 2015 11:02
@kaspar030 kaspar030 force-pushed the add_netdev2 branch 3 times, most recently from 349f5d1 to eeff25b Compare July 8, 2015 21:44
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Aug 21, 2015

  • rebased, adapted to new gnrc structure

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 21, 2015
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Aug 21, 2015

Is there a reason not to put this into drivers/include? I'm always confused when I look for the current netdev header and cannot find it there.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Aug 21, 2015 via email

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Aug 21, 2015

Apart from the location of the file, I'm fine with the proposal.

@kaspar030 kaspar030 force-pushed the add_netdev2 branch 2 times, most recently from d88418c to 83eb1e2 Compare August 21, 2015 14:29
@kaspar030 kaspar030 mentioned this pull request Aug 21, 2015
Merged
miri64
miri64 reviewed Aug 21, 2015
View reviewed changes
drivers/include/net/netdev2.h
Copy link
Copy Markdown
Member

@miri64 miri64 Aug 21, 2015

Choose a reason for hiding this comment

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

Shouldn't this return an iovec too?

Copy link
Copy Markdown
Member

@miri64 miri64 Aug 21, 2015

Choose a reason for hiding this comment

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

(as an out parameter) That would make the receive function you are having such problems with in #3683 much easier, because you don't need to copy twice.

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Aug 21, 2015

Choose a reason for hiding this comment

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

Why? The driver has the whole packet available at the time recv is called.

Copy link
Copy Markdown
Member

@miri64 miri64 Aug 21, 2015

Choose a reason for hiding this comment

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

Nvm, I did not read on the behavior of the return value.

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Aug 23, 2015

I think it would make sense to add on and off methods to the driver struct. It will be necessary to have an abstraction for turning an interface off and on when using a power saving MAC protocol or duty cycling the network interface manually.
An alternative would be to implement power modes via netopt get/set. Any opinions? pros, cons?

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Aug 23, 2015

I would go with get/set here. IMO this has two advantages: 1. it keeps the interface slim and 2. it can be used more fine-grained, e.g. if the transceiver device supports multiple sleep modes.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Aug 26, 2015

  • rebased

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Aug 26, 2015

Is this really still WIP?

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Aug 26, 2015

Btw. I think you can squash. ACK

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed 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: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Aug 26, 2015
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Aug 26, 2015

Regarding on/off in the interface, I'd say, let's go with set/get first. If it turns out to be a performance problem, we can easily extend the interface.

miri64
miri64 reviewed Aug 26, 2015
View reviewed changes
drivers/include/net/netdev2.h Outdated
Copy link
Copy Markdown
Member

@miri64 miri64 Aug 26, 2015

Choose a reason for hiding this comment

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

net otherwise you are adding it to itself.

Copy link
Copy Markdown
Member

@OlegHahm OlegHahm Aug 26, 2015

Choose a reason for hiding this comment

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

Shouldn't it be rather @ingroup drivers?

Copy link
Copy Markdown
Member

@miri64 miri64 Aug 26, 2015

Choose a reason for hiding this comment

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

Right. Sorry about that. But then the group should also be called drivers_netdev2.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Aug 26, 2015

-force-push-amended doxygen group fix

miri64
miri64 reviewed Aug 26, 2015
View reviewed changes
drivers/include/net/netdev2.h Outdated
Copy link
Copy Markdown
Member

@miri64 miri64 Aug 26, 2015

Choose a reason for hiding this comment

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

Doxygen is complaining about this line, but technically its already documented in l81-86. Merge anyway?

Copy link
Copy Markdown
Member

@OlegHahm OlegHahm Aug 26, 2015

Choose a reason for hiding this comment

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

I'm not in favour of introducing more doxygen warnings.

Copy link
Copy Markdown
Member

@OlegHahm OlegHahm Aug 26, 2015

Choose a reason for hiding this comment

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

And I know @kaspar030 is gonna hate me for this comment.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Aug 26, 2015

double green!

miri64 added a commit that referenced this pull request Aug 26, 2015
@miri64
Merge pull request #3210 from kaspar030/add_netdev2
4cf8cbc 
sys: add improved network device API
@miri64 miri64 merged commit 4cf8cbc into RIOT-OS:master Aug 26, 2015
@miri64
Copy link
Copy Markdown
Member

miri64 commented Aug 26, 2015

\o/

@kaspar030 kaspar030 deleted the add_netdev2 branch August 26, 2015 17:47
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Aug 26, 2015

Yeah! Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@haukepetersen haukepetersen

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kaspar030 @miri64 @OlegHahm @jnohlgard @Kijewski @haukepetersen