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.

Sign up for GitHub

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

Jump to bottom

evtimer: add mbox support #14498

Merged
merged 1 commit into from Aug 21, 2020
Merged

evtimer: add mbox support #14498

merged 1 commit into from Aug 21, 2020

Conversation

brummer-simon
Copy link
Member

Hi,

This PR adds evtimer_msg_mbox_event_t (extends evtimer_msg_event_t with mbox support) and related test code.
It can be tested by executing the contained test code.

With this PR, the xtimers in gnrc_tcp can be replaced with a single evtimer per TCB, as discussed in #14294.

@benpicco benpicco added Area: sys Area: System Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jul 10, 2020
miri64
miri64 reviewed Jul 11, 2020
View reviewed changes
tests/evtimer_msg_mbox/tests/01-run.py Outdated
child.expect('Test successful')
print('Test successful')
except Exception:
print('Test failed')
Copy link
Member

Choose a reason for hiding this comment

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

This ironically makes the test succeed (from the perspective of the CI), because there is no error exit case now. If you want output, just make the echo parameter of the run() function True and remove the prints and exception handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the exception handling

@miri64 miri64 requested a review from kaspar030 July 11, 2020 00:27
@miri64 miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jul 11, 2020
@brummer-simon brummer-simon mentioned this pull request Jul 12, 2020
Merged
miri64
miri64 requested changes Jul 27, 2020
View reviewed changes
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.

I don't see a reason for evtimer_mbox falling back to evtimer_msg other than a slight usage advantage. However, implicit behavior often makes things more complicated and harder to test.

sys/include/evtimer_msg_mbox.h Outdated Show resolved Hide resolved
sys/include/evtimer_msg_mbox.h Outdated Show resolved Hide resolved
sys/include/evtimer_msg_mbox.h Outdated Show resolved Hide resolved
sys/include/evtimer_msg_mbox.h Outdated
evtimer_msg_mbox_event_t *mbevent = (evtimer_msg_mbox_event_t *)event;

/* If a message box is set, store message, otherwise
* fallback to message handler of evtimer_msg */
Copy link
Member

Choose a reason for hiding this comment

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

Why? This makes things more complicated than they need to. If you want to send to a thread's message queue rather than an mbox use evtimer_msg and not use implicit behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see your point there. Originally this PR was intended to be able to send messages to mboxes and normal messages from the same evtimer, reducing the nessecary changes in gnrc_tcp later on.

However rewriting this to only support mboxes is the better approach.

@brummer-simon brummer-simon force-pushed the evtimer-add_mbox_support branch 4 times, most recently from 9c42e6a to e520c6a Compare August 3, 2020 11:15
@brummer-simon
Copy link
Member Author

All previous discussed issues are addressed now. Please review again.

@miri64
Copy link
Member

miri64 commented Aug 3, 2020

All previous discussed issues are addressed now. Please review again.

Can you please use fixup commits? With such big time differences between review and addressing I start to lose overview.

@brummer-simon
Copy link
Member Author

I'll do next time. However with the changes, it was basically a rewrite of the PR contents so fixup would not have made that much sence.

@brummer-simon
Copy link
Member Author

@miri64 - Would you do me a favor and review this PR again? From my point of view it looks good.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware 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 Aug 21, 2020
miri64
miri64 approved these changes Aug 21, 2020
View reviewed changes
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.

Code looks good to me, tests pass on samr21-xpro and nrf52840-dongle. Let's wait for Murdock, if it finds any issues.

miri64
miri64 requested changes Aug 21, 2020
View reviewed changes
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.

Coccinelle found some stuff. You might need to include kernel_defines.h for that.

tests/evtimer_mbox/main.c Outdated Show resolved Hide resolved
tests/evtimer_mbox/main.c Outdated Show resolved Hide resolved
tests/evtimer_mbox/main.c Outdated Show resolved Hide resolved
miri64
miri64 approved these changes Aug 21, 2020
View reviewed changes
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! Please squash.

@brummer-simon
Copy link
Member Author

Done

@miri64
Copy link
Member

miri64 commented Aug 21, 2020

And go. Thanks for this contribution. Looking forward to merging #14500 :-)

@miri64 miri64 merged commit fb63453 into RIOT-OS:master Aug 21, 2020
@brummer-simon brummer-simon deleted the evtimer-add_mbox_support branch August 21, 2020 14:34
@brummer-simon
Copy link
Member Author

#14500 is already rebased ;)

Thanks for your reviewing efforts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees
No one assigned
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@brummer-simon @miri64 @benpicco