Revert "Debug build flag + change to sleep behavior in debug mode" #4200

sg- · 2017-04-19T15:20:22Z

Reverts #4097

theotherjimmy · 2017-04-19T15:53:34Z

targets/TARGET_Maxim/TARGET_MAX32630/sleep.c

@@ -34,13 +34,13 @@
 #include "sleep_api.h"
 #include "lp.h"

-void hal_sleep(void)
+void sleep(void)


can we keep this change? I would like to not re-break master.

Yes, we should

theotherjimmy · 2017-04-19T15:53:41Z

targets/TARGET_Maxim/TARGET_MAX32630/sleep.c

 {
    LP_EnterLP2();
 }

 // Low-power stop mode
-void hal_deepsleep(void)
+void deepsleep(void)


This change too.

theotherjimmy · 2017-04-19T15:53:48Z

targets/TARGET_Maxim/TARGET_MAX32630/sleep.c

 {
-    hal_sleep();
+    sleep();


And this one.

theotherjimmy · 2017-04-19T15:55:26Z

If we wanted the same behavior of #4097 without the extra flags, could we do:

#ifndef DEBUG

?

theotherjimmy · 2017-04-19T15:55:43Z

@geky Did I get ^ right?

theotherjimmy · 2017-04-19T16:04:54Z

/morph export-build

geky · 2017-04-19T16:09:08Z

It looks like NDEBUG is only set for the release profile (not develop). Perhaps we should be setting NDEBUG in the develop profile as well?

There is a change needed for the sleep behaviour @bulislaw is looking for, although !NDEBUG being different from MBED_DEBUG does not make sense to me, so +1 for this revert.

bulislaw · 2017-04-19T17:21:30Z

I think we don't want to set 'NDEBUG' for 'develop' as it should carry some debug info. It has to to be fair otherwise people using online ide won't get any error info. For me it would make sense to define debug level flags the same way we have debug level profiles.

theotherjimmy · 2017-04-19T17:25:43Z

I think it makes sense to not sleep only for debug builds. Therefore: #ifndef DEBUG.

mbed-bot · 2017-04-19T20:50:49Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph export-build

geky · 2017-04-19T21:10:06Z

@studavekar, @bridadan
not sure what happened, but mbed bot looks unhappy:

groovy.lang.GroovyRuntimeException: Ambiguous method overloading for method java.util.ArrayList#getAt.
Cannot resolve which method to invoke for [null] due to overlapping prototypes between:
	[interface groovy.lang.Range]
	[interface java.util.Collection]

bridadan

Agreed with @theotherjimmy, can we keep the fix in?

bridadan · 2017-04-19T21:13:22Z

@geky Yup, the config on the CI side wasn't happy. I've pushed another patch, but because there are other jobs in the queue I can't really test my changes before they go live. I believe I've fixed it once in for all, but no way to tell for sure until we run it again.

bulislaw · 2017-04-20T08:25:36Z

Ok, let's take a step back. What is the problem with the original PR? People don't like MBED_DEBUG flag? Or not sleeping only for debug profile? Or something else?

0xc0170 · 2017-04-20T09:26:55Z

Ok, let's take a step back. What is the problem with the original PR? People don't like MBED_DEBUG flag? Or not sleeping only for debug profile? Or something else?

Reverts #4097

If we are reverting something, we should provide a reason why. @sg- Can you provide details?

sg- · 2017-04-20T11:43:08Z

This revert is not to go in as is but the way it went in was wrong and I'm even questioning the original motivation for NDEBUG in the sleep APIs which seems to be the reason we got here in the first place. See comments in #4097

I think the initial addition of NDEBUG was wrong to begin with given I cant tell what problem it solved or any documentation to support its addition.

0xc0170 · 2017-04-24T07:29:52Z

/morph export-build

mbed-bot · 2017-04-24T08:45:10Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 19

All exports and builds passed!

0xc0170 · 2017-04-24T13:11:31Z

Here's my summary (having looked at relevant PR that touched sleep and profiles)

As it was until recently:

sleep() function did sleep only if a board supports sleep functionality, and NDEBUG is defined.
NDEBUG is defined only for the release build.

Introduced as a fix to allow debugging applications using develop/debug profiles.
With the patch for MBED_DEBUG:

Leave NDEBUG out of the picture, introduce MBED_DEBUG. This is defined only for debug profile. Thus sleep is enabled by default (develop profile). And also enabled for the release sleep.
On the other side, breaks uvisor (not yet supported there to go to sleep in the unpriviledged code) .

I think the 2nd is good (introducing another macro, disconnecting from NDEBUG) to enable sleep for develop and release. The debug profile is for debugging, thus disabling sleep. The ideal solution would be to use one macro - NDEBUG but develop can't have that one defined (asserts would be off in the online IDE which we want to keep in).

bulislaw · 2017-04-24T13:43:46Z

That's good analysis. Why are we thinking about reverting this PR or at least parts of it?

geky · 2017-04-24T15:31:27Z

My one qualm is that NDEBUG not matching MBED_DEBUG is a bit confusing. But I can see why you'd want to keep them seperate here.

We should probably add which flags are defined by each profile:
https://docs.mbed.com/docs/mbed-os-handbook/en/latest/dev_tools/build_profiles/

And if we're going down the route of per-profile macros, should we define MBED_RELEASE and MBED_DEVELOP?

bulislaw · 2017-04-25T08:54:07Z

Yeah, if we agree that per (standard) profile macros are something want to have, I'll add the remaining ones and update docs.

0xc0170 · 2017-04-25T09:17:06Z

And if we're going down the route of per-profile macros, should we define MBED_RELEASE and MBED_DEVELOP?

what would be the use for these? Shall an app change its behaviour for these (something like it does for NDEBUG/MBED_DEBUG) ?

bulislaw · 2017-04-25T09:34:15Z

We don't have usage for now, that's why I haven't added them in the first place. But if we add them to be build profile and docs they are ready to be used when needed. If we don't do that we risk people doing it yet another way.

sg- · 2017-04-27T15:42:48Z

https://vilimpoc.org/blog/2017/04/24/power-profiling-on-mbed-nordic-nrf5-series/

Only enabling low power (not lowest but low) based on release profile doesn't seem nice. We shouldn't poke at a solution here and if there is no solution, legacy FTW.

adbridge · 2017-05-17T14:26:45Z

@sg- @bulislaw @geky So is there any progress on this?

sg- · 2017-06-15T15:23:47Z

Will plan some larger work to address the problems here and document the usage.

Revert "Debug build flag + change to sleep behavior in debug mode"

195fdf1

theotherjimmy suggested changes Apr 19, 2017

View reviewed changes

AnotherButler mentioned this pull request Apr 19, 2017

Build profile docs change related to sleep ARMmbed/mbed-os-5-docs#140

Merged

bridadan suggested changes Apr 19, 2017

View reviewed changes

bridadan mentioned this pull request Apr 21, 2017

Release candidate for mbed-os-5.4.4 #4209

Merged

theotherjimmy added the needs: review label Apr 24, 2017

adbridge added needs: work and removed needs: review labels May 4, 2017

sg- closed this Jun 15, 2017

sg- deleted the revert-4097-build_debug_macro branch June 15, 2017 15:23

sg- removed the needs: work label Jun 15, 2017

Revert "Debug build flag + change to sleep behavior in debug mode" #4200

Revert "Debug build flag + change to sleep behavior in debug mode" #4200

Uh oh!

Conversation

sg- commented Apr 19, 2017

Uh oh!

theotherjimmy Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

0xc0170 Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Apr 19, 2017

Uh oh!

theotherjimmy commented Apr 19, 2017

Uh oh!

geky commented Apr 19, 2017

Uh oh!

bulislaw commented Apr 19, 2017

Uh oh!

theotherjimmy commented Apr 19, 2017

Uh oh!

mbed-bot commented Apr 19, 2017

Result: FAILURE

Uh oh!

geky commented Apr 19, 2017

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

bridadan commented Apr 19, 2017

Uh oh!

bulislaw commented Apr 20, 2017

Uh oh!

0xc0170 commented Apr 20, 2017

Uh oh!

sg- commented Apr 20, 2017

Uh oh!

0xc0170 commented Apr 24, 2017

Uh oh!

mbed-bot commented Apr 24, 2017

Result: SUCCESS

Output

Uh oh!

0xc0170 commented Apr 24, 2017

Uh oh!

bulislaw commented Apr 24, 2017

Uh oh!

geky commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bulislaw commented Apr 25, 2017

Uh oh!

0xc0170 commented Apr 25, 2017

Uh oh!

bulislaw commented Apr 25, 2017

Uh oh!

sg- commented Apr 27, 2017

Uh oh!

adbridge commented May 17, 2017

Uh oh!

sg- commented Jun 15, 2017

Uh oh!

Uh oh!

theotherjimmy commented Apr 19, 2017 •

edited

Loading

geky commented Apr 24, 2017 •

edited

Loading