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

shell: add commands to manage neighbor cache manually #2583

Merged
merged 1 commit into from
Apr 2, 2015
Merged

shell: add commands to manage neighbor cache manually #2583

merged 1 commit into from
Apr 2, 2015

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 12, 2015

Depends on #2461 and all its dependencies. (merged)

Waiting for #2602 (merged)

Depends on #2705 (merged)

@miri64 miri64 added Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first NSTF labels Mar 12, 2015
@miri64 miri64 added this to the Network Stack Task Force milestone Mar 12, 2015
@miri64 miri64 force-pushed the shell/feat/ipv6_nc branch 2 times, most recently from 5e1e069 to 8699c56 Compare March 14, 2015 01:31
@miri64
Copy link
Member Author

miri64 commented Mar 14, 2015

Rebased to current master

@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 14, 2015
@miri64
Copy link
Member Author

miri64 commented Mar 19, 2015

Can I haz review?

@miri64 miri64 assigned Lotterleben and unassigned haukepetersen Mar 23, 2015
Lotterleben
Lotterleben reviewed Mar 24, 2015
View reviewed changes
sys/shell/commands/sc_ipv6_nc.c
void _ipv6_nc_manage(int argc, char **argv)
{
if (argc > 2) {
kernel_pid_t iface = (kernel_pid_t)atoi(argv[2]);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this only work for add? For del, argv[2] would be an ipv6 address... or am I missing something?
If not, I'd propose to just move this line below line 115.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: Moved it.

@Lotterleben
Copy link
Member

Done for now :)

@miri64 miri64 force-pushed the shell/feat/ipv6_nc branch from ea1139f to a697517 Compare March 24, 2015 15:32
@miri64
Copy link
Member Author

miri64 commented Mar 24, 2015

Rebased to #2705 and addressed comments

@miri64 miri64 mentioned this pull request Mar 25, 2015
Merged
@miri64
Copy link
Member Author

miri64 commented Mar 25, 2015

Merge stalled before #2602 is merged. (merged)

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 25, 2015
@miri64 miri64 force-pushed the shell/feat/ipv6_nc branch from a697517 to 7ea1fb7 Compare March 29, 2015 22:12
@miri64
Copy link
Member Author

miri64 commented Mar 29, 2015

Rebased to current #2705 and adapted for shell handler return values.

@OlegHahm OlegHahm force-pushed the master branch 3 times, most recently from 9f184dd to 45554bf Compare March 31, 2015 13:01
@Lotterleben
Copy link
Member

Otherwise looks good to my eyes, but I don't have any strong opinons here, style-wise. @OlegHahm @haukepetersen, any complaints?

@miri64
Copy link
Member Author

miri64 commented Mar 31, 2015

Please comment on changes in the PR not in the commits ;-)

I actually would like to add support for #2721, but that I can do later, too.

@miri64
Copy link
Member Author

miri64 commented Mar 31, 2015

Fixed.

@Lotterleben
Copy link
Member

Thanks. Personally, I'd like to not let this PR linger any longer than it has to...

@miri64
Copy link
Member Author

miri64 commented Mar 31, 2015

Squash then?

@Lotterleben
Copy link
Member

Yup :) (I think I need to work on the ambiguousness of my comments :D)

@miri64 miri64 force-pushed the shell/feat/ipv6_nc branch from ff84d58 to 63a46c8 Compare March 31, 2015 19:13
@miri64
Copy link
Member Author

miri64 commented Mar 31, 2015

Done, but #2705 needs to get merged before we can merge this one ;-) (I took the liberty to assign you this PR :-))

@Lotterleben
Copy link
Member

Oooh boy :D I have a long train ride ahead of me tomorrow, will go through it then, okay?

@miri64
Copy link
Member Author

miri64 commented Mar 31, 2015

Okay :-)

@miri64 miri64 force-pushed the shell/feat/ipv6_nc branch from 63a46c8 to bc072db Compare April 1, 2015 23:13
@miri64
Copy link
Member Author

miri64 commented Apr 1, 2015

Rebased to current master, adapt to API change in #2705 and squashed

@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 1, 2015
@Lotterleben
Copy link
Member

Thank you :) ACK if travis is happy.

miri64 added a commit that referenced this pull request Apr 2, 2015
@miri64
Merge pull request #2583 from authmillenon/shell/feat/ipv6_nc
56f5a83 
shell: add commands to manage neighbor cache manually
@miri64 miri64 merged commit 56f5a83 into RIOT-OS:master Apr 2, 2015
@miri64 miri64 deleted the shell/feat/ipv6_nc branch April 2, 2015 10:50
@OlegHahm
Copy link
Member

OlegHahm commented Apr 2, 2015

Sorry to hop in that late: do we really want to provide these commands for every build? They seem to be rather a debugging/development tool, so I would expect them to only get linked with DEVELHELP set or similar.

@miri64
Copy link
Member Author

miri64 commented Apr 2, 2015

Isn't that the main purpose for the shell anyways?

@OlegHahm
Copy link
Member

OlegHahm commented Apr 2, 2015

Is there a English equivalent to German "jein" (it's obviously not "yo", so maybe "nes"?). Anyway, I think you're right that the shell is usually not meant for real, productive deployment, but I would still distinguish between commands that are helpful for normal operation (ifconfig, chan, route...) and helper tools that interfere with normal (network) behavior. The commands from this PR are probably not the only debatable candidates (txtsnd might be another good candidate).

@miri64
Copy link
Member Author

miri64 commented Apr 2, 2015

(Also for some reason related to #2725 I did not manage to compile any application on native with the DEVELHELP macro so this is out of the question ;-))

@miri64
Copy link
Member Author

miri64 commented Apr 2, 2015

(But I see your point, but as you've said: this is not only related to this PR)

@OlegHahm
Copy link
Member

OlegHahm commented Apr 2, 2015

Right. I'll open an issue.

@OlegHahm
Copy link
Member

OlegHahm commented Apr 2, 2015

(Also for some reason related to #2725 I did not manage to compile any application on native with the DEVELHELP macro so this is out of the question ;-))

Sounds weird to me. Since when do we have memory issues on native?

@miri64
Copy link
Member Author

miri64 commented Apr 2, 2015

I don't think it's a memory issue (because without DEVELHELP everything works fine), it's just that DEBUG_ENABLE + DEVELHELP are not possible right now ;-) (but maybe I did not configure the stack sizes correctly… I was one of the people who missunderstood …STACKSIZE_PRINTF ;-))

@miri64
Copy link
Member Author

miri64 commented Apr 2, 2015

But regardless: putting this into DEVELHELP just adds more ambiguity to this macro… I would prefer to add a shell submodule shell_helper or something like that, that is exactly the same as shell_commands in infrastructure, but only contains this little commands that are more helpers than real functionality.

@OlegHahm OlegHahm mentioned this pull request Apr 2, 2015
Closed
@OlegHahm
Copy link
Member

OlegHahm commented Apr 2, 2015

see #2755

@miri64 miri64 mentioned this pull request Apr 2, 2015
Merged
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
Projects
None yet
Milestone
Network Stack Task Force
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @Lotterleben @OlegHahm @haukepetersen