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: prepare for NDP #2908

Merged
merged 4 commits into from
May 13, 2015
Merged

ipv6_nc: prepare for NDP #2908

merged 4 commits into from
May 13, 2015

Conversation

miri64
Copy link
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
Member

looks good?

@OlegHahm
Copy link
Member

OlegHahm commented May 5, 2015

That's an ACK?

@OlegHahm
Copy link
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
Member

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

@OlegHahm
Copy link
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
Member

@OlegHahm Sure, I'll do my best!

@miri64
Copy link
Member Author

miri64 commented May 6, 2015

Addressed comments

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

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

@miri64
Copy link
Member Author

miri64 commented May 8, 2015

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

@miri64
Copy link
Member Author

miri64 commented May 8, 2015

Fixed issues that Travis had.

@miri64
Copy link
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
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
Member

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
vtimer_t rtr_timeout; /**< timeout timer for router flag */

/**
* @brief (Re)Transmission timer for neigbor solicitations of this entry and
Copy link
Member

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
Member Author

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
Member

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
Member

(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
vtimer_t nbr_sol_timer;

/**
* @brief Delay timer for neigbor advertisements of this entry.
Copy link
Member

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
Member Author

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
Member

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
Member

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
Member Author

miri64 commented May 12, 2015

Addressed comments

@miri64
Copy link
Member Author

miri64 commented May 12, 2015

(was your comment an implicit ACK?)

@Lotterleben
Copy link
Member

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
Member

(ACK after squashing & happy Travis, of course)

@miri64
Copy link
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
Member

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
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
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
Member

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