netdev: at86rf2xx: fix chan. length check#7427
netdev: at86rf2xx: fix chan. length check#7427OlegHahm wants to merge 1 commit intoRIOT-OS:masterfrom
Conversation
Channels can have up to 16 bits, but only 8 bit channels are valid for 802.15.4.
| assert(chan <= UINT8_MAX); | ||
| * 2.4 GHz band radios have different legal values. Here, we just | ||
| * take the first eight bits. */ | ||
| uint8_t chan = ((uint8_t *)value)[0]; |
There was a problem hiding this comment.
The API doc says uint16_t so it should be uint16_t (though I don't know if there are radio standards with channel numbers >255, at this point I'm against changing the API, because I don't see much benefit in it).
There was a problem hiding this comment.
(casting from uint16_t pointer to uint8_t pointer doesn't seem like a sane idea to me)
There was a problem hiding this comment.
The same thing is done in the driver anyway.
There was a problem hiding this comment.
I don't understand what you mean by that.
There was a problem hiding this comment.
| break; | ||
| case NETOPT_CHANNEL: | ||
| assert(len != sizeof(uint8_t)); | ||
| assert(len <= sizeof(uint16_t)); |
There was a problem hiding this comment.
I don't get it. Why should't I be allowed to pass a 8 bit channel if 802.15.4 channels always fit into 8 bit?
There was a problem hiding this comment.
Because if the user provides a 16-bit channel (or the shell_commands module for that matter) the cast in netdev_ieee802154 will cast a uint16_t pointer to a uint8_t pointer, discarding the least-significant byte, which on a big-endian platform would contain the actual value, so the value would always resolve to 0.
There was a problem hiding this comment.
Well, that seems to be broken for the last two years anyway, but I see your point. So, an upper layer API that uses 8 bit integers for radio channels need to upcast its parameter or what is the best solution for this?
There was a problem hiding this comment.
Yes, I think that is the best solution.
|
|
||
| case NETOPT_CHANNEL_PAGE: | ||
| assert(len != sizeof(uint8_t)); | ||
| assert(len <= sizeof(uint16_t)); |
| { | ||
| assert(len == sizeof(uint16_t)); | ||
| uint16_t chan = *((uint16_t *)value); | ||
| assert(len <= sizeof(uint16_t)); |
| /* real validity needs to be checked by device, since sub-GHz and | ||
| * 2.4 GHz band radios have different legal values. Here we only | ||
| * check that it fits in an 8-bit variabl*/ | ||
| assert(chan <= UINT8_MAX); |
There was a problem hiding this comment.
If chan is uint8_t this doesn't make sense.
|
Overall I would just change https://github.com/RIOT-OS/RIOT/pull/7427/files#diff-148e890332a08fcca786b1e362e7f970R387 and https://github.com/RIOT-OS/RIOT/pull/7427/files#diff-148e890332a08fcca786b1e362e7f970R399 to be |
|
Simple: replace the assert(len != sizeof(uint8_t);with assert(len == sizeof(uint16_t);for |
|
Provided alternative in #7488. |
|
I interpret #7488 (comment) in that way, that @OlegHahm agrees with me to close this PR. Re-open if you disagree. |
Channels can have up to 16 bits, but only 8 bit channels are valid for
802.15.4.