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 common define MBED_ALL_STATS_ENABLED to enable all statistics #6784

Merged
merged 2 commits into from May 11, 2018

Conversation

deepikabhavnani
Copy link

Description

Requirement: All mbed OS statistics should be enabled with single macro MBED_ALL_STATS_ENABLED

Pull request type

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

CC @SenRamakri

Copy link
Member

@bulislaw bulislaw 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 like how the block is repeated multiple times, can we find a one place to do it? Config file would be good I guess.

@deepikabhavnani
Copy link
Author

@bulislaw @0xc0170 - I am confused with the usage of Stack and Heap macro. In case of stack we check if it is defined and true (https://github.com/ARMmbed/mbed-os/blob/master/rtos/TARGET_CORTEX/mbed_rtx_conf.h#L48) but in case of heap we just check if it is defined (https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_alloc_wrappers.cpp#L49).

What is the correct behavior and should we have it consistent to both?

@bulislaw
Copy link
Member

bulislaw commented May 2, 2018

What does our documentation says? I don't think it matters but id rather have it consistent.

As part of Device Health requirement, all mbed OS statistics should be
enabled with single macro `MBED_ALL_STATS_ENABLED`
@deepikabhavnani
Copy link
Author

@studavekar - morph build and test is done with MBED_STACK_STATS_ENABLED and MBED_HEAP_STATS_ENABLED enabled. Should we move to MBED_ALL_STATS_ENABLED?

Please note few new features will come in with MBED_ALL_STATS_ENABLED like #6795.

@SenRamakri
Copy link
Contributor

All STATS flag can start becoming unmanageable as we start adding more stats like CPU stats, errors etc. But I still would like to enable everything in one shot. Can we capture this using configuration system or may be in the mbedcli command line parser itself.

@0xc0170
Copy link
Contributor

0xc0170 commented May 3, 2018

@bulislaw @0xc0170 - I am confused with the usage of Stack and Heap macro. In case of stack we check if it is defined and true (https://github.com/ARMmbed/mbed-os/blob/master/rtos/TARGET_CORTEX/mbed_rtx_conf.h#L48) but in case of heap we just check if it is defined (https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_alloc_wrappers.cpp#L49).

As I understand this: _ENABLED macro - if its defined, then its enabled. If not defined, disabled. Macros like we have in device_has are similar. If DEVICE is defined then its enabled.

If it was MBED_STATS_HEAP then I would assume it can have values - 0 or 1 (true/false)

All STATS flag can start becoming unmanageable as we start adding more stats like CPU stats, errors etc. But I still would like to enable everything in one shot. Can we capture this using configuration system or may be in the mbedcli command line parser itself.

Config named stats that would contain all these?

@deepikabhavnani
Copy link
Author

Can we capture this using configuration system or may be in the mbedcli command line parser itself.

Defines are set as part of command line only

@deepikabhavnani
Copy link
Author

As I understand this: _ENABLED macro - if its defined, then its enabled.

👍 Updated based on similar understanding

@0xc0170
Copy link
Contributor

0xc0170 commented May 7, 2018

All STATS flag can start becoming unmanageable as we start adding more stats like CPU stats, errors etc. But I still would like to enable everything in one shot. Can we capture this using configuration system or may be in the mbedcli command line parser itself.

That would be a nice addition, we are currently have all these set as -D , also this PR.
Should this PR be blocked until, or this is good as it is and separate PR will propose a solution to status flags?

@SenRamakri
Copy link
Contributor

SenRamakri commented May 7, 2018

@0xc0170 @deepikabhavnani - Well, I was thinking lets make the ALL Stats flag as MBED_CONF_ flag which can be used through config system. So, the preferred way ALL_STATS flag is enabled is through our config system. So, that we don't have to sprinkle that in our code multiple places.

@deepikabhavnani
Copy link
Author

. So, that we don't have to sprinkle that in our code multiple places.

Even if we append flags with MBED_CONF, we will still have to add it to code. I am not sure if we have mechanism to detect flag dependency and enable it in config system. @theotherjimmy - Correct me if I am wrong.

@theotherjimmy
Copy link
Contributor

theotherjimmy commented May 7, 2018

@SenRamakri You do not nee an MBDE_CONF_ prefix to be a configuration value. @deepikabhavnani That really begs the question: Is it a good idea? Why would you expect a user to go through the process or turning on a configuration parameter just to make 5 more configuration parameters have an affect? Because one of the is the RTOS present flag, which a user never really touches.

@theotherjimmy
Copy link
Contributor

@deepikabhavnani The config system does not include anything about dependencies. It was designed to avoid the dependency resolution problem.

SenRamakri
SenRamakri previously approved these changes May 8, 2018
@@ -45,7 +45,11 @@
#error "OS Tickrate must be 1000 for system timing"
#endif

#if !defined(OS_STACK_WATERMARK) && (defined(MBED_STACK_STATS_ENABLED) && MBED_STACK_STATS_ENABLED)
#if !defined(OS_STACK_WATERMARK) && defined(MBED_STACK_STATS_ENABLED)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need it duplicated? Can we use #if !defined(OS_STACK_WATERMARK) && (defined(MBED_ALL_STATS_ENABLED) || defined(MBED_STACK_STATS_ENABLED))

All API header files should be part of mbed.h
@deepikabhavnani
Copy link
Author

@bulislaw @0xc0170 @SenRamakri - Please review

@deepikabhavnani
Copy link
Author

/morph build

@mbed-ci
Copy link

mbed-ci commented May 9, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented May 9, 2018

@mbed-ci
Copy link

mbed-ci commented May 10, 2018

@deepikabhavnani
Copy link
Author

Retry export build

/morph export-build

@mbed-ci
Copy link

mbed-ci commented May 10, 2018

@cmonr cmonr merged commit ab7a856 into ARMmbed:master May 11, 2018
@deepikabhavnani deepikabhavnani deleted the mbed_stats_fix branch May 11, 2018 18:44
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