Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

at86rf2xx: port to netdev2 #4646

Merged
merged 3 commits into from
Mar 24, 2016
Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jan 16, 2016

Ports at86rf2xx to netdev2

Depends on #4645 (merged) and #4678 (closed).

@miri64 miri64 added Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers labels Jan 16, 2016
@miri64 miri64 added this to the Release 2016.03 milestone Jan 16, 2016
@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch from 3cdc688 to 274318b Compare January 16, 2016 19:14
@kaspar030
Copy link
Contributor

nice!

@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch 2 times, most recently from 30edbf4 to f8fc6a0 Compare January 18, 2016 20:33
@biboc
Copy link
Member

biboc commented Jan 20, 2016

Great job!

@thomaseichinger
Copy link
Member

Please consider miri64#25 to get this into a compiling state.

@miri64
Copy link
Member Author

miri64 commented Jan 20, 2016

Please be aware, that this PR is still WIP!

@Kijewski Kijewski added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 20, 2016
@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch 2 times, most recently from b2b5fd3 to 46724d2 Compare January 24, 2016 23:20
@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 24, 2016
@miri64
Copy link
Member Author

miri64 commented Jan 25, 2016

No more WIP and #4678 as additional dependency, but there is an issue: When I send an IEEE 802.15.4 frame from a master-build gnrc_networking the addressed node receives it just fine. However, when I send with the test application in this PR (or with gnrc_networking for that matter, too) the _irq_handler in at86rf2xx_netdev.c doesn't even fire... The packets seem to have the same format and the channel is also the same (otherwise the sniffer wouldn't receive), so I have no clue, what's going on.

Here's a dump from the situation: https://www.cloudshark.org/captures/3d0d88044e65.

  • 0x37ba (and 5a:55:44:5c:b9:4c37:ba respectively) is the node with code from master (packets are received by tests/driver_at86rf2xx of this branch)
  • 0x760e is the node with code from this branch (packets are not received by tests/driver_at86rf2xx of this branch [that's why there are resends in 21-24])

@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch from 46724d2 to ca0c25b Compare January 25, 2016 00:40
@thomaseichinger
Copy link
Member

@authmillenon Comparing packet 19 and 21 from your dump, the byte order of the PAN id seems the other way around.

@miri64
Copy link
Member Author

miri64 commented Jan 26, 2016

Ah thanks! Will fix with the redo of the ieee802154 intermediate layer.

@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch from ca0c25b to a662561 Compare January 28, 2016 12:26
@miri64
Copy link
Member Author

miri64 commented Jan 28, 2016

Rebased and adapted to current version of #4645.

}

void at86rf2xx_set_pan(at86rf2xx_t *dev, uint16_t pan)
{
dev->pan = pan;
dev->pan = byteorder_htons(pan);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the culprit of my previous reception problems, btw. If I assess the situation correctly the byteorder of the PAN is returned wrong on driver->get on most devices, it should be in network byte order but is in little-endian (this PR intentionally doesn't fix that, since it is not topic of this PR).
@haukepetersen @jfischer-phytec-iot @jremmert-phytec-iot @thomaseichinger can you assess the situation for kw2xrf and xbee?

Copy link
Member

Choose a reason for hiding this comment

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

isn't 802.15.4 PAN id supposed to be in little endian byte order (i.e. the opposite of "network byte order")?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes in the MAC header (which is the case here). I was talking about the handing of the option to the upper layers. But your comment solicited me to read the doc again and I was wrong: The PAN is supposed to be handed up in host byte order. I guess I was confused because the ifconfig config command of my implementation prints it address-like (as in "in network byte order", though it isn't). Will fix this.

Copy link
Member

Choose a reason for hiding this comment

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

Quote 802.15.4-2011 (emphasis mine):

5.2 MAC frame formats

This subclause specifies the format of the MAC frame (MPDU).
The frames in the MAC sublayer are described as a sequence of fields in a specific order. All frame formats in this subclause are depicted in the order in which they are transmitted by the PHY, from left to right, where the leftmost bit is transmitted first in time. Bits within each field are numbered from 0 (leftmost and least significant) to k – 1 (rightmost and most significant), where the length of the field is k bits. Fields that are longer than a single octet are sent to the PHY in the order from the octet containing the lowest numbered bits to the octet containing the highest numbered bits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@gebart: I know! Again: my comment wasn't about the order in the MAC frame (this was and still is right), but in the RIOT internal representation and storing according to the documentation.

@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch 2 times, most recently from 292b1b7 to e365d66 Compare January 28, 2016 22:32
@miri64
Copy link
Member Author

miri64 commented Jan 28, 2016

Rebased an squashed... Hopefully for the last time :-)

@miri64
Copy link
Member Author

miri64 commented Jan 28, 2016

Nope, the GNRC glue code is still buggy :(

@miri64
Copy link
Member Author

miri64 commented Mar 16, 2016

I provided a fix in #5096. Since the bug is already in master I decided to provide the fix separately. To test please merge it in here.

@miri64 miri64 added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 16, 2016
@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch from 4222a17 to 0c1b6ae Compare March 17, 2016 16:45
@miri64
Copy link
Member Author

miri64 commented Mar 17, 2016

#5096 was merged so I rebased to current master again.

@miri64
Copy link
Member Author

miri64 commented Mar 18, 2016

ping?

@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch from 0c1b6ae to 14c6ba3 Compare March 18, 2016 09:31
@miri64
Copy link
Member Author

miri64 commented Mar 18, 2016

Rebased and adapted to changes from #4862


netdev->driver = &at86rf2xx_driver;
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't (shouldn't) this be done by the caller?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, maybe not, but this casting back seems strange to me.

Copy link
Member

Choose a reason for hiding this comment

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

But I guess

dev->netdev->netdev->driver = &at86rf2xx_driver;

isn't very pretty, either.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other netdev2s are also the setting the driver in the setup and it makes more sense to me. Why expose the driver when it other stuff needs to be set for the device too.

It was your idea to use this kind of inheritance ;P

Copy link
Member Author

Choose a reason for hiding this comment

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

(and I warned about the implications)

@OlegHahm
Copy link
Member

I would say: ready to be squashed and let's have Murdock taking a look then.

@miri64 miri64 force-pushed the at86rf2xx/api/netdev2 branch from 14c6ba3 to dff0c52 Compare March 23, 2016 16:45
@miri64
Copy link
Member Author

miri64 commented Mar 23, 2016

Squashed.

@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 23, 2016
@OlegHahm OlegHahm added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 23, 2016
@OlegHahm
Copy link
Member

I am fine with this PR, Murdock is fine, what about you, @haukepetersen?

@haukepetersen
Copy link
Contributor

I am good

@haukepetersen
Copy link
Contributor

so go!

@haukepetersen haukepetersen merged commit 2510fe0 into RIOT-OS:master Mar 24, 2016
@OlegHahm
Copy link
Member

Yeah! Nice work, @authmillenon!

@emmanuelsearch
Copy link
Member

+1 it's the beginning of a new era ;)

@kYc0o
Copy link
Contributor

kYc0o commented Mar 24, 2016

finally! :D congrats @authmillenon ;-)

@miri64 miri64 deleted the at86rf2xx/api/netdev2 branch March 24, 2016 14:51
@miri64
Copy link
Member Author

miri64 commented Mar 24, 2016

Thank you all for the review! :-)

@zhuoshuguo
Copy link
Contributor

Congratulations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Development

Successfully merging this pull request may close these issues.