netreg: Initial import by miri64 · Pull Request #2404 · RIOT-OS/RIOT

miri64 · 2015-02-07T13:45:35Z

Depends on ~~#2397~~ (merged) ~~#2440~~ (merged). Tests will follow when review is somewhat positive ;-)

haukepetersen · 2015-02-07T15:41:58Z

sys/include/net/ng_netreg.h

we put this in netapi and netdev to uint16_t...

Question is: is this enough?

haukepetersen · 2015-02-07T15:56:06Z

I am not quite convinced of this implementation, I think the lookup is still too expensive! (Compare to #2310)

OlegHahm · 2015-02-07T17:07:24Z

What*s the relation to #2310?

OlegHahm · 2015-02-07T17:11:08Z

sys/net/crosslayer/ng_netreg/ng_netreg.c

miri64 · 2015-02-07T17:43:09Z

@haukepetersen I was thinking about doing a lookup on a two-dimensional array (1 axis: type, other axis demux_type % n), but that seemed to expensive memory-wise. The lookup is in the very common case of 1:1 mappings nearly as fast as your implementation (I have just two comparisons more), since the list will be only one element long. Only for 1:n mappings there will be an iteration, but I don't expect more than two or maybe three protocols talking to the same lower layer in practice. On the network layer we have only IPv4 and IPv6, and we both already argued, that more network protocols are not very realistic. On the transport layer we currently have UDP and TCP and maybe we implement DCCP and SCTP someday. But it is not to expect that you need more than two of them for your application (either you use UDP and TCP together, or you use the benefits of either DCCP or SCTP to not having to do that ;-)). On application layer you might have more than that, but in general you will use the socket API as a "normal" user. And that is one thread only, too.

miri64 · 2015-02-07T17:47:00Z

@haukepetersen I just looked at your implementation again: How does your implementation caters to demultiplexing contexts?

miri64 · 2015-02-07T18:43:21Z

Utilizes utlist instead of clist now

haukepetersen · 2015-02-08T18:38:00Z

I just looked at your implementation again: How does your implementation caters to demultiplexing contexts?

it doesn't. The context stuff was introduced after I did the PR...

miri64 · 2015-02-08T20:42:10Z

The context stuff was introduced after I did the PR...

It was not (it was already in the introductory PR of netapi), but this is not the point: With the demultiplexing context we either have to decide for a slower, but more memory efficient list implementation or a more memory-wasting, but faster, hash table implementation. A simple array-lookup is just not possible.

miri64 · 2015-02-08T20:45:21Z

A tree-based data structure would also be possible, but would need at least one extra pointer + the ROM size would be increasing since it would have a more complex code.

miri64 · 2015-02-09T15:23:26Z

Changed semantic, added tests and fixes following from them

haukepetersen · 2015-02-09T17:48:22Z

Concerning the underlying data-structure: I think we could just provide more then one implementation in the future, one optimized for run-time and one for memory...

Otherwise this PR looks good to me: ACK

Lotterleben · 2015-02-11T21:50:03Z

sys/include/net/ng_netreg.h

If I understood it correctly, the demultiplexing context here works similar to the one from the netapi– why does the demultiplexing context in netapi gets its own typedef'ed type (netapi_reg_demux_ctx_t) and here it's a uint16_t?

This type does not exist in the most current version of netapi, since we moved all of the registration to this module. netapi_reg_demux_ctx_t is quite a mouthful compared to a simple uint16_t, so I left it out ;)

Lotterleben · 2015-02-12T11:12:02Z

(I've had a bit of a hard time understanding ng_netreg_register(), so I added some suggestions for explanatory comments)

miri64 · 2015-02-12T11:20:35Z

Addressed comments and moved from (only available during testing) reset- to init-based setting of the values, just in case someone decides KERNEL_PID_UNDEF can't be 0 anymore ;-)

Lotterleben · 2015-02-12T11:33:21Z

👍 I'll run the tests on a SAMR21 after lunch and if that goes through and Travis is happy, I'd ACK, too.

haukepetersen · 2015-02-12T12:10:44Z

please squash.

miri64 · 2015-02-12T12:13:09Z

I'll wait till #2440 is merged.

haukepetersen · 2015-02-12T12:15:22Z

sorry, overlooked that dependency...

haukepetersen · 2015-02-12T15:12:35Z

now now now :-)

miri64 · 2015-02-12T15:14:25Z

Rebased and squashed. @Lotterleben asked if she can merge ;-)

haukepetersen · 2015-02-12T15:15:58Z

let's just give Travis another chance to complain... @Lotterleben merge at will when Travis is happy!

Lotterleben · 2015-02-12T17:05:07Z

Travis is happy, the tests on the SAMR21are happy... Imma merge this now ✨

netreg: Initial import

haukepetersen · 2015-02-12T17:06:46Z

yeah!

This was referenced Feb 7, 2015

nettype: intial import #2397

Merged

Network stack refactoring: TODO list #2278

Closed

LudwigKnuepfer added this to the Network Stack Task Force milestone Feb 7, 2015

haukepetersen reviewed Feb 7, 2015
View reviewed changes

OlegHahm assigned Lotterleben Feb 7, 2015

OlegHahm reviewed Feb 7, 2015
View reviewed changes

sys/net/crosslayer/ng_netreg/ng_netreg.c Outdated

Copy link
Copy Markdown

Member

OlegHahm Feb 7, 2015

Choose a reason for hiding this comment

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

parens

miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 9, 2015

miri64 force-pushed the netreg/feat/initial branch from 9c216e1 to 9602e72 Compare February 9, 2015 09:45

miri64 force-pushed the netreg/feat/initial branch from e3c092a to 3e3885c Compare February 9, 2015 23:47

Lotterleben reviewed Feb 11, 2015
View reviewed changes

miri64 mentioned this pull request Feb 12, 2015

msp430: add assert header #2440

Merged

miri64 added 2 commits February 12, 2015 16:13

netreg: Initial import

c80c04d

unittests: add ng_netreg tests

cd711ea

miri64 force-pushed the netreg/feat/initial branch from 599e61f to cd711ea Compare February 12, 2015 15:13

miri64 removed 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 12, 2015

Lotterleben added a commit that referenced this pull request Feb 12, 2015

Merge pull request #2404 from authmillenon/netreg/feat/initial

e9afa1e

netreg: Initial import

Lotterleben merged commit e9afa1e into RIOT-OS:master Feb 12, 2015

OlegHahm mentioned this pull request Feb 13, 2015

net: Introduction of global protocol handler registry -> netreg (WIP) #2310

Closed

miri64 deleted the netreg/feat/initial branch February 13, 2015 22:25

Conversation

miri64 commented Feb 7, 2015

Uh oh!

haukepetersen Feb 7, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 7, 2015

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Feb 7, 2015

Uh oh!

OlegHahm commented Feb 7, 2015

Uh oh!

OlegHahm Feb 7, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 commented Feb 7, 2015

Uh oh!

miri64 commented Feb 7, 2015

Uh oh!

miri64 commented Feb 7, 2015

Uh oh!

haukepetersen commented Feb 8, 2015

Uh oh!

miri64 commented Feb 8, 2015

Uh oh!

miri64 commented Feb 8, 2015

Uh oh!

miri64 commented Feb 9, 2015

Uh oh!

haukepetersen commented Feb 9, 2015

Uh oh!

Lotterleben Feb 11, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

Lotterleben commented Feb 12, 2015

Uh oh!

miri64 commented Feb 12, 2015

Uh oh!

Lotterleben commented Feb 12, 2015

Uh oh!

haukepetersen commented Feb 12, 2015

Uh oh!

miri64 commented Feb 12, 2015

Uh oh!

haukepetersen commented Feb 12, 2015

Uh oh!

haukepetersen commented Feb 12, 2015

Uh oh!

miri64 commented Feb 12, 2015

Uh oh!

haukepetersen commented Feb 12, 2015

Uh oh!

Lotterleben commented Feb 12, 2015

Uh oh!

haukepetersen commented Feb 12, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants