at86rf2xx: fix option setting #5227

miri64 · 2016-04-01T12:05:31Z

haukepetersen · 2016-04-01T12:56:46Z

seems to me like you broke the code: if an option is set successfully, the res var is still set to -ENOTSUP...

miri64 · 2016-04-01T12:57:47Z

That's the idea, so it jumps into https://github.com/RIOT-OS/RIOT/pull/5227/files#diff-148e890332a08fcca786b1e362e7f970R572.

miri64 · 2016-04-01T12:58:31Z

Ah wait, for some instances you are right.

miri64 · 2016-04-01T13:05:17Z

Okay, now I replaced the instances where the values are just invalid for the scope of the device with -EINVAL to prevent jumping into the netdev2_ieee802154_set() function. It seems to me the more correct error code anyway.

haukepetersen · 2016-04-01T13:18:02Z

looks better :-)

miri64 · 2016-04-01T13:30:32Z

Rebased to current master.

haukepetersen · 2016-04-01T16:07:59Z

drivers/at86rf2xx/at86rf2xx_netdev.c

@@ -433,7 +433,7 @@ static int _set(netdev2_t *netdev, netopt_t opt, void *val, size_t len)
                uint8_t chan = ((uint8_t *)val)[0];
                if (chan < AT86RF2XX_MIN_CHANNEL ||
                    chan > AT86RF2XX_MAX_CHANNEL) {
-                    res = -ENOTSUP;
+                    res = -EINVAL;
                    break;
                }
                at86rf2xx_set_chan(dev, chan);


maybe I am confused, but something still seems off: initially, res is set to -ENOTSUP. If the given channel is invalid, we set it to -EINVAL, so far so good. But if the channel is valid, we do not change res, so when we get to the break below, res is still set to -ENOTSUP. This will trigger the call to netdev2_ieee802154_set(...) at the end of the function. Is this intentional?

I know GitHub is blending it out, but it is commented on in the line below. This PR does not change this behavior since the chan field in the netdev2_ieee802154_t struct still needs to be set.

I think res should be getting set here on a valid channel change, since netdev2_ieee802154_set() doesn't handle NETOPT_CHANNEL...

True. Fixed.

Nice. So perhaps this comment is redundant now: https://github.com/authmillenon/RIOT/blob/253dbe225366efcb6506ccc01a5fb19ab7557b47/drivers/netdev2_ieee802154/netdev2_ieee802154.c#L226

aeneby · 2016-04-01T20:54:39Z

Could I also suggest that, for this PR, we remove the existing code in the at86rf2xx driver that should be getting done in netdev2_ieee802154_set()? I'm referring to lines like

RIOT/drivers/at86rf2xx/at86rf2xx_getset.c

Line 128 in ceab9bd

dev->netdev.long_addr[i] = (uint8_t)(addr >> (i * 8));

but there's probably more.

miri64 · 2016-04-01T22:50:50Z

Could I also suggest that, for this PR, we remove the existing code in the at86rf2xx driver that should be getting done in netdev2_ieee802154_set()?

I'm all for it, but this is touching legacy code by @haukepetersen, that was intended to keep the driver independent from netdev. Are you alright with this change, @haukepetersen?

aeneby · 2016-04-09T21:57:47Z

Also, I thinkNETOPT_AUTOACK is being handled incorrectly by netdev2_ieee802154_{g,s}et(). It's getting/setting the NETDEV2_IEEE802154_ACK_REQ flag, which as I understand it is not the same thing as AUTOACK.

aeneby · 2016-04-09T22:13:58Z

The documentation here needs updating too, given the previous commits:

RIOT/drivers/include/net/netdev2/ieee802154.h

Line 129 in d171c96

* The setting of netdev2_ieee802154_t::chan is omitted since the legality of

aeneby · 2016-04-10T23:09:06Z

Also, I think NETOPT_AUTOACK is being handled incorrectly by netdev2_ieee802154_{g,s}et().

#5287

aeneby · 2016-04-12T18:41:53Z

@haukepetersen what do you think about the PR in its current state?

haukepetersen · 2016-04-13T12:49:30Z

drivers/netdev2_ieee802154/netdev2_ieee802154.c

+        case NETOPT_CHANNEL:
+            /* validity needs to be checked by device, since sub-GHz and 2.4 GHz
+             * band radios have different legal values */
+            if (len > sizeof(dev->chan)) {


use assert here instead...

The rest of the file doesn't use assert either yet. Let's do this in a seperate follow-up.

haukepetersen · 2016-04-13T12:54:21Z

in general I am ok with this, though I think there is still a bug in the behavior: the the set function, netdev2_ieee802154_set() should always be called, even if the option was already addressed. If this is not the case, we will change e.g. the address in the device, but we will not update the netdev2_ieee802154 state struct accordingly...

aeneby · 2016-04-13T18:40:19Z

But there are some options for which calling netdev2_ieee802154_set() would be redundant, since they can only be handled by the driver itself. NETOPT_STATE, for example, which is presumably why this option is not included as a case. Calling netdev2_ieee802154_set() with NETOPT_STATE will return -ENOTSUP all the way back up the call chain.

haukepetersen · 2016-04-14T12:25:57Z

exactly, and that is the problem of the current code. Just have a look at my WIP PR for the CC2420 driver https://github.com/RIOT-OS/RIOT/pull/5314/files#diff-9557a9fbe33e77bb84867da9d97c67c9R808. This way both are called, but the -ENOTSUP is only returned if none them know what to do with the data..

aeneby · 2016-04-14T19:28:48Z

But that approach also has problems - what if an application tries to set an invalid channel, for example? Since only the driver can know if the requested channel is valid, it will be set to an invalid value in the netdev2_ieee802154 struct by netdev2_ieee802154_set(), and then cc2420_set_chan() will presumably return an error code, leaving things in an inconsistent state.

Since some options cannot be dealt with at all by netdev2_ieee802154_set() (NETOPT_STATE, NETOPT_AUTOACK, etc), why do you feel that "the set function, netdev2_ieee802154_set() should always be called, even if the option was already addressed"?

aeneby · 2016-04-14T19:45:04Z

If I can be selfish for a second :) I'm really only interested in the changes to netdev2_ieee802154.c here since I can't test #5291 properly without the NETOPT_CHANNEL support. @authmillenon, how would you feel about separating this code out into a new PR while the at86rf2xx-specific stuff is discussed? The changes are orthogonal, and it would also keep this PR specific to the at86rf2xx driver.

haukepetersen · 2016-04-15T08:24:30Z

Ok, I see your needs. I didn't really want to go into the deeper architectural discussion here (as the hole concept is currently flawed as is). So for now, I would be fine with taking this PR as is, if we fix it, so that certain params (as address, pan id) are also passed to the netdev2_ieee.. layer

aeneby · 2016-04-15T11:20:31Z

if we fix it, so that certain params (as address, pan id) are also passed to the netdev2_ieee.. layer

They already are...

miri64 · 2016-04-17T12:23:55Z

@authmillenon, how would you feel about separating this code out into a new PR while the at86rf2xx-specific stuff is discussed?

Since at86rf2xx is using the netdev2_ieee802154, not changing the at86rf2xx code untouched in this PR would leave it faulty in master. So I'm not feeling very positive about this idea.

haukepetersen · 2016-04-20T12:40:30Z

ok, never mind, the issue I was referring to does not exist, so I am ok with this PR.

haukepetersen · 2016-04-20T14:05:43Z

something is off when setting a channel:

2016-04-20 16:05:20,451 - INFO # > ifconfig 7 set chan 22
2016-04-20 16:05:20,452 - INFO # error: unable to set channel

haukepetersen · 2016-04-20T14:06:02Z

but the channel is actually set, so the return value is off?

miri64 · 2016-04-20T14:28:43Z

Fixed. Please retest.

haukepetersen · 2016-04-20T14:32:52Z

that did the job. Looks good now -> ACK once squashed and Murock gives a green

miri64 · 2016-04-20T14:32:59Z

Done

aeneby · 2016-04-20T21:30:23Z

@authmillenon @haukepetersen there is no way this has been tested properly. You declare chan as uint16_t and then immediately assert "chan <= UINT8_MAX" when setting the channel.

miri64 · 2016-04-20T21:50:22Z

Yes, we tested it properly: netopt expects channel of type uint16_t while the channel field in IEEE 802.15.4 is uint8_t. That's why the assert is there.

miri64 · 2016-04-20T21:50:53Z

The driver has to make sure the channel is in the correct range, as the comment states.

aeneby · 2016-04-20T22:19:40Z

But then if netdev2_ieee802154_set is called with NETOPT_CHANNEL and an 8-bit value, the cast to uint16_t will read in garbage and the assertion will likely fail. Why aren't we checking len in that case?

aeneby · 2016-04-20T22:24:37Z

Or just declaring chan as uint8_t?

miri64 · 2016-04-20T22:44:07Z

But then if netdev2_ieee802154_set is called with NETOPT_CHANNEL and an 8-bit value, the cast to uint16_t will read in garbage and the assertion will likely fail. Why aren't we checking len in that case?

Under the given test conditions NETOPT_CHANNEL is not given as an 8-bit value (since it is documented that it should be a 16-bit value). True, the check should probably better be != but I fixed that in #5327 already.

Or just declaring chan as uint8_t?

IEEE 802.15.4 isn't the only radio standard we support and apparently (pure conjecture, given that I did not define the type for that NETOPT) there are radio standards with channel value > 255.

aeneby · 2016-04-20T22:49:30Z

OK - I understand. It just seemed weird to require a 16-bit value, only to throw 8 bits of it away, especially on memory-constrained devices. Perhaps we could check if NETOPT_CHANNEL really does need to be 16-bit.

miri64 · 2016-04-21T11:36:58Z

Since this 16-bit value is typically allocated on the stack I would argue that keeping the variety of types in NETOPT at a small level (i. e. trying to use the most common one) might even be beneficial for memory constraints, since you only have to allocate a small number of variables to handle multiple options (since you can reuse them then).

miri64 assigned haukepetersen Apr 1, 2016

miri64 added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 1, 2016

miri64 added this to the Release 2016.04 milestone Apr 1, 2016

miri64 force-pushed the at86rf2xx/fix/set branch from 7eceddf to 28ef573 Compare April 1, 2016 13:30

haukepetersen reviewed Apr 1, 2016
View reviewed changes

haukepetersen reviewed Apr 13, 2016
View reviewed changes

miri64 mentioned this pull request Apr 17, 2016

netdev2_ieee802154: replace overflow checks with asserts #5327

Merged

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 Apr 20, 2016

miri64 force-pushed the at86rf2xx/fix/set branch from 4c774b9 to 0fa5b81 Compare April 20, 2016 14:32

miri64 and others added 2 commits April 20, 2016 16:35

at86rf2xx: fix option setting

7b78775

netdev2_ieee802154: fix channel setting

0fa5b81

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 Apr 20, 2016

miri64 merged commit cedf7d8 into RIOT-OS:master Apr 20, 2016

miri64 deleted the at86rf2xx/fix/set branch April 20, 2016 15:33

miri64 mentioned this pull request Apr 20, 2016

at86rf2xx: fix option setting (backport) #5375

Merged

at86rf2xx: fix option setting #5227

at86rf2xx: fix option setting #5227

Conversation

miri64 commented Apr 1, 2016

haukepetersen commented Apr 1, 2016

miri64 commented Apr 1, 2016

miri64 commented Apr 1, 2016

miri64 commented Apr 1, 2016

haukepetersen commented Apr 1, 2016

miri64 commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneby commented Apr 1, 2016

miri64 commented Apr 1, 2016

aeneby commented Apr 9, 2016

aeneby commented Apr 9, 2016

aeneby commented Apr 10, 2016

aeneby commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Apr 13, 2016

aeneby commented Apr 13, 2016

haukepetersen commented Apr 14, 2016

aeneby commented Apr 14, 2016

aeneby commented Apr 14, 2016

haukepetersen commented Apr 15, 2016

aeneby commented Apr 15, 2016

miri64 commented Apr 17, 2016

haukepetersen commented Apr 20, 2016

haukepetersen commented Apr 20, 2016

haukepetersen commented Apr 20, 2016

miri64 commented Apr 20, 2016

haukepetersen commented Apr 20, 2016

miri64 commented Apr 20, 2016

aeneby commented Apr 20, 2016

miri64 commented Apr 20, 2016

miri64 commented Apr 20, 2016

aeneby commented Apr 20, 2016

aeneby commented Apr 20, 2016

miri64 commented Apr 20, 2016

aeneby commented Apr 20, 2016

miri64 commented Apr 21, 2016