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

gnrc_mac: add timeout module. #5949

Merged
merged 1 commit into from
Jan 22, 2019

Conversation

zhuoshuguo
Copy link
Contributor

@zhuoshuguo zhuoshuguo commented Oct 14, 2016

Add timeout module to "gnrc_mac" to provide timeout setting and handling for MAC protocols which are developed based on "gnrc_mac".

The timeout module here is built on (a wrapper of) evtimer.

Test example (tests/gnrc_mac_timeout in this PR) results on samr21-xrpo:

2019-01-19 16:19:31,956 - INFO # Testing gnrc_mac timeout module (start time = 8 ms)
2019-01-19 16:19:31,961 - INFO # Set timeout_1, should be expired at 1008 ms)
2019-01-19 16:19:31,965 - INFO # Set timeout_2, should be expired at 2546 ms)
2019-01-19 16:19:31,969 - INFO # Set timeout_3, should be expired at 3479 ms)
2019-01-19 16:19:31,975 - INFO # Are the reception times of all 3 msgs close to the supposed values?
2019-01-19 16:19:31,975 - INFO # 
2019-01-19 16:19:31,978 - INFO # If yes, the tests were successful
2019-01-19 16:19:32,966 - INFO # At   1008 ms received msg 1: timeout_1 (set at 8 ms) expired, supposed to be 1008 ms!
2019-01-19 16:19:32,969 - INFO # At   1008 ms: timeout_1 is not running.
2019-01-19 16:19:32,972 - INFO # At   1008 ms: timeout_2 is running.
2019-01-19 16:19:32,976 - INFO # At   1008 ms: timeout_3 is running.
2019-01-19 16:19:34,513 - INFO # At   2546 ms received msg 2: timeout_2 (set at 8 ms) expired, supposed to be 2546 ms!
2019-01-19 16:19:34,516 - INFO # At   2546 ms: timeout_1 is not running.
2019-01-19 16:19:34,520 - INFO # At   2546 ms: timeout_2 is not running.
2019-01-19 16:19:34,523 - INFO # At   2546 ms: timeout_3 is running.
2019-01-19 16:19:35,451 - INFO # At   3479 ms received msg 3: timeout_3 (set at 8 ms) expired, supposed to be 3479 ms!
2019-01-19 16:19:35,455 - INFO # At   3479 ms: timeout_1 is not running.
2019-01-19 16:19:35,458 - INFO # At   3479 ms: timeout_2 is not running.
2019-01-19 16:19:35,462 - INFO # At   3479 ms: timeout_3 is not running.

@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from 2896be9 to a0a4550 Compare October 14, 2016 16:11
@miri64
Copy link
Member

miri64 commented Oct 14, 2016

@zhuoshuguo I have some ideas how to integrate this more concise with the rest of GNRC, but did not find any time to realize them yet. Would you mind to meet up or something like that next week so we can merge our ideas?

@zhuoshuguo
Copy link
Contributor Author

@miri64 Definitely, so, let's meet up next week, let's say Monday, maybe through Skype, what do you think?
By the way, thanks for your suggestions and advices in advance. :-)

@miri64 miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT GNRC labels Oct 15, 2016
@miri64 miri64 self-assigned this Oct 15, 2016
@zhuoshuguo
Copy link
Contributor Author

@miri64 Hi, when do you think we can talk about this? :-) My plan is to merge the related MAC modules and finally the Lw-MAC before the recent RIOT release, i.e., the end of October. Do you think it is possible?

gnrc_netdev2_t* gnrc_netdev2;

/* Store timeouts used for protocol */
gnrc_mac_timeout_t timeouts[GNRC_MAC_TIMEOUT_COUNT];
Copy link
Member

Choose a reason for hiding this comment

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

This too can just be added directly to gnrc_netdev2 with zhuoshuguo#2

@miri64
Copy link
Member

miri64 commented Oct 20, 2016

Lot of what I see here is related to what I plan for NDP (a timer-based queue). @kaspar030 could you maybe finalize some version of your event timer? Maybe considering the functionalities @zhuoshuguo is requiring here, too?

@miri64
Copy link
Member

miri64 commented Oct 20, 2016

@miri64 Hi, when do you think we can talk about this? :-) My plan is to merge the related MAC modules and finally the Lw-MAC before the recent RIOT release, i.e., the end of October. Do you think it is possible?

I'm sorry, there still needs to be put a lot of clean-up work into these PRs (and consolidation with existing types or types that already planned by other people, see above) so I don't think we manage to include these (and thus Lw-MAC as well) into this release :-(. But let's see, where we stand next week.

@zhuoshuguo
Copy link
Contributor Author

@miri64 Thanks, Martine, I see that there are indeed lots of clean-up work waiting to be done, which is also far more than what I expect @emmanuelsearch @OlegHahm (actually, I am little in mess now) . Thus, I understand that it could be difficult to include those PRs (and Lw-MAC) into this release. Anyway, I will continue to digest your suggestions for me and my PRs, and figure out how to adjust Lw-MAC for those newly coming features. Talk to you tomorrow. :-)

@miri64
Copy link
Member

miri64 commented Oct 31, 2016

@zhuoshuguo @kaspar030 published his work at #5999 by now. Could you maybe have a look at it and see how you can adapt? If you have any questions, just ask.

@zhuoshuguo
Copy link
Contributor Author

@miri64 Thanks, Martine, I will check how I can adapt.

@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from a0a4550 to 7881cbe Compare November 6, 2016 15:05
@miri64
Copy link
Member

miri64 commented Nov 7, 2016

Needs rebase.

@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from 337b6ac to 23dec57 Compare November 7, 2016 09:59
@zhuoshuguo
Copy link
Contributor Author

Rebased.

@miri64
Copy link
Member

miri64 commented Nov 7, 2016

@zhuoshuguo please only squash when asked to. Also: you squashed #5999 into your commit now too.

@zhuoshuguo
Copy link
Contributor Author

Opps...my fault. Anyway, I think this PR will be only merged after #5999, so then, I can still adjust, right?

@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from c5f6575 to 16c2275 Compare November 7, 2016 14:21
@zhuoshuguo
Copy link
Contributor Author

Updated, removed evtimer from original commit. Added descriptions.

@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from 16c2275 to da3bdb4 Compare November 7, 2016 14:26
@zhuoshuguo
Copy link
Contributor Author

@miri64 do you think the folder which contains gnrc_mac c files (e.g., sys/net/gnrc/link_layer/gnrc_mac/timeout.c ) should be named as gnrc_mac, mac (this maybe misleading), mac_internal or mac_helper, etc.

@zhuoshuguo zhuoshuguo changed the title gnrc: add timeout module to gnrc_mac gnrc_mac: add timeout module to gnrc_mac Nov 22, 2016
@zhuoshuguo zhuoshuguo mentioned this pull request Feb 13, 2017
@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from 6a2a006 to 36ef66d Compare May 19, 2017 09:55
@zhuoshuguo
Copy link
Contributor Author

Rebased on master.

@miri64
Copy link
Member

miri64 commented Dec 18, 2018

Ping @zhuoshuguo?

@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from 99f3df4 to a9beec4 Compare December 19, 2018 14:53
@zhuoshuguo
Copy link
Contributor Author

@miri64 Updated, and locally tested through on samr21-xpro.

@zhuoshuguo
Copy link
Contributor Author

PS: the error reported earlier was from old version code block (guess the error missed the check of old compiler), has been adapted in the newest commit.

@smlng
Copy link
Member

smlng commented Jan 9, 2019

tested on native and PhyNode looks good to me, though I don't know where it is actually used. But to me it looks mergeable: @miri64? I would say we don't block this for minor reasons any longer.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Jan 9, 2019
@miri64 miri64 added the Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines label Jan 9, 2019
@miri64
Copy link
Member

miri64 commented Jan 9, 2019

Uncrustify does not report anything of interest.

@miri64 miri64 added the Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines label Jan 9, 2019
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

The tests lack some documentation. Please provide at least a README and ideally a test script.

@zhuoshuguo
Copy link
Contributor Author

@miri64 No problem, will add some documentation soon.

@smlng
Copy link
Member

smlng commented Jan 17, 2019

CI is not happy, some simple stuff, i.e. Arduino do not have enough mem. And then some llvm issue that might be a bit uglier?

@zhuoshuguo
Copy link
Contributor Author

zhuoshuguo commented Jan 19, 2019

The tests lack some documentation. Please provide at least a README and ideally a test script.

@miri64 Added documentation for the test example. By the way, isn't this the test script?

@zhuoshuguo
Copy link
Contributor Author

CI is not happy, some simple stuff, i.e. Arduino do not have enough mem. And then some llvm issue that might be a bit uglier?

@smlng any tips about how to solve these simple stuffs? :-)

@miri64
Copy link
Member

miri64 commented Jan 19, 2019

The tests lack some documentation. Please provide at least a README and ideally a test script.

@miri64 Added documentation for the test example. By the way, isn't this the test script?

Thanks for the documentation! You've linked the main.c of your test application. That's not a script that's the program ;-). We usually provide some scripts (e.g. in python) with the test application to allow for automated execution (for some reference see the other tests in the tests/ directory; some of them contain a tests directory in turn with one or more python scripts that interact with the application).

@miri64
Copy link
Member

miri64 commented Jan 19, 2019

@smlng any tips about how to solve these simple stuffs? :-)

Add the boards reported to the BOARD_INSUFFICIENT_MEMORY list of your test application's Makefile:

BOARD_INSUFFICIENT_MEMORY := arduino-duemilanove arduino-mega2560 arduino-uno \
                             chronos nucleo-f030r8 nucleo-f031k6 nucleo-f042k6 \
                             nucleo-l031k6 nucleo-l053r8 stm32f0discovery waspmote-pro

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Please squash.

@zhuoshuguo
Copy link
Contributor Author

@miri64 should I also rebase?

@miri64
Copy link
Member

miri64 commented Jan 21, 2019

Not necessary (+ it makes it easier to check if there is a difference). But if it is easier for you to just rebase, do it.

@zhuoshuguo zhuoshuguo force-pushed the add_timeout_module_to_gnrc_mac branch from 8ddccb8 to 79e5586 Compare January 21, 2019 14:50
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Squash was successful.

@miri64 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 Jan 21, 2019
@zhuoshuguo
Copy link
Contributor Author

Thanks!~ @miri64 All checks pass now.

@miri64 miri64 merged commit d3f8739 into RIOT-OS:master Jan 22, 2019
@aabadie aabadie added this to the Release 2019.01 milestone Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants