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

net: added ng_nomac MAC implementation #2426

Merged
merged 2 commits into from
Mar 12, 2015
Merged

net: added ng_nomac MAC implementation #2426

merged 2 commits into from
Mar 12, 2015

Conversation

haukepetersen
Copy link
Contributor

@haukepetersen haukepetersen added NSTF Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: network Area: Networking labels Feb 10, 2015
miri64
miri64 reviewed Feb 10, 2015
View reviewed changes
sys/include/net/ng_nomac.h
* @return -EINVAL if creation of thread fails
* @return -ENODEV if *dev* is invalid
*/
kernel_pid_t nomac_init(char *stack, int stacksize, char priority,
Copy link
Member

Choose a reason for hiding this comment

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

s/nomac/ng_nomac

@haukepetersen
Copy link
Contributor Author

rebased

@miri64
Copy link
Member

miri64 commented Feb 10, 2015

Hint: don't rebase all the stuff on top of each other. Choose one branch as your starting point and merge the others in one at a time. This way you don't create new commits that will never appear in master creating merge conflicts that will hurt you as soon as the original ones get merged ;-)

@miri64
Copy link
Member

miri64 commented Feb 10, 2015

e.g

$ git rebase -i netreg/feat/initial
# the vim view of rebase interactive-mode:
x git merge pktbuf/api/use-pkt_t
pick … your commits

@haukepetersen haukepetersen added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 10, 2015
miri64
miri64 reviewed Feb 10, 2015
View reviewed changes
sys/net/link_layer/ng_nomac/ng_nomac.c
DEBUG("nomac: NG_NETAPI_MSG_TYPE_SND received");
dev->driver->send_data(dev, (ng_pktsnip_t *)msg.content.ptr);
break;
case NG_NETAPI_MSG_TYPE_SETOPT:
Copy link
Member

Choose a reason for hiding this comment

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

Are they called that way in netapi now? if yes, they slipped past my review. For consistency with the other setter/getters of netdev and the wrappers of netapi it self those should be called NG_NETAPI_MSG_TYPE_SET and NG_NETAPI_MSG_TYPE_GET

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is what they are named. But I agree that they should be changed, will do.

Copy link
Member

Choose a reason for hiding this comment

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

See #2427

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #2428

@haukepetersen
Copy link
Contributor Author

hm, someday I will learn this :-)

Anyway, put in the registering to netif and fixed the function naming issue.

@haukepetersen
Copy link
Contributor Author

adapted NETAPI msg type names...

@haukepetersen haukepetersen mentioned this pull request Feb 10, 2015
Merged
@miri64 miri64 mentioned this pull request Feb 10, 2015
Closed
36 tasks
@miri64
Copy link
Member

miri64 commented Feb 11, 2015

Need's rebase + I predict, that you need to port netdev_dummy before the Travis will be happy ;-)

@haukepetersen
Copy link
Contributor Author

I wait with the rebase until the dependencies are merged...

miri64
miri64 reviewed Feb 12, 2015
View reviewed changes
sys/net/link_layer/ng_nomac/ng_nomac.c
/* dispatch NETDEV and NETAPI messages */
switch (msg.type) {
case NG_NETDEV_MSG_TYPE_EVENT:
DEBUG("nomac: NG_NETDEV_MSG_TYPE_EVENT received");
Copy link
Member

Choose a reason for hiding this comment

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

missing \n

@haukepetersen
Copy link
Contributor Author

added missing newlines...

@haukepetersen
Copy link
Contributor Author

rephrased some documentation

@miri64
Copy link
Member

miri64 commented Feb 12, 2015

Just one question: what do you need ng_pktbuf for in this module?

@haukepetersen
Copy link
Contributor Author

very good question, I guess I don't.

miri64
miri64 reviewed Mar 9, 2015
View reviewed changes
sys/net/link_layer/ng_nomac/ng_nomac.c
#if ENABLE_DEBUG
if (sendto == NULL) {
DEBUG("nomac: unable to forward packet of type %i\n",
pkt->next->type);
Copy link
Member

Choose a reason for hiding this comment

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

here to s/next->//

Copy link
Member

Choose a reason for hiding this comment

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

also you have to release the packet here.

@miri64
Copy link
Member

miri64 commented Mar 9, 2015

Apart from my comments this looks okay. ACK when applied.

miri64
miri64 reviewed Mar 9, 2015
View reviewed changes
sys/include/net/ng_nomac.h
* @return -ENODEV if *dev* is invalid
*/
kernel_pid_t ng_nomac_init(char *stack, int stacksize, char priority,
const char *name, ng_netdev_t *dev);
Copy link
Member

Choose a reason for hiding this comment

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

indentation.

@haukepetersen
Copy link
Contributor Author

rebased and addressed comments. @authmillenon, do you re-ack?

miri64
miri64 reviewed Mar 10, 2015
View reviewed changes
sys/net/link_layer/ng_nomac/ng_nomac.c
#if ENABLE_DEBUG
if (sendto == NULL) {
DEBUG("nomac: unable to forward packet of type %i\n", pkt->type);
ng_pktbuf_release(pkt);
Copy link
Member

Choose a reason for hiding this comment

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

Why only in debug context? Also: return is missing.

@miri64
Copy link
Member

miri64 commented Mar 10, 2015

Had still some minor complaints ;-)

@haukepetersen
Copy link
Contributor Author

removed the ENABLE_DEBUG guards in case no once wants to get the data.

@miri64
Copy link
Member

miri64 commented Mar 10, 2015

I gave the NULL pointer controversy some thought: IF it might come to it, it's relatively easy to patch in afterwards. So ACK, please squash

@miri64 miri64 mentioned this pull request Mar 11, 2015
Closed
@miri64
Copy link
Member

miri64 commented Mar 12, 2015

@haukepetersen please squash

@haukepetersen
Copy link
Contributor Author

did. Let's wait for Travis...

@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 12, 2015
@haukepetersen
Copy link
Contributor Author

seems like Travis is a little lazy today...

@haukepetersen
Copy link
Contributor Author

added one missing include, maybe this gets Travis going...

@miri64
Copy link
Member

miri64 commented Mar 12, 2015

Aaalmost done :D

@miri64
Copy link
Member

miri64 commented Mar 12, 2015

\o/

miri64 added a commit that referenced this pull request Mar 12, 2015
@miri64
Merge pull request #2426 from haukepetersen/ng_nomac
ba14149 
net: added ng_nomac MAC implementation
@miri64 miri64 merged commit ba14149 into RIOT-OS:master Mar 12, 2015
@haukepetersen haukepetersen deleted the ng_nomac branch March 13, 2015 08:52
@jonas-rem jonas-rem mentioned this pull request May 20, 2015
Closed
3 tasks
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: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Network Stack Task Force
Development

Successfully merging this pull request may close these issues.
5 participants
@haukepetersen @miri64 @phiros @thomaseichinger @jnohlgard