netdev2: some parameter type changes by miri64 · Pull Request #5495 · RIOT-OS/RIOT

miri64 · 2016-06-02T12:14:50Z

recv's len parameter and send's count parameter are currently int, though it wouldn't make any sense to have them < 0. We could add an assert every time this function is implemented, but I would rather vote to change the type.

As a bonus I made the type of recv's buf parameter generic - no reason for it to be a char and it might lead to unnecessary static casting requirements (already happening in https://github.com/RIOT-OS/RIOT/blob/master/tests/driver_at86rf2xx/recv.c#L37).

kaspar030 · 2016-06-02T12:18:34Z

drivers/include/net/netdev2.h

     *
     * @return nr of bytes sent, or <=0 on error
     */
-    int (*send)(netdev2_t *dev, const struct iovec *vector, int count);


the int comes from how POSIX uses iovecs (see man readv).

Is there a reason to stay with posix here? Not that it would make a difference, as no iovec count should be > wordsize/2 on our boards.

I can also use unsigned here, because it's not really a size. I think there is no reason to stay with POSIX here. A wrapper can cast if ever want to give readv access to netdev2 directly. Since the range of int >0 is smaller than the one of unsigned or size_t we should be on the save side.

I can also use unsigned here, because it's not really a size

did

kaspar030 · 2016-06-02T15:39:23Z

As a bonus I made the type of recv's buf parameter generic

I think I'm with you on that. Let's discuss that in #5497 and maybe change all over the place.

miri64 · 2016-06-02T18:44:59Z

BTW, I came to this error, because I tried to compile iotlab-m3 with -Wextra to finally merge #1121 ;-) (but gave up on it again...).

miri64 · 2016-06-05T11:19:53Z

Now that #5497 was discussed to an extend, what's with this PR?

miri64 · 2016-08-02T11:47:48Z

Fixes #5717 ;-)

jnohlgard · 2016-08-02T11:53:40Z

Oops, I guess I should have searched first!

jnohlgard · 2016-08-02T14:08:15Z

ACK for the change, needs fixing in the drivers though

miri64 · 2016-08-02T15:01:59Z

Done. Please Re-review.

miri64 · 2016-08-02T15:02:35Z

(per-driver commits can also be squashed down if you prefer it)

kaspar030 · 2016-08-02T15:05:59Z

(per-driver commits can also be squashed down if you prefer it)

Hmyeah, what about two commits, one for the netdev2 change, one for the drivers?

jnohlgard · 2016-08-02T16:48:25Z

re-ACK for the change. Murdock seem down though (502 Bad Gateway)..

kaspar030 · 2016-08-02T17:00:59Z

re-ACK for the change. Murdock seem down though (502 Bad Gateway)..

Should be back up. I'm migrating the Server it is running on. I didn't migrate murdock, yet, but was stupid enough to migrate the VPN vm that connects the build box.

miri64 · 2016-08-03T06:28:54Z

Done.

kaspar030 · 2016-08-03T06:33:33Z

@miri64 Yesterday murdock complained about wrong types in netdev2_test, did you fix those?

miri64 · 2016-08-03T07:08:45Z

@miri64 Yesterday murdock complained about wrong types in netdev2_test, did you fix those?

nope, but did so now.

miri64 · 2016-08-03T07:09:21Z

(also fixed a squashing error).

kaspar030 · 2016-08-03T07:44:10Z

cc2420 and cc2538 are missing adaption.

`len` and `count` are both values that should never go `< 0`, so instead of having to test this (in theory) every time the function is called (regardless of by assert or if its unnecessary code), I propose to change it to `size_t`. As a bonus I made the type of recv's buf parameter generic - no reason for it to be a char and it might lead to unnecessary static casting requirements

miri64 · 2016-08-03T08:01:26Z

Done and rebased to master to get those devices into this PR.

kaspar030 · 2016-08-03T08:28:31Z

Looking good, pls squash. (let's not merge before RC2 is out, because I think we can still just fast-forward).

miri64 · 2016-08-03T10:33:07Z

Done

miri64 · 2016-08-03T12:50:37Z

Murdock is happy.

kaspar030 · 2016-08-03T13:09:05Z

Well, as this is not pressing, should we wait until the release is out? Saves us the backporting edit of upcoming quickfixes...

miri64 · 2016-08-03T13:11:33Z

👍

miri64 added 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. labels Jun 2, 2016

miri64 assigned kaspar030 Jun 2, 2016

kaspar030 reviewed Jun 2, 2016
View reviewed changes

miri64 mentioned this pull request Aug 2, 2016

netdev2: send/recv length arguments should be unsigned #5717

Closed

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 2, 2016

jnohlgard 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 2, 2016

jnohlgard added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 2, 2016

miri64 force-pushed the netdev2/enh/type-changes branch from 5cce180 to 5b6fb12 Compare August 3, 2016 06:28

kaspar030 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 3, 2016

miri64 force-pushed the netdev2/enh/type-changes branch from 5b6fb12 to d2eebd6 Compare August 3, 2016 07:08

miri64 force-pushed the netdev2/enh/type-changes branch from d2eebd6 to 2d09c90 Compare August 3, 2016 08:00

cpu: drivers: adapt devices for netdev2 parameter type change

bd2429f

miri64 force-pushed the netdev2/enh/type-changes branch from 2d09c90 to bd2429f Compare August 3, 2016 10:33

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 3, 2016

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: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 3, 2016

miri64 merged commit 49521dc into RIOT-OS:master Aug 5, 2016

miri64 deleted the netdev2/enh/type-changes branch August 5, 2016 12:10

miri64 mentioned this pull request Aug 5, 2016

kw2xrf: adaption to netdev2 #5469

Closed

miri64 added this to the Release 2016.10 milestone Oct 14, 2016

Conversation

miri64 commented Jun 2, 2016

Uh oh!

kaspar030 Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

kaspar030 Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

miri64 Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

miri64 Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Jun 2, 2016

Uh oh!

miri64 commented Jun 2, 2016

Uh oh!

miri64 commented Jun 5, 2016

Uh oh!

miri64 commented Aug 2, 2016

Uh oh!

jnohlgard commented Aug 2, 2016 via email

Uh oh!

jnohlgard commented Aug 2, 2016

Uh oh!

miri64 commented Aug 2, 2016

Uh oh!

miri64 commented Aug 2, 2016

Uh oh!

kaspar030 commented Aug 2, 2016

Uh oh!

jnohlgard commented Aug 2, 2016

Uh oh!

kaspar030 commented Aug 2, 2016

Uh oh!

miri64 commented Aug 3, 2016

Uh oh!

kaspar030 commented Aug 3, 2016

Uh oh!

miri64 commented Aug 3, 2016

Uh oh!

miri64 commented Aug 3, 2016

Uh oh!

kaspar030 commented Aug 3, 2016

Uh oh!

miri64 commented Aug 3, 2016

Uh oh!

kaspar030 commented Aug 3, 2016

Uh oh!

miri64 commented Aug 3, 2016

Uh oh!

miri64 commented Aug 3, 2016

Uh oh!

kaspar030 commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaspar030 commented Aug 3, 2016 •

edited

Loading