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

Add MBED_ALL_STATS_ENABLED to config system #8761

Merged
merged 1 commit into from
Nov 27, 2018

Conversation

kegilbert
Copy link
Contributor

Description

Add MBED_ALL_STATS_ENABLED to the config system. Added define guards in mbed_stats.h to both disallow accidental conflicting redefines (which will generate a warning without the new guards), and to allow:

 "platform.all-stats-enabled": true,          
 "platform.cpu-stats-enabled": false          

to enable all stats but the CPU ones.

May get a merge conflict with #8734 after the rollup PR gets in, will fix when we get there.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@cmonr
Copy link
Contributor

cmonr commented Nov 15, 2018

@kegilbert New record? Already time for a rebase 😄

@kegilbert
Copy link
Contributor Author

Gah waited all day for that rollup to get in, figured I'd put this PR up now....should've waited 30 minutes.

Updooted.

@cmonr
Copy link
Contributor

cmonr commented Nov 15, 2018

Gah waited all day for that rollup to get in

That makes two of us...

@cmonr
Copy link
Contributor

cmonr commented Nov 15, 2018

Might want to check with this as well, since it'll be next: #8760

@kegilbert
Copy link
Contributor Author

@deepikabhavnani
Copy link

@kegilbert - Good work here, but I will like to wait for #8592.

@lauri-piikivi - Had query if network statistics config option should be in platform or network. Also we enable all stats in 'MBED_ALL_STATS_ENABLED'. With define it is not an issue, but in case of config should it be in feature/netsocket or platform?

#define MBED_THREAD_STATS_ENABLED 1

#ifndef MBED_SYS_STATS_ENABLED
#define MBED_SYS_STATS_ENABLED 1

Choose a reason for hiding this comment

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

Suggested change
#define MBED_SYS_STATS_ENABLED 1
#define MBED_SYS_STATS_ENABLED 1

Copy link

@deepikabhavnani deepikabhavnani Nov 17, 2018

Choose a reason for hiding this comment

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

We don't tab the code in #ifndef #endif. Similar change for other defines as well

@deepikabhavnani
Copy link

@lauri-piikivi - Network statistics will be kept separate from platform statistics. Will update the network stats PR accordingly.

@deepikabhavnani
Copy link

@kegilbert - Looks good to me, just small changes suggested.

@0xc0170 0xc0170 requested a review from a team November 19, 2018 12:14
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Nov 20, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Nov 20, 2018

@mbed-ci
Copy link

mbed-ci commented Nov 20, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 22, 2018

@kegilbert Can you please rebase?

@bulislaw
Copy link
Member

bulislaw commented Nov 26, 2018

All the PRs need to be engineering ready (marked as "needs: CI") by the end of the day (Austin time). Otherwise it won't make 5.11 and will need to come in the next release (5.12 for features, 5.11.1 for fixes and new platforms).

@cmonr
Copy link
Contributor

cmonr commented Nov 26, 2018

@kegilbert Rebase now, or forever hold your peace.

@kegilbert
Copy link
Contributor Author

@cmonr rebased

@cmonr
Copy link
Contributor

cmonr commented Nov 27, 2018

CI restarted.

@mbed-ci
Copy link

mbed-ci commented Nov 27, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 3
Build artifacts
Build logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants