Skip to content

ipv6_nc: prepare for NDP#2908

Merged
miri64 merged 4 commits intoRIOT-OS:masterfrom
miri64:ipv6_nc/enh/opt
May 13, 2015
Merged

ipv6_nc: prepare for NDP#2908
miri64 merged 4 commits intoRIOT-OS:masterfrom
miri64:ipv6_nc/enh/opt

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented May 3, 2015

NDP needs still some adaptions to the neighbor cache to make its usage more managable.

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. NSTF labels May 3, 2015
@miri64 miri64 added this to the Network Stack Task Force milestone May 3, 2015
@miri64 miri64 mentioned this pull request May 3, 2015
Merged
@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 4, 2015

looks good?

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented May 5, 2015

That's an ACK?

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented May 5, 2015 

/home/travis/build/RIOT-OS/RIOT/sys/net/network_layer/ng_ipv6/nc/ng_ipv6_nc.c: In function ‘ng_ipv6_nc_add’:
/home/travis/build/RIOT-OS/RIOT/sys/net/network_layer/ng_ipv6/nc/ng_ipv6_nc.c:51:9: warning: return makes pointer from integer without a cast [enabled by default]
         return -EFAULT;

@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 5, 2015

@OlegHahm I wasn't sure if I could ACK this.. In hindsight, probably a good thing ^^

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented May 5, 2015

Actually, it would be great if you could review this (after @authmillenon fixed the problems), because I won't have much time for reviews until end of May.

@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 5, 2015

@OlegHahm Sure, I'll do my best!

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 6, 2015

Addressed comments

@OlegHahm OlegHahm assigned Lotterleben and unassigned OlegHahm May 6, 2015
@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 7, 2015

Thanks.. Could you please squash so we can see what travis thinks? :)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 8, 2015

Travis does not even like it now. Will look into it :D

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 8, 2015

Fixed issues that Travis had.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 8, 2015

(and used ng_netif_addr_to_str() for output as a bonus ;-))

@miri64 miri64 mentioned this pull request May 10, 2015
Closed
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 11, 2015

Sorry for all these "last minute" changes /o\ I'm done now. Please re-review.

@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 12, 2015

No problem, I was a bit idle anyway, so thanks for the buttkicking ;)

Lotterleben
Lotterleben reviewed May 12, 2015
View reviewed changes
sys/include/net/ng_ipv6/nc.h Outdated
Copy link
Copy Markdown
Member

@Lotterleben Lotterleben May 12, 2015

Choose a reason for hiding this comment

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

ultra-nitpick: there is an h missing in "neigbor" :D

Copy link
Copy Markdown
Member Author

@miri64 miri64 May 12, 2015

Choose a reason for hiding this comment

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

Pointing out spelling errors is not nitpicky ;)

Copy link
Copy Markdown
Member

@Lotterleben Lotterleben May 12, 2015

Choose a reason for hiding this comment

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

Well, we had this discussion about not delaying PRs with tiny issues, so ;)

@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 12, 2015

(I don't expect you to address my nitpicks, I'm writing them down just in case )

Lotterleben
Lotterleben reviewed May 12, 2015
View reviewed changes
sys/include/net/ng_ipv6/nc.h Outdated
Copy link
Copy Markdown
Member

@Lotterleben Lotterleben May 12, 2015

Choose a reason for hiding this comment

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

Also, why do some of the comments get a @brief?

Copy link
Copy Markdown
Member Author

@miri64 miri64 May 12, 2015

Choose a reason for hiding this comment

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

Because the line would be too long for inline-doc (the /**<... */ thing). Semantically they are identical.

Copy link
Copy Markdown
Member

@Lotterleben Lotterleben May 12, 2015

Choose a reason for hiding this comment

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

Makes sense. Thanks for the explanation!

@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 12, 2015

Ok, I think I'm done with this round of reviewing :)

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 12, 2015
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 12, 2015

Addressed comments

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 12, 2015

(was your comment an implicit ACK?)

@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 12, 2015

Nope because the table thing wasn't resolved, but now I can ACK (let's keep the conversation about the table going though)

@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 12, 2015

(ACK after squashing & happy Travis, of course)

@miri64 miri64 force-pushed the ipv6_nc/enh/opt branch from 35368e5 to 396b913 Compare May 12, 2015 18:06
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 12, 2015

Squashed

@Lotterleben Lotterleben removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 13, 2015
@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 13, 2015

Everything is green now, but Travis still says it failed... .
I think it can be merged anyway, can't it? Not keen on waiting another day for the build to pass again :D

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 13, 2015

I'm very confused about this… Just to be sure I restarted the static tests. But in either case: let's merge it, yes.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented May 13, 2015

And there we go, it's green :-)

miri64 added a commit that referenced this pull request May 13, 2015
@miri64
Merge pull request #2908 from authmillenon/ipv6_nc/enh/opt
d1c2f7f 
ipv6_nc: prepare for NDP
@miri64 miri64 merged commit d1c2f7f into RIOT-OS:master May 13, 2015
@miri64 miri64 deleted the ipv6_nc/enh/opt branch May 13, 2015 09:05
@Lotterleben
Copy link
Copy Markdown
Member

Lotterleben commented May 13, 2015

I restarted the first task after removing the NEEDS SQUASHING label, maybe that caused a hickup with Travis... Anyway, yay!

@miri64 miri64 mentioned this pull request May 15, 2015
Closed
miri64 added a commit to miri64/RIOT that referenced this pull request May 15, 2015
@miri64
ng_ipv6: fix for API change in RIOT-OS#2908
0df6729
miri64 added a commit that referenced this pull request May 15, 2015
@miri64
Merge pull request #2987 from authmillenon/ipv6/fix/reachable
66e6e7d 
ng_ipv6: fix for API change in #2908
@miri64 miri64 added the Area: network Area: Networking label Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Lotterleben Lotterleben

Labels

Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Network Stack Task Force

Development

Successfully merging this pull request may close these issues.

3 participants

@miri64 @Lotterleben @OlegHahm