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

xbee: fixed several bugs #4445

Merged
merged 8 commits into from
Apr 19, 2016
Merged

xbee: fixed several bugs #4445

merged 8 commits into from
Apr 19, 2016

Conversation

Yonezawa-T2
Copy link
Contributor

This PR fixes several bugs in XBee driver.

  • tx_lock is not unlocked
  • destination address field for broadcast packet is wrong
  • RSSI header field is incorrectly parsed
  • adds debug output of incoming/outgoing packets
  • short address should be disabled when long address is enabled
  • channel 0x1A is not supported by XBee-PRO
  • _set_addr destructs long address in memory
  • adds packet filtering functionality for debugging to emulate non-transitive network

See commit messages for details.

Tested on OS X with modifications of #4443 and #4444.

@OlegHahm OlegHahm added this to the Release 2016.03 milestone Dec 9, 2015
@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: drivers Area: Device drivers labels Dec 9, 2015
@Yonezawa-T2
Copy link
Contributor Author

XBee 802.15.4 manual is at http://ftp1.digi.com/support/documentation/90000982_S.pdf

See "Figure 23: TX Packet (16-bit address) frames" (p.95) for API format for destination address field for broadcast packet e99759f.

See "Figure 25: RX packet (64-bit address) frames" and "Figure 26: RX Packet (16-bit address) frames" (p.96) for RSSI header field 5f56ef2.

See Unicast Mode section (p.28) for behavior of short address and long address 5807145.

See "Table 11: XBee / XBee-PRO commands" (p.41) for valid channels of XBee and XBee-PRO 7c36311.

@haukepetersen
Copy link
Contributor

Awesome catches! Testing it now.

@haukepetersen
Copy link
Contributor

first quick test results:

  • the code changes look sane (especially the byte offsets for the TX and RX frames - I who's idea it was to skip byte 4...)
  • Sending works gread (with short and long addresses)
  • Receiving does not seem to work for me, but I am not sure if it is the driver of my setup... Looking into this

@Yonezawa-T2
Copy link
Contributor Author

I tested using branch https://github.com/Yonezawa-T2/RIOT/tree/xbee_border_router, which contains other fixes on GNRC, particularly #4446 affects sender's IPv6 address and ICMPv6 check sum.

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Feb 27, 2016
@OlegHahm
Copy link
Member

@Yonezawa-T2, can you rebase, please?
@haukepetersen, would you ACK?

@Yonezawa-T2
Copy link
Contributor Author

rebased.

@Yonezawa-T2
Copy link
Contributor Author

Should I split the PR into separate PRs? Some of the commits are bug fixes while others are enhancements.

@Yonezawa-T2
Copy link
Contributor Author

Reordered commits so that we can drop enhancement commits by git-reset.

@@ -289,9 +307,19 @@ static int _set_addr_len(xbee_t *dev, uint16_t *val, size_t len)
switch (*val) {
case 8:
dev->addr_flags |= XBEE_ADDR_FLAGS_LONG;

// disable short address
Copy link
Contributor

Choose a reason for hiding this comment

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

please use c-style comments...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@kYc0o kYc0o removed the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Apr 1, 2016
@Yonezawa-T2
Copy link
Contributor Author

Rebased.

@kYc0o
Copy link
Contributor

kYc0o commented Apr 16, 2016

ping @Yonezawa-T2 can you rebase again? sorry for the delay, I'll test this ASAP.

Destination address is at tx_buf[5] and tx_buf[6] rather than tx_buf[4] and
tx_buf[5].

Broadcast header is overridden by following code mistakenly.
@Yonezawa-T2
Copy link
Contributor Author

Rebased.

@kYc0o kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 18, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Apr 18, 2016

@kaspar030 can you look at the error? I cannot get why it's failing...

@OlegHahm
Copy link
Member

It's failing because two commits need squashing (according to their commit messages).

@kYc0o
Copy link
Contributor

kYc0o commented Apr 18, 2016

Oh... can we have a bit more output on that?

@kYc0o
Copy link
Contributor

kYc0o commented Apr 18, 2016

@Yonezawa-T2 so, can you squash? I think this is ready :) I'll do some more tests in the mean time

@OlegHahm
Copy link
Member

It's saying:

Pull request needs squashing:
37def13 SQUASH ME xbee: fixed comment style for "fixed RSSI header parsing"
4358c17 SQUASH ME xbee: fixed comment style for "disable short address..."

@kYc0o
Copy link
Contributor

kYc0o commented Apr 18, 2016

Oh yeah, but since there is no "error" or something like that, in the whole black output I wasn't unable to see it. Thanks!

@kYc0o
Copy link
Contributor

kYc0o commented Apr 18, 2016

Works like a charm. Waiting for squashing and Murdock afterwards.

XBee supports channels 0x0B-0x1A while XBee-PRO supports only 0x0C-0x17.
XBee sends short address even for `API_ID_TX_LONG_ADDR` if short address is
enabled. This results in check sum error of ICMPv6 since the IP address is
computed based on long address on the sender side while it is computed based on
short address on the receiver side.
`_set_addr` is called from `xbee_init` with lower bytes of the long address.
If `_set_addr` destructs the given address, the long address is also destructed.
When debugging multihop wireless network, it is useful to emulate non-transitive
network, that is, node A can communicate with B and B can communicate with C,
but A cannot communicate with C directly.

If `XBEE_DENIED_ADDRESSES`, which is an array of XBee long addresses, is
defined, packets from those addresses are dropped silently.

Example:
CFLAGS += "-DXBEE_DENIED_ADDRESSES={ 0x02, 0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc, 0xde, 0x02, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11 }"
@Yonezawa-T2
Copy link
Contributor Author

Yonezawa-T2 commented Apr 19, 2016

[del]Rebased.[/del]
[edit]Squashed.[/edit]

@kYc0o
Copy link
Contributor

kYc0o commented Apr 19, 2016

ACK and GO!

@kYc0o kYc0o merged commit f968ea0 into RIOT-OS:master Apr 19, 2016
@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 19, 2016
@miri64
Copy link
Member

miri64 commented Apr 19, 2016

Needs backport

@miri64
Copy link
Member

miri64 commented Apr 19, 2016

Nope, already in releas branch. Sorry.

@miri64 miri64 removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 19, 2016
@miri64
Copy link
Member

miri64 commented Apr 19, 2016

Arghs... this is so confusing.... no it is not in release branch... please provide a backport...

@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 19, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Apr 19, 2016

Yes it needs it :)

@miri64 miri64 removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 19, 2016
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants