netdev2: some parameter type changes #5495
Conversation
miri64
added
Area: network
| @@ -128,7 +128,7 @@ typedef struct netdev2_driver { | |||
| * | |||
| * @return nr of bytes sent, or <=0 on error | |||
| */ | |||
| int (*send)(netdev2_t *dev, const struct iovec *vector, int count); | |||
There was a problem hiding this comment.
the int comes from how POSIX uses iovecs (see man readv).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can also use unsigned here, because it's not really a size
did
I think I'm with you on that. Let's discuss that in #5497 and maybe change all over the place. |
|
BTW, I came to this error, because I tried to compile |
|
Now that #5497 was discussed to an extend, what's with this PR? |
|
Fixes #5717 ;-) |
|
Oops, I guess I should have searched first!
|
kaspar030
added
the
CI: ready for build
jnohlgard
added
the
CI: needs squashing
|
ACK for the change, needs fixing in the drivers though |
|
Done. Please Re-review. |
|
(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? |
|
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. |
jnohlgard
added
CI: ready for build
miri64
force-pushed
the
netdev2/enh/type-changes
branch
from
5cce180 to
5b6fb12
Compare
|
Done. |
kaspar030
removed
the
CI: needs squashing
|
@miri64 Yesterday murdock complained about wrong types in netdev2_test, did you fix those? |
miri64
force-pushed
the
netdev2/enh/type-changes
branch
from
5b6fb12 to
d2eebd6
Compare
nope, but did so now. |
|
(also fixed a squashing error). |
|
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
force-pushed
the
netdev2/enh/type-changes
branch
from
d2eebd6 to
2d09c90
Compare
|
Done and rebased to master to get those devices into this PR. |
|
Looking good, pls squash. (let's not merge before RC2 is out, because I think we can still just fast-forward). |
miri64
force-pushed
the
netdev2/enh/type-changes
branch
from
2d09c90 to
bd2429f
Compare
|
Done |
miri64
added
CI: ready for build
kaspar030
added
CI: ready for build
|
Murdock is happy. |
|
Well, as this is not pressing, should we wait until the release is out? Saves us the backporting edit of upcoming quickfixes... |
|
👍 |
recv'slenparameter andsend'scountparameter are currentlyint, 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'sbufparameter generic - no reason for it to be acharand 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).