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.

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

sys: net: Initial import of a general interface to a network protocol #1448

Merged
merged 1 commit into from
Nov 20, 2014

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jul 18, 2014

The definitions for the new network stack API

@miri64 miri64 added this to the Release NEXT MAJOR milestone Jul 18, 2014
@miri64
Copy link
Member Author

miri64 commented Jul 19, 2014

@thomaseichinger @rousselk could you maybe look if the dummy MAC layer makes sense somehow. I based it mainly on #925, (edit ->) but tried to generalize the assumptions a bit.

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2014

@LudwigOrtmann since HEAD~1 touches nativenet, this PR might interest you, too.

@thomaseichinger
Copy link
Member

The dummy_mac seems good to me. One thing we have to consider, what if we have 2+ radio devices of the same kind e.g. same radio driver implementation. We'd have to determine somehow which device to use. MAC / radio driver internally we can talk about either passing the device to each function call, what we do in low level drivers right now, or save the device in a variable. I'd plead for version 1, since it is more consistent to our existing model.
Anyway, I think we need a radio_device_t or similar in dummy_mac_init like we use for UART, GPIO and SPI.

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2014

The idea for dummy_mac here is very simple: the radio driver is "identified" by it's functions, which the driver developer supplies to dummy_mac via dummy_mac_init() in ops (yes, a few driver functionalities need to be adjusted/expanded). The call of dummy_mac_init() can than be made on board (or maybe better driver) initialization, if no better suitable MAC layer is available.
I took great care that it is possible to call dummy_mac_init() with a whole different set of operations, so that you can have 2+ separate instances of the dummy_mac control thread; theoretically one for each radio/cable.

Since radio devices are very heterogenous and boards can have reattachable network devices (:point_right: arduino) I think there is no easy way via some radio_device_t as we have for example with UART, GPIO, etc. On the contrary: since you have to check for the value of radio_device_t on every command receival, the code might get more complicated than now.

@thomaseichinger
Copy link
Member

I agree with you but I don't think we can find a generic way around radio_device_t. It is used then for the underling layers (mainly the radio driver functions provided in ops) to determine which SPI/I2C -> GPIOs -> hardware radio module to use. These are all specified in the board's periph_conf.h but I think there has to be a initial hint to determine this. Except we assume we won't have boards with 2+ radio devices of the exact same kind (means using the same radio driver functions), which will be the case for most of them. I don't know if we want to restrict ourselves to this.

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2014

Ah yes, now I see your problem. I totally forgot about that :/

@miri64
Copy link
Member Author

miri64 commented Jul 29, 2014

@thomaseichinger see #1492

@miri64
Copy link
Member Author

miri64 commented Aug 14, 2014

Rebased to current status of #1492.

@miri64 miri64 changed the title sys: Initial import of a general interface to a network protocol sys: net: Initial import of a general interface to a network protocol Aug 15, 2014
@miri64
Copy link
Member Author

miri64 commented Aug 19, 2014

Rebased and cleaned up PR a bit.

@miri64
Copy link
Member Author

miri64 commented Aug 19, 2014

Note: I'm aware that Travis is failing. Will fix that in a few hours ;-)

@miri64 miri64 force-pushed the network-layer-interface branch 4 times, most recently from fa256fc to ecd3610 Compare August 26, 2014 08:52
@miri64 miri64 force-pushed the network-layer-interface branch 2 times, most recently from 73d6fa0 to 4a7a728 Compare August 27, 2014 07:26
@miri64
Copy link
Member Author

miri64 commented Aug 27, 2014

Rebased due to b799292 and adapted dummy_mac for ISR event handling

@miri64 miri64 added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Sep 8, 2014
@miri64 miri64 force-pushed the network-layer-interface branch from 2120d7f to f66b26d Compare September 9, 2014 23:07
netapi_ack_t ack;
int ack_result;

DEBUG("send command to %d, cmd = %p, cmd->type = %d\n",
Copy link
Member

Choose a reason for hiding this comment

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

Use PRIkernel_pid instead of %d.

@OlegHahm
Copy link
Member

Be consistent with full-stops in the documentation.

@miri64
Copy link
Member Author

miri64 commented Nov 18, 2014

Addressed @OlegHahm's comments and added new functionality to the REG command.

*
* @param[in] pid The PID of the protocol's control thread.
* @param[in] reg_pid The PID of the upper layer protocol's control thread.
* @param[in] demux_ctx Protocol specific demuxing context
Copy link
Member

Choose a reason for hiding this comment

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

full-stop missing ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops 😊

@miri64
Copy link
Member Author

miri64 commented Nov 19, 2014

Fixed last comment

*/
typedef struct {
/* cppcheck-suppress unusedStructMember because interface is not used yet */
netapi_cmd_type_t type; /**< Type of the command. Must be NETAPI_CMD_RCV. */
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this member superfluous if it has to be NETAPI_CMD_RCV anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, otherwise you wouldn't be able to identify the incoming netapi_cmd_t as a netapi_rcv_pkt_t.

Copy link
Member

Choose a reason for hiding this comment

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

Why not doing it with pointers like

typedef struct {
    netapi_cmd_type_t type;
    void *data;
} netapi_msg_t;

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why have another pointer (and 2-4 byte more for every command depending on the platform) deeper down when you can have all the data on the same level?

Other answer: because I use it like this (very successfully, see this file in #1680 e.g.) for months now and it would cost way more time than I'd like to put into it to change it to your approach ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I see.

@OlegHahm
Copy link
Member

Kicked Travis. Apart from the name of netapi_send_data2(): ACK.

* @return -ENOMSG if wrong acknowledgement was received or was no
* acknowledgement at all.
*/
int netapi_send_data(kernel_pid_t pid, netdev_hlist_t *upper_layer_hdrs,
Copy link
Member Author

Choose a reason for hiding this comment

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

@OlegHahm Use-case: Send data as it is intended to be: Payload and Headers seperated, a lower layer is in charge of the reassembly.

@miri64 miri64 force-pushed the network-layer-interface branch from ef93fa4 to 8ceb24e Compare November 20, 2014 13:31
@miri64
Copy link
Member Author

miri64 commented Nov 20, 2014

Rebased to master

@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 Nov 20, 2014
@OlegHahm
Copy link
Member

ACK. From my point of view, ready to be squashed and merged. @haukepetersen, you want to comment on this?

@miri64 miri64 force-pushed the network-layer-interface branch from 8ceb24e to 598e73b Compare November 20, 2014 13:36
@miri64
Copy link
Member Author

miri64 commented Nov 20, 2014

Squashed

@haukepetersen
Copy link
Contributor

no, just looked through it again, looks fine to me -> ACK

@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 Nov 20, 2014
@miri64
Copy link
Member Author

miri64 commented Nov 20, 2014

Very core-like for something that is not used by now :D

@miri64 miri64 force-pushed the network-layer-interface branch from 598e73b to b7a0794 Compare November 20, 2014 13:51
@miri64
Copy link
Member Author

miri64 commented Nov 20, 2014

Changed commit author.

miri64 added a commit that referenced this pull request Nov 20, 2014
sys: net: Initial import of a general interface to a network protocol
@miri64 miri64 merged commit 4c0aaa6 into RIOT-OS:master Nov 20, 2014
@miri64 miri64 deleted the network-layer-interface branch November 20, 2014 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants