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

Add option to make Nanostack use global event queue #7107

Merged
merged 2 commits into from Jul 12, 2018

Conversation

Projects
None yet
@kjbracey-arm
Contributor

kjbracey-arm commented Jun 4, 2018

Description

As a potential memory saving, offer the option to let Nanostack run its event loop in the global event queue context.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

Could arguably be a "fix" for the increased memory usage of 5.9.0. for mini client.

@kjbracey-arm kjbracey-arm requested review from SeppoTakalo and TeroJaasko Jun 4, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 4, 2018

@TeroJaasko - untested, but do you think this would work for you? Should just need to switch your big stack allocation to the global event queue, along with setting it to "dispatch from from main", and then do so.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:ns-global-events branch from fa38e8e to 31e46fd Jun 5, 2018

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Jun 7, 2018

This needs lot of testing before I would turn this option on, hoever, if off by default, its safer to go in and enter longer testing runs.

@@ -0,0 +1,86 @@
/*
* Copyright (c) 2016 ARM Limited, All Rights Reserved

This comment has been minimized.

@0xc0170

0xc0170 Jun 11, 2018

Member

shouldn't these new files have licenses as the rest of t he files (apache) ?

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Jun 13, 2018

Which release could this make?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 19, 2018

@JanneKiiskila This is an interesting PR. It can definitely be seen as a fix as @kjbracey-arm mentioned, however it could also be seen as a new feature, because it's adding new functionality that wasn't previously there. If this is deemed closer to a feature than a fix, it will be released in 5.10.

I'm more inclined to lean towards a fix, since this particular changeset appears to not require users to change their applications. @ARMmbed/mbed-os-maintainers thoughts?

By the way, the new files still need full license headers.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

Based on the changes and feedback above (about having it off and testing it for longer), I would use minor version for this one. However as noted, if there is an issue that this is addressing, considering patch would be fine.

@JanneKiiskila @kjbracey-arm please advise

I would like to run CI, but waiting for the license fix

@cmonr cmonr added needs: work and removed needs: review labels Jun 20, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 25, 2018

@kjbracey-arm @JanneKiiskila Any update on the PR?

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Jun 29, 2018

Looks good to me. This works on my K64F & ESP8266. Tested after rebasing with current Mbed OS master. Verified also with the MBED_CONF_EVENTS_SHARED_DISPATCH_FROM_APPLICATION option.

One could even modify eventOS_scheduler_run() to have

#if defined(MBED_CONF_NANOSTACK_HAL_EVENT_LOOP_USE_MBED_EVENTS) && defined(MBED_CONF_EVENTS_SHARED_DISPATCH_FROM_APPLICATION)
    EventQueue *queue = mbed::mbed_event_queue();
    queue->dispatch_forever();
#else
<original code>
#endif

so this would be a drop in replacement and one could select the underlying event dispatching model by mbed_app.json. But that might be too evil.

This still creates larger code than Mbed OS 5.8, where one could get event based application with networking stack running in a single thread (ie. from main() function) without most of RTOS code for semaphore and threads. If the high priority event queue could also be optionally dispatched from the application thread, the amount of RTOS primitives and RAM usage could be minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 30, 2018

@JanneKiiskila @kjbracey-arm please advise

I would like to run CI, but waiting for the license fix

@kjbracey-arm kjbracey-arm dismissed stale reviews from TeroJaasko and SeppoTakalo via 898ca13 Jul 2, 2018

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:ns-global-events branch from 31e46fd to 898ca13 Jul 2, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jul 2, 2018

Inserted Apache licence

@@ -0,0 +1,58 @@
/*
* Copyright (c) 2016 ARM Limited, All Rights Reserved

This comment has been minimized.

@0xc0170

0xc0170 Jul 2, 2018

Member

These 2 files are still not having the proper license (+header file)

extern "C" {
#endif
void ns_event_loop_mutex_init(void);

This comment has been minimized.

@0xc0170

0xc0170 Jul 2, 2018

Member

Should this have doxygen doc (I noticed a comment in the code file, can be moved here)

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jul 2, 2018

Contributor

Don't think so - it's internal API. Only exists because of a split between two C files here.

Ultimately gets called a couple of layers down inside public APIs that do have Doxygen (ns_hal_init for Nanostack HAL-using apps like mbed client, else the constructor of a Nanostack-based NetworkInterface).

This comment has been minimized.

@0xc0170

0xc0170 Jul 2, 2018

Member

We could use /internal and still move that comment here. Not certain if this is our practice (I searched platform/drivers for internal keyword, not found it in the docs) but would encourage that.

@ARMmbed/mbed-docs documentation for internal functions? we have doxygen internal flag set to NO so will be excluded. No internal flag mention in https://docs.mbed.com/docs/mbed-os-api-reference/en/latest/APIs/API_Documentation/

* Copyright (c) 2016 ARM Limited, All Rights Reserved
*/
#include <mbed_assert.h>

This comment has been minimized.

@0xc0170

0xc0170 Jul 2, 2018

Member

why assert is having <> for inclusion?

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:ns-global-events branch from 898ca13 to a25fba3 Jul 2, 2018

@@ -2,18 +2,20 @@
* Copyright (c) 2016 ARM Limited, All Rights Reserved

This comment has been minimized.

@0xc0170

0xc0170 Jul 2, 2018

Member

As we are changing this file, also license should fixed (can we make sure have proper license headers in nanostack) ?

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:ns-global-events branch from f73d1b9 to e8ceedd Jul 2, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jul 2, 2018

Popped in some \internal Doxy

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Jul 11, 2018

I want this to a patch release, we can't wait for 5.10 with this one.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 11, 2018

I want this to a patch release, we can't wait for 5.10 with this one.

This adds a new functionality, can't be part of the patch release unfortunately.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 11, 2018

/morph build

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 11, 2018

It's not a new feature really. It's changing the internals of how nanostack works to fix an issue with the size of the system. I don't see a reason why that wouldn't go into a patch release.

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Jul 11, 2018

Thank @bulislaw , I agree 100% with you.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jul 11, 2018

@bulislaw @JanneKiiskila
I think the main question is:
Does this add a new api that can be used by a developer? If the answer is yes then it is a new feature and should go to a feature release, irrespective of whether or not it has been added with backwards compatibility. We have mechanisms in place for allowing the development of code in a more controlled environment other than 'master' between feature releases, it's called using a feature branch. However if the changes are purely internal and there is no new public facing API then it can go to a patch release.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 11, 2018

No it doesn't, it changes internals of Nanostack and exposes a config option so that MCC can tweak it.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jul 11, 2018

So anyone can use this option and tweak the configuration, potentially breaking something ? Sounds like a change in behaviour of the code which is also only supposed to go to feature releases. It is a very grey area.
Normally where there are instances like this our position is as follows:
The submitter or other authorized person makes their case for inclusion in a patch release directly to @ChiefBureaucraticOfficer citing any project impact etc and then he can make the decision.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 11, 2018

Any change can break things, even bug fix can break something so I don't think that's specific to features.

On the side note @kjbracey-arm @JanneKiiskila why do we give people choice? It's just more configuration to support and tests, could we only support the shared event queue?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 11, 2018

I made my comment without realizing my earlier comment:

Based on the changes and feedback above (about having it off and testing it for longer), I would use minor version for this one. However as noted, if there is an issue that this is addressing, considering patch would be fine.

👍

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented Jul 11, 2018

I am generally inclined to approve, but could you clarify one item? Does it change functionality for existing projects? i.e. if I have designed a product already and update to this patch, will I have to change any code for compatibility?

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 11, 2018

Build : SUCCESS

Build number : 2570
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7107/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 11, 2018

@ChiefBureaucraticOfficer no, you won't need to change anything for cases that used to work. This change adds an optimisation that can be switched on to save some memory.

Before we merge that we should understand why do we need to have this choice and not just use the optimised version.

@cmonr cmonr added the do not merge label Jul 11, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 11, 2018

Marking DNM for the time being to make sure this doesn't get merged without @bulislaw question is asnwered.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented Jul 12, 2018

Ok, based on bulislav's comments above, I see no reason to hold up this PR any further. Lets move forward with integration.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jul 12, 2018

From internal discussion:

The changes may have impact on memory pressure in the system and also utilisation of main scheduling queue, therefore they shouldn't be make default in patch release.

That makes sense to me, we should proceed with merging that with patch release tag.

@cmonr cmonr merged commit 6800215 into ARMmbed:master Jul 12, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 791 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10188 cycles (+567 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7107 from kjbracey-arm/ns-global-events
Add option to make Nanostack use global event queue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment