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.

Sign up for GitHub

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

Jump to bottom

ipv6/nc: doc fix and unittest for unmanaged nc entries #4721

Merged
merged 2 commits into from
Feb 1, 2016
Merged

ipv6/nc: doc fix and unittest for unmanaged nc entries #4721

merged 2 commits into from
Feb 1, 2016

Conversation

cgundogan
Copy link
Member

This PR includes some doc fixes and a minor enhanced version of gnrc_ipv6_nc_is_reachable().
The old version of gnrc_ipv6_nc_is_reachable() also returned true for GNRC_IPV6_NC_STATE_UNMANAGED while the new enhanced version will return false for that case. I guess this shouldn't be a problem @authmillenon ?

Nevertheless, the new version eats slightly less ROM.

Ignore the below size comparisons

~/r/R/e/gnrc_networking > make info-buildsizes-diff OLDBIN=master-bin NEWBIN=enhance-bin
text    data    bss     dec     BOARD/BINDIRBASE

-4      0       0       -4      arduino-due
57304   224     19896   77424   master-bin
57300   224     19896   77420   enhance-bin

-8      0       0       -8      arduino-mega2560
80970   10706   13752   105428  master-bin
80962   10706   13752   105420  enhance-bin

-24     0       0       -24     avsextrem
132104  2332    95965   230401  master-bin
132080  2332    95965   230377  enhance-bin

-4      0       0       -4      cc2538dk
57244   220     19872   77336   master-bin
57240   220     19872   77332   enhance-bin

-16     0       0       -16     ek-lm4f120xl
57592   220     19872   77684   master-bin
57576   220     19872   77668   enhance-bin

0       0       0       0       f4vi1
59180   224     20000   79404   master-bin
59180   224     20000   79404   enhance-bin

0       0       0       0       fox
72880   220     22968   96068   master-bin
72880   220     22968   96068   enhance-bin

0       0       0       0       frdm-k64f
59328   1248    19864   80440   master-bin
59328   1248    19864   80440   enhance-bin

0       0       0       0       iotlab-m3
72608   220     22968   95796   master-bin
72608   220     22968   95796   enhance-bin

-4      0       0       -4      limifrog-v1
58432   220     20000   78652   master-bin
58428   220     20000   78648   enhance-bin

-4      0       0       -4      mbed_lpc1768
56904   220     19872   76996   master-bin
56900   220     19872   76992   enhance-bin

-16     0       0       -16     msba2
159304  2332    95965   257601  master-bin
159288  2332    95965   257585  enhance-bin

0       0       0       0       msbiot
59492   224     20032   79748   master-bin
59492   224     20032   79748   enhance-bin

0       0       0       0       mulle
77628   1284    23248   102160  master-bin
77628   1284    23248   102160  enhance-bin

-8      0       0       -8      nucleo-f091
59936   220     19872   80028   master-bin
59928   220     19872   80020   enhance-bin

-16     0       0       -16     nucleo-f303
57448   220     19880   77548   master-bin
57432   220     19880   77532   enhance-bin

0       0       0       0       nucleo-f401
59180   224     20000   79404   master-bin
59180   224     20000   79404   enhance-bin

-4      0       0       -4      nucleo-l1
58316   220     19992   78528   master-bin
58312   220     19992   78524   enhance-bin

-4      0       0       -4      openmote-cc2538
57152   220     19872   77244   master-bin
57148   220     19872   77240   enhance-bin

0       0       0       0       pba-d-01-kw2x
74848   1248    23304   99400   master-bin
74848   1248    23304   99400   enhance-bin

-24     0       0       -24     pttu
132312  2332    95965   230609  master-bin
132288  2332    95965   230585  enhance-bin

-4      0       0       -4      remote
57828   220     19872   77920   master-bin
57824   220     19872   77916   enhance-bin

-8      0       0       -8      saml21-xpro
60132   220     19992   80344   master-bin
60124   220     19992   80336   enhance-bin

-4      0       0       -4      samr21-xpro
77120   220     22984   100324  master-bin
77116   220     22984   100320  enhance-bin

-16     0       0       -16     slwstk6220a
57092   220     20000   77312   master-bin
57076   220     20000   77296   enhance-bin

0       0       0       0       stm32f3discovery
57448   220     19880   77548   master-bin
57448   220     19880   77548   enhance-bin

0       0       0       0       stm32f4discovery
59424   224     20016   79664   master-bin
59424   224     20016   79664   enhance-bin

-4      0       0       -4      udoo
57304   224     19896   77424   master-bin
57300   224     19896   77420   enhance-bin

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: doc Area: Documentation labels Jan 30, 2016
@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 30, 2016
@miri64
Copy link
Member

miri64 commented Jan 30, 2016

The old version of gnrc_ipv6_nc_is_reachable() also returned true for GNRC_IPV6_NC_STATE_UNMANAGED while the new enhanced version will return false for that case. I guess this shouldn't be a problem @authmillenon ?

Actually it is, because with this manually added entries (which are UNMANAGED) are never reachable...

@cgundogan cgundogan force-pushed the pr/nc/enhance branch 5 times, most recently from d8ffc36 to 4fb3ff8 Compare January 30, 2016 14:24
@cgundogan
Copy link
Member Author

Actually it is, because with this manually added entries (which are UNMANAGED) are never reachable...

ah you are right. I tried to reorder the macros to move the unmanaged bit further in the direction of the MSB,
but the ROM improvement wasn't so big anyomre. And for cortex-m0 platforms it even required 4 more bytes. So I removed this change and only include the doc fix and additionally included a unittest that checks whether the nc_entry is reachable when unmanaged.

@cgundogan cgundogan changed the title ipv6/nc: doc fix and minor enhancements ipv6/nc: doc fix and unittest for unmanaged nc entries Jan 30, 2016
@cgundogan cgundogan added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: tests Area: tests and testing framework and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jan 30, 2016
miri64
miri64 reviewed Feb 1, 2016
View reviewed changes
sys/include/net/gnrc/ipv6/nc.h
@@ -88,7 +88,6 @@ extern "C" {
#define GNRC_IPV6_NC_IS_ROUTER (0x08) /**< The neighbor is a router */

#define GNRC_IPV6_NC_TYPE_MASK (0x30) /**< Mask for neighbor cache state */
#define GNRC_IPV6_NC_TYPE_POS (4) /**< Shift of neighbor cache state */
Copy link
Member

Choose a reason for hiding this comment

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

Huh?

Copy link
Member Author

Choose a reason for hiding this comment

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

unused code. Or is that also intended as a reminder? (:

Copy link
Member

Choose a reason for hiding this comment

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

Really?

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay… probably legacy code from the beginning of the API. Can be removed indeed.

@miri64
Copy link
Member

miri64 commented Feb 1, 2016

Ack and go.

miri64 added a commit that referenced this pull request Feb 1, 2016
@miri64
Merge pull request #4721 from cgundogan/pr/nc/enhance
e6f3349 
ipv6/nc: doc fix and unittest for unmanaged nc entries
@miri64 miri64 merged commit e6f3349 into RIOT-OS:master Feb 1, 2016
@cgundogan cgundogan deleted the pr/nc/enhance branch February 1, 2016 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

Labels
Area: doc Area: Documentation Area: network Area: Networking Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgundogan @miri64