Profiles - default defines NDEBUG macro

[0xc0170]

While I was adding the debug msg to some tests, I noticed default was missing NDEBUG macro, thus I am adding it here. Please review

@theotherjimmy @sg- @c1728p9 @geky @bridadan

[bridadan]

Don't we want this in the default profile? To assist developers if an assert is hit? Wouldn't this be better just to be in the small profile?

[theotherjimmy]

Certainly one of -DDEBUG and -DNDEBUG should be defined by the profile.

[geky]

Won't this disable all assertions outside of the debug build? I'm pretty sure we want assertions on be default to give new users the best experience.

[betzw]

In my opinion NDEBUG should not make part of the default profile but of a release profile.
See also mbed-cli issue #347.

[0xc0170]

Thanks for the inputs and the reference for mbed-cli issue.

Don't we want this in the default profile? To assist developers if an assert is hit? Wouldn't this be better just to be in the small profile?

As the commit msg says, if tests are run (with default profile), there should not be debug messages printed. How to disable them? How to write a logging library if we don't have proper flag to tell an app/library ? Do a user need to specify a specific flag? I know this has been in the code base for a while. It's confusing, now that we added NDEBUG macro there for small profile. Why default is not NDEBUG ? It's clearly not debug profile. the only exceptions I can think of are asserts.

It might be better to have mbed specific macro? A user could do:

#if defined(MBED_DEBUG) && (MBED_DEBUG > 0)
// enable logging
// can do levels here to enable specific msgs
#else
// disable logging
#endif

MBED_DEBUG could have levels (for illustration only , 0 - disabled, 1 - info, 2 - trace. 3 - verbose). Clearly, as this PR is, we can't as it would be breaking change (disabling asserts), thus lets discuss what are the other approaches to this problem.

Adding @kjbracey-arm @jupe for feedback

[bulislaw]

I would say that we should avoid compiling in printf if not explicitly used by the user for default profile.

[geky]

@bulislaw has a good point, not to mention stringifying the assert arguments adds quite a bit of overhead.

This behaviour seems reasonable to me:

• debug - assert prints message and loops on failure
• default - assert loops on failure
• small - no assert

[bulislaw]

We could keep the LED blinking for default profile which would indicate troubles but shouldn't be a big overhead in code size.

[jupe]

Do we agree to use mbed-trace as common tracing library? If so there is proper pre-compiler variables already which can be used by application to select tracing levels or just disable all traces. Library allows to configure trace level on run time as well via c api..Only thing is that trace module doesn't affect to assert messages at all at the moment I think.

I would say that we should avoid compiling in printf if not explicitly used by the user for default profile.

We could further develop trace library so that we could drop printf() requirement by using some separate precompiler variable, but then we lose a lot of trace data as well.. hope we doesn't need to do it..

Anyway, I think that developing and testing builds should contains those traces because it gives a lot of valuable information about behaviors, but release build could be compiled without traces..

CC: @SeppoTakalo @tommikas

[geky]

Adopting the trace library for the debug build makes a lot of sense to me. But for the default build I think we would want to avoid pulling it in for the same reason we want to avoid printf.

Tangential question, the default profile isn't really "release" right? We have debug for debugging, and small for release. The default profile seems specifically for introducing new users.

[bulislaw]

How big mbed-trace is? It's not that I'm suggesting mbedos is big... lets say it's a bit fluffy... but seriously we do need to watch what we integrate.

[bridadan]

@geky I believe default has been traditionally been used for the "release", but we don't really have a formal "release" profile or anything. We just always test with default.

I think the small profile is really only used when you have a finished application or your application has become too big to fit with default. Though that's just how I've used it, not sure about other people.

[sg-]

Lots of good comments here. I think there are a few things needed before making a change like this. Blinking an error pattern first would require changing what exists for 1 led boards so it doesn't look like Blinky. Blinking error patterns implies either:

1. there is an encoded meaning
2. you have options to change the build profile (if you know what these are) to get a printed message of what failed
3. you have a connected debugger and IDE to determine where the failure is.

If we can address some of these then lets reopen, otherwise, closing for now. Should also consider how log/info/debug fits in with the current use of assert.