gnrc: create the basic "gnrc_mac" type for providing common MAC functionalities by zhuoshuguo · Pull Request #5941 · RIOT-OS/RIOT

zhuoshuguo · 2016-10-13T16:58:16Z

The goal is to finally create/provide a "gnrc_mac" module which will provide common parameters and functionalities for MAC developers.

This PR first introduces a basic "gnrc_mac_t" type which will be improved by latter PRs to contain more key parameters and functionalities that most MAC protocols will use/adopt, to increase code reuse for MAC developers.

miri64 · 2016-10-13T17:07:25Z

sys/include/net/gnrc/gnrc_mac/gnrc_mac_types.h

+    /* PID of MAC thread */
+    kernel_pid_t pid;
+    /* NETDEV device used by MAC */
+    gnrc_netdev2_t* netdev;


At least these to above are already provided by gnrc_netdev2_t, which should be used to implement MAC anyways by providing a send and recv implementation in my opinion. I also think that was how @kaspar030 intended it. Feel free to amend to that struct, but I think it could be beneficial to #ifdef MODULE_GNRC_MAC them out

Actually, this is inherited and extracted from the original Lw-MAC module, and I didn't changed too much the original design.

But, anyway, that is a good idea to put some new MAC operations into gnrc_netdev2_t, I will see what I can do. :-)

miri64 · 2016-10-13T17:07:59Z

sys/include/net/gnrc/gnrc_mac/gnrc_mac_types.h

+    kernel_pid_t pid;
+    /* NETDEV device used by MAC */
+    gnrc_netdev2_t* netdev;
+    const netdev2_driver_t* netdev2_driver;


This is incorporated in gnrc_mac_t::netdev. Why is this here?

zhuoshuguo · 2016-10-14T09:05:18Z

updated.

miri64 · 2016-10-20T13:19:15Z

See zhuoshuguo#2

zhuoshuguo · 2016-10-28T09:12:02Z

sys/include/net/gnrc/netdev2.h

+#endif
 } gnrc_netdev2_t;

+bool gnrc_netdev2_get_rx_started(gnrc_netdev2_t *dev)


@miri64 should we static inline this function and others below here?

Ooops, that was my code right? Yes, that would be better.

Yes, that was originally come from your code.
I will adapt them. :-)

miri64 · 2016-10-28T09:19:20Z

sys/include/net/gnrc/netdev2.h

 #include "kernel_types.h"
 #include "net/netdev2.h"
 #include "net/gnrc.h"
+#include "net/gnrc/mac.h"


This needs to be changed to net/gnrc/mac/types.h, too.

Yeah! Thanks.

zhuoshuguo · 2016-10-28T09:35:51Z

sys/include/net/gnrc/netdev2.h

+#endif
 } gnrc_netdev2_t;

+#ifdef MODULE_GNRC_MAC


@miri64 I also #ifdef MODULE_GNRC_MAC the gnrc_mac related functions, is that OK? (I think I also have to do it, otherwise the compile will not pass due to mac_info)

Yes, helps to throw compiler errors if you use the functions if you not supposed to.

zhuoshuguo · 2016-10-28T14:43:31Z

sys/include/net/gnrc/netdev2.h

+ * @brief set the rx_started state of the device
+ *
+ * This function is intended to be called only in the _event_cb function of
+ * the netdev2 device.


@miri64 could you help check the correctness of the description here? That is also how I understand the usage of these functions. Thanks

Uhmm.. I don't know about the usage restrictions of this function... as is it can be called from anywhere in the MAC thread (but is restricted to that thread). @daniel-k was the rx_started boolean from your implementation supposed to only be called in the event callback?

@daniel-k was the rx_started boolean from your implementation supposed to only be called in the event callback?

In my implementation rx_started was part of the global LwMAC state struct. I didn't follow the refactoring to that detail, so I don't know who introduced gnrc_netdev2_set_rx_started(), sorry :/

But this function might be dangerous in general. If I remember correctly, _event_cb() can be called in interrupt context as well as in thread context, but it is definetly not thread-safe (docs should reflect that IMHO). Furthermore, manipulating the rx_started information from outside the MAC layer will also cause malfunction.

In my implementation rx_started was part of the global LwMAC state struct. I didn't follow the refactoring to that detail, so I don't know who introduced gnrc_netdev2_set_rx_started(), sorry :/

That was me, but the functionality is the same as the rx_started variable, just a little bit more memory friendly ;-)

But this function might be dangerous in general. If I remember correctly, _event_cb() can be called in interrupt context as well as in thread context, but it is definetly not thread-safe (docs should reflect that IMHO). Furthermore, manipulating the rx_started information from outside the MAC layer will also cause malfunction.

Okay then let's restrict the usage to the event callback. @zhuoshuguo please replace "_event_cb of the netdev2 device" with "netdev2_t::event_callback()" this way it gets properly referenced by the doc parser.

miri64 · 2016-10-28T19:02:25Z

sys/include/net/gnrc/netdev2.h

+ * @brief set the transmission feedback of the device
+ *
+ * This function is intended to be called only in the _event_cb function of
+ * the netdev2 device.


Same goes here.

zhuoshuguo · 2016-10-31T10:28:18Z

@miri64 with your agreement on the function description, updated. Sorry for the late reply. :-)

miri64 · 2016-10-31T14:28:06Z

I'm fine with this PR now as is. Maybe we can get this into the release. Please squash.

zhuoshuguo · 2016-10-31T15:33:04Z

Squashed.

miri64 · 2016-10-31T18:05:42Z

sys/include/net/gnrc/mac/types.h

+extern "C" {
+#endif
+
+typedef enum {


This type (and its members) still requires documentation (please squash that immediately).

miri64 · 2016-11-01T16:14:29Z

Postponed due to feature freeze.

zhuoshuguo · 2016-11-02T09:11:21Z

Updated, added documentation. @miri64 Sorry for the slow action.

zhuoshuguo · 2016-11-07T09:01:55Z

@miri64 Is this PR OK for merge now? :-)

miri64

Yes, ACK and go when Murdock is happy.

zhuoshuguo force-pushed the gnrc_mac_type branch from 298ee7a to a55ec0a Compare October 13, 2016 16:59

miri64 reviewed Oct 13, 2016

View reviewed changes

miri64 added Area: network Area: Networking GNRC labels Oct 13, 2016

miri64 self-assigned this Oct 13, 2016

zhuoshuguo mentioned this pull request Oct 13, 2016

gnrc: add l2_address into gnrc_netdev2_t #5942

Merged

zhuoshuguo force-pushed the gnrc_mac_type branch from a55ec0a to ba9cb41 Compare October 14, 2016 09:04

This was referenced Oct 14, 2016

gnrc_mac: add timeout module. #5949

Merged

gnrc: Add priority packet queue module to gnrc #5950

Merged

zhuoshuguo commented Oct 28, 2016

View reviewed changes

miri64 reviewed Oct 28, 2016

View reviewed changes

zhuoshuguo commented Oct 28, 2016

View reviewed changes

miri64 reviewed Oct 28, 2016

View reviewed changes

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 31, 2016

miri64 added this to the Release 2016.10 milestone Oct 31, 2016

zhuoshuguo force-pushed the gnrc_mac_type branch from fb42331 to ed7deef Compare October 31, 2016 15:32

miri64 requested changes Oct 31, 2016

View reviewed changes

miri64 modified the milestones: Release 2017.01, Release 2016.10 Nov 1, 2016

zhuoshuguo force-pushed the gnrc_mac_type branch from ed7deef to d8082c9 Compare November 2, 2016 09:09

gnrc_mac: add gnrc_mac module into netdev2.

6f39d8c

zhuoshuguo force-pushed the gnrc_mac_type branch from d8082c9 to 6f39d8c Compare November 4, 2016 16:05

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 4, 2016

miri64 approved these changes Nov 7, 2016

View reviewed changes

miri64 merged commit e6ad438 into RIOT-OS:master Nov 7, 2016

miri64 mentioned this pull request Nov 7, 2016

gnrc_netdev2: link-layer retransmissions outside the transceiver driver #4795

Closed

kYc0o mentioned this pull request Jan 11, 2017

Standard IEEE 802.15.4 basic MAC layer for RIOT "gnrc" netstack #4184

Closed

Conversation

zhuoshuguo commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuoshuguo commented Oct 14, 2016

Uh oh!

miri64 commented Oct 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuoshuguo Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-k Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuoshuguo commented Oct 31, 2016

Uh oh!

miri64 commented Oct 31, 2016

Uh oh!

zhuoshuguo commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 1, 2016

Uh oh!

zhuoshuguo commented Nov 2, 2016

Uh oh!

zhuoshuguo commented Nov 7, 2016

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhuoshuguo Oct 28, 2016 •

edited

Loading

daniel-k Oct 28, 2016 •

edited

Loading