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

frame802154: comply with IEEE 802.15.4-2015 on PAN ID Field Handling #1914

Merged
merged 2 commits into from Dec 8, 2016

Conversation

yatch
Copy link
Contributor

@yatch yatch commented Oct 31, 2016

Summary

frame802154_has_panid() is not fully compliant with IEEE 802.15.4-2015 [1] for frames of the frame version 0b10. This is because the current implementation is based on Table 2a in IEEE 802.15.4e-2012 [2], which "cannot specify all aspects of how the PAN ID Compression field is to be used", according to a document [3].

For example, in the case where the source address is an extended address, the destination is a short address, and the PAN ID Compression is 0, the Source PAN ID is present as per IEEE 802.15.4-2015. On the other hand, as per IEEE 802.15.4e-2012, the Source PAN ID is not present, which is the case where the Source address is present, the Destination address is present, and PAN ID Compression is 0.

This PR aims for rewriting frame802154_has_panid() with reference to Table 7-2 of IEEE 802.15.4-2015.

This is related to #1723.

[1] IEEE 802.15.4-2015 https://standards.ieee.org/findstds/standard/802.15.4-2015.html
[2] IEEE 802.15.4e-2012 https://standards.ieee.org/findstds/standard/802.15.4e-2012.html
[3] Proper PAN ID Field Settings for 802.15.4-2015: https://mentor.ieee.org/802.15/dcn/15/15-15-0911-01-0mag-proper-pan-id-field-settings-for-802-15-4-2015.docx

Notice

This may affect interopeability with previous versions of Contiki, although keeping the current implementation could cause an issue on communication with an other implementation.

Test

The test code has moved under "regression-tests/25-ieee802154/". You can run the test as follows:

$ cd regression-tests/25-ieee802154
$ make
Running test 01-panid-handling with random Seed 1: ...... OK

The following information is obsolete:

<obsolete>
With examples/unit-test/frame802154, you can test frame802154_has_panid(). Run make under the directory. You'll have an executable file named test.native.

Here is the result of test.native with the current implementation of 'frame802154_has_panid()`:

$ ./test.native
Contiki-3.x-2925-g672d603 started with IPV6, RPL
Rime started with address 1.2.3.4.5.6.7.8
MAC nullmac RDC nullrdc NETWORK sicslowpan
Tentative link-local IPv6 address fe80:0000:0000:0000:0302:0304:0506:0708
Run unit-test

---
............
SUCEEDED - PAN ID Cmpr Handing (frame-ver: 0b00)
............
SUCEEDED - PAN ID Cmpr Handing (frame-ver: 0b01)
........E
FAILED   - PAN ID Cmpr Handing (frame-ver: 0b10): at test index 8

After applying the patch, the test called "PAN ID Cmpr Handing (frame-ver: 0b10)" passed.

$ ./test.native
Contiki-3.x-2928-ge68a530 started with IPV6, RPL
Rime started with address 1.2.3.4.5.6.7.8
MAC nullmac RDC nullrdc NETWORK sicslowpan
Tentative link-local IPv6 address fe80:0000:0000:0000:0302:0304:0506:0708
Run unit-test

---
............
SUCEEDED - PAN ID Cmpr Handing (frame-ver: 0b00)
............
SUCEEDED - PAN ID Cmpr Handing (frame-ver: 0b01)
..................
SUCEEDED - PAN ID Cmpr Handing (frame-ver: 0b10)

</obsolete>

@g-oikonomou
Copy link
Contributor

This seems sensible to me at first glance. I'll have to read the references provided here, but perhaps @simonduq @nvt can comment?

(fcf->dest_addr_mode != FRAME802154_LONGADDRMODE ||
fcf->src_addr_mode != FRAME802154_LONGADDRMODE)) {
src_pan_id = 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

the above statement is very difficult to read and match to the standard. Could you simply expand it in the same style as the first statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@simonduq
Copy link
Member

Thanks, looks good, except for the src_pan_id statement which is really hard to maintain. See comment.

Also, great that you provide a unit test. Would be awesome if you could add it to Travis (automated tests directly from github)

@yatch
Copy link
Contributor Author

yatch commented Nov 23, 2016

OK, I'll try the Travis thing; in that case, I should move the test code under the regression-test directory, shouldn't I? Which directory is the best to put them? Or better to create a new one with a name like "25-ieee802154"? Hmm, there are two directories which have "23-" prefix. These numbers may have some meanings...?

@simonduq
Copy link
Member

No meaning in the numbers, this is just for ordering. The duplicate 23 is a mistake, probably from two PRs that were issued and merged concurrently. We should fix this.

For yours, I would add "25-ieee802154 yes! Thanks :)

@yatch yatch force-pushed the pr/802154-panid-handling branch 6 times, most recently from 3ae3be9 to 19bbd75 Compare November 23, 2016 17:09
@yatch
Copy link
Contributor Author

yatch commented Nov 23, 2016

I've updated this PR:

  • addressed the comment by @simonduq ; rewrote the src_pan_id statement
  • moved the unit-test under the regression-tests directory, refined it to be executed by Cooja, and made the test done by Travis

I did several force-pushes over a short time; they may have bothered you.... Sorry about that...

@simonduq
Copy link
Member

Excellent! 👍 for me (assuming Travis passes)

@simonduq
Copy link
Member

setting timeout for merging

@nvt
Copy link
Member

nvt commented Dec 5, 2016

Nice to see such a complete PR with references and a unit test! Could you clarify whether the changed behavior is intended for the same frame version as the previous code? Now the 802.15.4-2015 table is applied when fcf->frame_version == FRAME802154_IEEE802154E_2012.

@simonduq
Copy link
Member

simonduq commented Dec 5, 2016

My two cents would be that we keep FRAME802154_IEEE802154E_2012 as version 0b10, because the version was introduced in 15.4e-2012. Now 15.4-2015 added some fixes, but the frame version is still 0b10.

@yatch what do you think?

This patch changes the behaviors of frame802154_has_panid() for frames
of frame version 0b10 so that it complies with the PAN ID Field Handling
specification in IEEE 802.15.4-2015. For the other frame versions, 0b00
and 0b01, no change is made in frame802154_has_panid().

For more information, please refer to:
contiki-os#1914
@yatch
Copy link
Contributor Author

yatch commented Dec 5, 2016

@nvt Thanks for the comment. I added some text to the commit log for the clarification (and rebased). Table 7-2 of IEEE 802.15.4-2015 that was referred in the code is for frame version 0b10, which is the value of FRAME802154_IEEE802154E_2012. "IEEE 802.15.4-2015" in the phrase of "Table 7-2 of IEEE 802.15.4-2015" is not the frame version but something like document version. It may be confusing.

@simonduq Speaking of FRAME802154_IEEE802154E_2012, I agree with you; keeping it as it is in this PR.

As a matter of fact, I was about to change it to FRAME802154_IEEE802154 or FRAME802154_IEEE802154_2015 to see Table 7-4 of IEEE 802.15.4-2015 which provides the summary of frame versions: "IEEE Std 802.15.4-2003", "IEEE Std 802.15.4-2006", and "IEEE 802.15.4." But, on second thought, I did not. I thought it needs a separate discussion on changing FRAME802154_IEEE802154E_2012 in the community.

FRAME802154_IEEE802154E_2012 has no practical issue, by the way :-)

@nvt
Copy link
Member

nvt commented Dec 8, 2016

👍

@nvt nvt merged commit d133ed8 into contiki-os:master Dec 8, 2016
@yatch yatch deleted the pr/802154-panid-handling branch December 8, 2016 17:43
alexrayne pushed a commit to alexrayne/contiki that referenced this pull request Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants