Skip to content

net: minimization and simplification of netdev (WIP)#2297

Closed
haukepetersen wants to merge 3 commits intoRIOT-OS:masterfrom
haukepetersen:opt_minimize_netdev
Closed

net: minimization and simplification of netdev (WIP)#2297
haukepetersen wants to merge 3 commits intoRIOT-OS:masterfrom
haukepetersen:opt_minimize_netdev

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

This PR is not meant to produce production code, but as base for discussion (I guess it won't even compile...).

I was looking at the netdev interface for the last two days and I think it has grown to complicated... Here is a proposal for simplification. One main idea is, that netdev_t is basically a parent class for network device descriptors, meaning that structs holding the state of a network device simply are forced to have their first two members being of type kernel_pid_t and netdev_driver_t *. The rest of the members are free to be defined by the driver itself, typically stuff like SPI device, GPIO pins used, internal state data and option fields...

The major changes are:

  • moved and renamed the interface to sys/net/include/netdev.h
  • removed specializations for 802.15.4 (as they are not needed in my opinion)
  • removed everything connnected to hlists -> this code is being moved to pkt.h anyhow... (pkt: Initial import #2158)
  • simplified parameters used in netdev functions
  • flattened the hierarchy for the netdev_t data structure
  • added some options (e.g. ENABLE_PRELOADING...)
  • changed set_state(state) to trigger(action) and added actions

I further touched the AT86RF231 driver slightly, to demonstrate the usage of the netapi interface:

  • I defined the device descriptor for the radio device (again: subclass of netdev_t). All functions of the radio driver should take this struct as their first argument
  • rewrote the receiving part of the radio driver to demonstrate the use of the packet buffer

I think this simplifies matters quite a bit, please let me know what you think!

@haukepetersen haukepetersen added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: drivers Area: Device drivers Area: network Area: Networking labels Jan 13, 2015
@haukepetersen
Copy link
Copy Markdown
Contributor Author

@thomaseichinger Are you currently working on the AT86RFxxx driver? I wasn't sure if it would make sense to go on and completely touch it...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would vote for calling the parameter pkt now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

makes sense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please explain how you imagine to get the source and destination address.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 13, 2015

Didn't look into it in depth now (I am to tired for this right now) but from what I can gather at first glance 👍 (except the comments I already gave)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How are the netdev instance and the device specific type (at86..._t in your example) connected?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Each driver has a device descriptor (a struct containing the devices run-time data). This is the same concept used for many of our device driver (e.g. sensors, network interfaces, etc). The above netdev_t struct simply forces any network device descriptor to have these two parameters on the first positions. This way you can cast any device descriptor to act as netdev_t for being used by MAC layer implementations, while the device driver internally uses it's own type (e.g.at86rf231_t).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

Actually I wouldn't look too much into documentation details for know, I have hardly touched it (as you can see). I rather wanted a first feedback on the changes in general before I spend more time on implementing them...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the name event_handler is much more precise (see https://github.com/RIOT-OS/RIOT/pull/2163/files#diff-d6bffe48827d881fd19847c6af23179bR351 in #2163)

@thomaseichinger
Copy link
Copy Markdown
Member

Please split send_data into load_data(netdev_t *dev, pkt_t *data) and tx_data(netdev_t *dev).

@haukepetersen
Copy link
Copy Markdown
Contributor Author

no, I won't. Just implement preloading by:

  • set_option(ENABLE_PRELOADING);
  • send_data(data...); -> this is loading the data onto the device
  • trigger(ACTION_TX); -> this sends the data

But maybe it makes sense to give send_data a more general name?

@thomaseichinger
Copy link
Copy Markdown
Member

Fine with me, 👍 for renaming send_data.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

hm, how about transfer()? The documentation then has to make clear, that this function has different behavior depending if preloading is enabled or not.

@thomaseichinger
Copy link
Copy Markdown
Member

Thinking about it, send_data() as it does not explicitly state when the data would be sent.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

True. I don't really have an opinion about this name - so whatever the community likes best...

@haukepetersen
Copy link
Copy Markdown
Contributor Author

rebased.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

After talking to @thomaseichinger about the requirements of using OpenWSN with the netdev interface, I generalized it a little more by renaming rcv_data_cb to event_cb. To avoid confusion, I also renamed event() to isr_event().

The concept is that the driver is now able to signal all kind of events (standardized in net_event_t) to the MAC layer. This events can be for example DATA_RECEIVED, TRANSMISSION_STARTED and so on. The MAC layer can decide what kind of events it wants to get by using the set_option() interface.

Now we have a two-directional event communication between a device driver and MAC layer, isr_event() being called by the MAC layer on receiving of a msg of type NETDEV_MSG_EVENT_TYPE and event_cb() being called by the device driver on configurable events.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the idea to hand over the netapi_pkt_t (see #2286) struct on NETDEV_EVENT_RX_COMPLETE e. g.?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(the structure that comes into arg for what event should be well documented is what I want to say ;))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not quite, the idea is to hand over a pkt_t * on NETDEV_EVENT_RX_COMPLETE.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

closed in favor of #2398

@haukepetersen haukepetersen deleted the opt_minimize_netdev branch February 7, 2015 13:01
@miri64 miri64 removed this from the Network Stack Task Force milestone Feb 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants