Re: debugassert vs assert in apps
Would a variation on this proposal be to add app-specific NXAPP_DEBUGASSERT etc.? This would then be "global" but unique to apps, not overlap with O/S DEBUGASSERTs, and make for a simpler global search/replace of current DEBUGASSERT in apps, and just need a one-off new set of entries in the apps' Kconfig? Over time, the NX apps debug "system" could be expanded/enhanced to allow app-by-app "APPNAME_ENABLE_DEBUG" that is similar to the OS DEBUG system, but would be quicker/easier to implement in one hit for now? (Accidentally sent this before as a direct reply to Nathan: sorry!) On 03/01/2024 16:43, Nathan Hartman wrote: On Wed, Jan 3, 2024 at 11:22 AM Gregory Nutt wrote: On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: That would seem a little odd since there was a PR a few years ago to change all instances of assert/ASSERT to DEBUGASSERT to save code size. How is that so? As I see here: https://github.com/apache/nuttx/blob/master/include/assert.h#L122 assert defined exactly as DEBUGASSERT. There shouldn't be any code size difference at all. When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of DEBUGASSERT compile to nothing (actually the current version compiles to an expression that is optimized out): #undef DEBUGASSERT /* Like ASSERT, but only if CONFIG_DEBUG_ASSERTIONS is defined */ #ifdef CONFIG_DEBUG_ASSERTIONS # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, __DEBUG_ASSERT_LINE__) #else # define DEBUGASSERT(f) ((void)(1 || (f))) #endif This value, ((void)(1 || (f))), is completely removed by the optimizer because of short-circuiting and dead code removal. So the code is much smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() is replaced with assert() than that code bloat would be unconditional, although somewhat less than when assertions are enabled. This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. Xiao Xiang made that change to reduce the size as needed by their products. He is the person you should be talking to. Maybe we need NX_DEBUGASSERT, NX_DEBUGPANIC, NX_DEBUGVERIFY. The NX prefix would make it more clear that this is NuttX-specific. These would be used in the OS only, not in applications, and CONFIG_DEBUG_ASSERTIONS would continue to control if these are real or optimized out. Applications that need their own specific, Kconfig-controlled debug assertion, should define one themselves, and their own Kconfig to optimize it out. Rationale: If you are debugging an application, enable assertions only in that application, not everywhere throughout the system. Cheers Nathan
Re: debugassert vs assert in apps
If someone can post with a consensus/decision once there is one, it would be most useful! On 03/01/2024 17:35, Fotis Panagiotopoulos wrote: Just to be clear, I am always referring to the standard C assert() function/macro. Not the unconditional NuttX ASSERT() macro. Notice the capitalization! (Which is another confusing point worth improving... +1 for NX_ASSERT() ) On Wed, Jan 3, 2024 at 7:32 PM Fotis Panagiotopoulos wrote: I am sorry, but I still don't see the difference. See this line here: https://github.com/apache/nuttx/blob/master/include/assert.h#L119 When NDEBUG is defined, assert also compiles to nothing. As it ought to do. (NDEBUG definition is also controlled by Kconfig, with CONFIG_NDEBUG) So, is there an actual difference that I currently have overseen? On Wed, Jan 3, 2024 at 6:54 PM Tomek CEDRO wrote: +1 :-) -- CeDeROM, SQ7MHZ,http://www.tomek.cedro.info On Wed, Jan 3, 2024, 17:44 Gregory Nutt wrote: +1 On 1/3/2024 10:43 AM, Nathan Hartman wrote: On Wed, Jan 3, 2024 at 11:22 AM Gregory Nutt wrote: On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: That would seem a little odd since there was a PR a few years ago to change all instances of assert/ASSERT to DEBUGASSERT to save code size. How is that so? As I see here: https://github.com/apache/nuttx/blob/master/include/assert.h#L122 assert defined exactly as DEBUGASSERT. There shouldn't be any code size difference at all. When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of DEBUGASSERT compile to nothing (actually the current version compiles to an expression that is optimized out): #undef DEBUGASSERT /* Like ASSERT, but only if CONFIG_DEBUG_ASSERTIONS is defined */ #ifdef CONFIG_DEBUG_ASSERTIONS # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, __DEBUG_ASSERT_LINE__) #else # define DEBUGASSERT(f) ((void)(1 || (f))) #endif This value, ((void)(1 || (f))), is completely removed by the optimizer because of short-circuiting and dead code removal. So the code is much smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() is replaced with assert() than that code bloat would be unconditional, although somewhat less than when assertions are enabled. This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. Xiao Xiang made that change to reduce the size as needed by their products. He is the person you should be talking to. Maybe we need NX_DEBUGASSERT, NX_DEBUGPANIC, NX_DEBUGVERIFY. The NX prefix would make it more clear that this is NuttX-specific. These would be used in the OS only, not in applications, and CONFIG_DEBUG_ASSERTIONS would continue to control if these are real or optimized out. Applications that need their own specific, Kconfig-controlled debug assertion, should define one themselves, and their own Kconfig to optimize it out. Rationale: If you are debugging an application, enable assertions only in that application, not everywhere throughout the system. Cheers Nathan
Re: debugassert vs assert in apps
Just to be clear, I am always referring to the standard C assert() function/macro. Not the unconditional NuttX ASSERT() macro. Notice the capitalization! (Which is another confusing point worth improving... +1 for NX_ASSERT() ) On Wed, Jan 3, 2024 at 7:32 PM Fotis Panagiotopoulos wrote: > I am sorry, but I still don't see the difference. > > See this line here: > https://github.com/apache/nuttx/blob/master/include/assert.h#L119 > > When NDEBUG is defined, assert also compiles to nothing. > As it ought to do. > > (NDEBUG definition is also controlled by Kconfig, with CONFIG_NDEBUG) > > So, is there an actual difference that I currently have overseen? > > On Wed, Jan 3, 2024 at 6:54 PM Tomek CEDRO wrote: > >> +1 :-) >> >> -- >> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info >> >> On Wed, Jan 3, 2024, 17:44 Gregory Nutt wrote: >> >> > +1 >> > >> > On 1/3/2024 10:43 AM, Nathan Hartman wrote: >> > > On Wed, Jan 3, 2024 at 11:22 AM Gregory Nutt >> > wrote: >> > >> On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: >> > That would seem a little odd since there was a PR a few years ago >> to >> > >>> change all instances of assert/ASSERT to DEBUGASSERT to save code >> size. >> > >>> >> > >>> How is that so? >> > >>> >> > >>> As I see here: >> > >>> https://github.com/apache/nuttx/blob/master/include/assert.h#L122 >> > >>> assert defined exactly as DEBUGASSERT. >> > >>> >> > >>> There shouldn't be any code size difference at all. >> > >> When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of >> > >> DEBUGASSERT compile to nothing (actually the current version >> compiles to >> > >> an expression that is optimized out): >> > >> >> > >> #undef DEBUGASSERT /* Like ASSERT, but only if >> > >> CONFIG_DEBUG_ASSERTIONS is defined */ >> > >> >> > >> #ifdef CONFIG_DEBUG_ASSERTIONS >> > >> # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, >> > >> __DEBUG_ASSERT_LINE__) >> > >> #else >> > >> # define DEBUGASSERT(f) ((void)(1 || (f))) >> > >> #endif >> > >> >> > >> This value, ((void)(1 || (f))), is completely removed by the >> optimizer >> > >> because of short-circuiting and dead code removal. So the code is >> much >> > >> smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() >> is >> > >> replaced with assert() than that code bloat would be unconditional, >> > >> although somewhat less than when assertions are enabled. >> > >> >> > >> This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. >> > >> >> > >> Xiao Xiang made that change to reduce the size as needed by their >> > >> products. He is the person you should be talking to. >> > > >> > > Maybe we need NX_DEBUGASSERT, NX_DEBUGPANIC, NX_DEBUGVERIFY. The NX >> > > prefix would make it more clear that this is NuttX-specific. These >> > > would be used in the OS only, not in applications, and >> > > CONFIG_DEBUG_ASSERTIONS would continue to control if these are real or >> > > optimized out. >> > > >> > > Applications that need their own specific, Kconfig-controlled debug >> > > assertion, should define one themselves, and their own Kconfig to >> > > optimize it out. Rationale: If you are debugging an application, >> > > enable assertions only in that application, not everywhere throughout >> > > the system. >> > > >> > > Cheers >> > > Nathan >> > >> > >> > >> >
Re: debugassert vs assert in apps
I am sorry, but I still don't see the difference. See this line here: https://github.com/apache/nuttx/blob/master/include/assert.h#L119 When NDEBUG is defined, assert also compiles to nothing. As it ought to do. (NDEBUG definition is also controlled by Kconfig, with CONFIG_NDEBUG) So, is there an actual difference that I currently have overseen? On Wed, Jan 3, 2024 at 6:54 PM Tomek CEDRO wrote: > +1 :-) > > -- > CeDeROM, SQ7MHZ, http://www.tomek.cedro.info > > On Wed, Jan 3, 2024, 17:44 Gregory Nutt wrote: > > > +1 > > > > On 1/3/2024 10:43 AM, Nathan Hartman wrote: > > > On Wed, Jan 3, 2024 at 11:22 AM Gregory Nutt > > wrote: > > >> On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: > > That would seem a little odd since there was a PR a few years ago to > > >>> change all instances of assert/ASSERT to DEBUGASSERT to save code > size. > > >>> > > >>> How is that so? > > >>> > > >>> As I see here: > > >>> https://github.com/apache/nuttx/blob/master/include/assert.h#L122 > > >>> assert defined exactly as DEBUGASSERT. > > >>> > > >>> There shouldn't be any code size difference at all. > > >> When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of > > >> DEBUGASSERT compile to nothing (actually the current version compiles > to > > >> an expression that is optimized out): > > >> > > >> #undef DEBUGASSERT /* Like ASSERT, but only if > > >> CONFIG_DEBUG_ASSERTIONS is defined */ > > >> > > >> #ifdef CONFIG_DEBUG_ASSERTIONS > > >> # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, > > >> __DEBUG_ASSERT_LINE__) > > >> #else > > >> # define DEBUGASSERT(f) ((void)(1 || (f))) > > >> #endif > > >> > > >> This value, ((void)(1 || (f))), is completely removed by the optimizer > > >> because of short-circuiting and dead code removal. So the code is > much > > >> smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() > is > > >> replaced with assert() than that code bloat would be unconditional, > > >> although somewhat less than when assertions are enabled. > > >> > > >> This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. > > >> > > >> Xiao Xiang made that change to reduce the size as needed by their > > >> products. He is the person you should be talking to. > > > > > > Maybe we need NX_DEBUGASSERT, NX_DEBUGPANIC, NX_DEBUGVERIFY. The NX > > > prefix would make it more clear that this is NuttX-specific. These > > > would be used in the OS only, not in applications, and > > > CONFIG_DEBUG_ASSERTIONS would continue to control if these are real or > > > optimized out. > > > > > > Applications that need their own specific, Kconfig-controlled debug > > > assertion, should define one themselves, and their own Kconfig to > > > optimize it out. Rationale: If you are debugging an application, > > > enable assertions only in that application, not everywhere throughout > > > the system. > > > > > > Cheers > > > Nathan > > > > > > >
Re: debugassert vs assert in apps
+1 :-) -- CeDeROM, SQ7MHZ, http://www.tomek.cedro.info On Wed, Jan 3, 2024, 17:44 Gregory Nutt wrote: > +1 > > On 1/3/2024 10:43 AM, Nathan Hartman wrote: > > On Wed, Jan 3, 2024 at 11:22 AM Gregory Nutt > wrote: > >> On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: > That would seem a little odd since there was a PR a few years ago to > >>> change all instances of assert/ASSERT to DEBUGASSERT to save code size. > >>> > >>> How is that so? > >>> > >>> As I see here: > >>> https://github.com/apache/nuttx/blob/master/include/assert.h#L122 > >>> assert defined exactly as DEBUGASSERT. > >>> > >>> There shouldn't be any code size difference at all. > >> When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of > >> DEBUGASSERT compile to nothing (actually the current version compiles to > >> an expression that is optimized out): > >> > >> #undef DEBUGASSERT /* Like ASSERT, but only if > >> CONFIG_DEBUG_ASSERTIONS is defined */ > >> > >> #ifdef CONFIG_DEBUG_ASSERTIONS > >> # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, > >> __DEBUG_ASSERT_LINE__) > >> #else > >> # define DEBUGASSERT(f) ((void)(1 || (f))) > >> #endif > >> > >> This value, ((void)(1 || (f))), is completely removed by the optimizer > >> because of short-circuiting and dead code removal. So the code is much > >> smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() is > >> replaced with assert() than that code bloat would be unconditional, > >> although somewhat less than when assertions are enabled. > >> > >> This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. > >> > >> Xiao Xiang made that change to reduce the size as needed by their > >> products. He is the person you should be talking to. > > > > Maybe we need NX_DEBUGASSERT, NX_DEBUGPANIC, NX_DEBUGVERIFY. The NX > > prefix would make it more clear that this is NuttX-specific. These > > would be used in the OS only, not in applications, and > > CONFIG_DEBUG_ASSERTIONS would continue to control if these are real or > > optimized out. > > > > Applications that need their own specific, Kconfig-controlled debug > > assertion, should define one themselves, and their own Kconfig to > > optimize it out. Rationale: If you are debugging an application, > > enable assertions only in that application, not everywhere throughout > > the system. > > > > Cheers > > Nathan > > >
Re: debugassert vs assert in apps
+1 On 1/3/2024 10:43 AM, Nathan Hartman wrote: On Wed, Jan 3, 2024 at 11:22 AM Gregory Nutt wrote: On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: That would seem a little odd since there was a PR a few years ago to change all instances of assert/ASSERT to DEBUGASSERT to save code size. How is that so? As I see here: https://github.com/apache/nuttx/blob/master/include/assert.h#L122 assert defined exactly as DEBUGASSERT. There shouldn't be any code size difference at all. When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of DEBUGASSERT compile to nothing (actually the current version compiles to an expression that is optimized out): #undef DEBUGASSERT /* Like ASSERT, but only if CONFIG_DEBUG_ASSERTIONS is defined */ #ifdef CONFIG_DEBUG_ASSERTIONS # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, __DEBUG_ASSERT_LINE__) #else # define DEBUGASSERT(f) ((void)(1 || (f))) #endif This value, ((void)(1 || (f))), is completely removed by the optimizer because of short-circuiting and dead code removal. So the code is much smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() is replaced with assert() than that code bloat would be unconditional, although somewhat less than when assertions are enabled. This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. Xiao Xiang made that change to reduce the size as needed by their products. He is the person you should be talking to. Maybe we need NX_DEBUGASSERT, NX_DEBUGPANIC, NX_DEBUGVERIFY. The NX prefix would make it more clear that this is NuttX-specific. These would be used in the OS only, not in applications, and CONFIG_DEBUG_ASSERTIONS would continue to control if these are real or optimized out. Applications that need their own specific, Kconfig-controlled debug assertion, should define one themselves, and their own Kconfig to optimize it out. Rationale: If you are debugging an application, enable assertions only in that application, not everywhere throughout the system. Cheers Nathan
Re: debugassert vs assert in apps
On Wed, Jan 3, 2024 at 11:22 AM Gregory Nutt wrote: > > On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: > >> That would seem a little odd since there was a PR a few years ago to > > change all instances of assert/ASSERT to DEBUGASSERT to save code size. > > > > How is that so? > > > > As I see here: > > https://github.com/apache/nuttx/blob/master/include/assert.h#L122 > > assert defined exactly as DEBUGASSERT. > > > > There shouldn't be any code size difference at all. > > When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of > DEBUGASSERT compile to nothing (actually the current version compiles to > an expression that is optimized out): > > #undef DEBUGASSERT /* Like ASSERT, but only if > CONFIG_DEBUG_ASSERTIONS is defined */ > > #ifdef CONFIG_DEBUG_ASSERTIONS > # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, > __DEBUG_ASSERT_LINE__) > #else > # define DEBUGASSERT(f) ((void)(1 || (f))) > #endif > > This value, ((void)(1 || (f))), is completely removed by the optimizer > because of short-circuiting and dead code removal. So the code is much > smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() is > replaced with assert() than that code bloat would be unconditional, > although somewhat less than when assertions are enabled. > > This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. > > Xiao Xiang made that change to reduce the size as needed by their > products. He is the person you should be talking to. Maybe we need NX_DEBUGASSERT, NX_DEBUGPANIC, NX_DEBUGVERIFY. The NX prefix would make it more clear that this is NuttX-specific. These would be used in the OS only, not in applications, and CONFIG_DEBUG_ASSERTIONS would continue to control if these are real or optimized out. Applications that need their own specific, Kconfig-controlled debug assertion, should define one themselves, and their own Kconfig to optimize it out. Rationale: If you are debugging an application, enable assertions only in that application, not everywhere throughout the system. Cheers Nathan
Re: debugassert vs assert in apps
On 1/3/2024 10:11 AM, Fotis Panagiotopoulos wrote: That would seem a little odd since there was a PR a few years ago to change all instances of assert/ASSERT to DEBUGASSERT to save code size. How is that so? As I see here: https://github.com/apache/nuttx/blob/master/include/assert.h#L122 assert defined exactly as DEBUGASSERT. There shouldn't be any code size difference at all. When CONFIG_DEBUG_ASSERTIONS is not defined, then all occurrences of DEBUGASSERT compile to nothing (actually the current version compiles to an expression that is optimized out): #undef DEBUGASSERT /* Like ASSERT, but only if CONFIG_DEBUG_ASSERTIONS is defined */ #ifdef CONFIG_DEBUG_ASSERTIONS # define DEBUGASSERT(f) _ASSERT(f, __DEBUG_ASSERT_FILE__, __DEBUG_ASSERT_LINE__) #else # define DEBUGASSERT(f) ((void)(1 || (f))) #endif This value, ((void)(1 || (f))), is completely removed by the optimizer because of short-circuiting and dead code removal. So the code is much smaller if CONFIG_DEBUG_ASSERTIONS is not enabled. If DEBUGASSERT() is replaced with assert() than that code bloat would be unconditional, although somewhat less than when assertions are enabled. This same kind of logic also applies to DEBUGPANIC and DEBUGVERIFY. Xiao Xiang made that change to reduce the size as needed by their products. He is the person you should be talking to.
Re: debugassert vs assert in apps
> That would seem a little odd since there was a PR a few years ago to change all instances of assert/ASSERT to DEBUGASSERT to save code size. How is that so? As I see here: https://github.com/apache/nuttx/blob/master/include/assert.h#L122 assert defined exactly as DEBUGASSERT. There shouldn't be any code size difference at all. On Wed, Jan 3, 2024 at 3:55 PM Gregory Nutt wrote: > On 1/3/2024 5:12 AM, Fotis Panagiotopoulos wrote: > > Hello everyone, > > > > I am glad that we all agree on this matter. > > > > We can handle this in the following steps: > > > > 1. Ensure that any new PRs and apps follow this convention. > > This is up to the reviewers, to enforce. > > > > 2. Get rid of all DEBUGASSERTs in apps. > > Unfortunately, a quick grep yielded 3410 results... > > > > How can this be managed? > >A: Find-and-replace DEBUGASSERT with assert. Is this safe to do so? > >Can I assume that assert is 100% safe to use where now > DEBUGASSERT is > > used? > That would seem a little odd since there was a PR a few years ago to > change all instances of assert/ASSERT to DEBUGASSERT to save code size. > >B: Modify checkpatch.sh to catch this on modified files? > >Just like we gradually fixed licenses, coding style etc? > > Any other ideas? > > > > 3. When the above are ready, move these definitions in nuttx/assert.h > > An alternative: > > 3. Move definitions to nuttx/assert.h > > 4. create an apps/include/assert.h that also defines DEBUGASSERT. When > the above are ready, just remove apps/include/assert. > > In fact just keeping DEBUGASSERT defined in an apps-specific > apps/include/assert.h is fine with me. The problem now is that > DEBUGASSERT is treated as if it were a standard OS interface. But > nothing in apps/include/ is standard and a DEBUGASSERT definition there > is completely healthy. > > >
Re: debugassert vs assert in apps
On 1/3/2024 5:12 AM, Fotis Panagiotopoulos wrote: Hello everyone, I am glad that we all agree on this matter. We can handle this in the following steps: 1. Ensure that any new PRs and apps follow this convention. This is up to the reviewers, to enforce. 2. Get rid of all DEBUGASSERTs in apps. Unfortunately, a quick grep yielded 3410 results... How can this be managed? A: Find-and-replace DEBUGASSERT with assert. Is this safe to do so? Can I assume that assert is 100% safe to use where now DEBUGASSERT is used? That would seem a little odd since there was a PR a few years ago to change all instances of assert/ASSERT to DEBUGASSERT to save code size. B: Modify checkpatch.sh to catch this on modified files? Just like we gradually fixed licenses, coding style etc? Any other ideas? 3. When the above are ready, move these definitions in nuttx/assert.h An alternative: 3. Move definitions to nuttx/assert.h 4. create an apps/include/assert.h that also defines DEBUGASSERT. When the above are ready, just remove apps/include/assert. In fact just keeping DEBUGASSERT defined in an apps-specific apps/include/assert.h is fine with me. The problem now is that DEBUGASSERT is treated as if it were a standard OS interface. But nothing in apps/include/ is standard and a DEBUGASSERT definition there is completely healthy.
Re: debugassert vs assert in apps
Hello everyone, I am glad that we all agree on this matter. We can handle this in the following steps: 1. Ensure that any new PRs and apps follow this convention. This is up to the reviewers, to enforce. 2. Get rid of all DEBUGASSERTs in apps. Unfortunately, a quick grep yielded 3410 results... How can this be managed? A: Find-and-replace DEBUGASSERT with assert. Is this safe to do so? Can I assume that assert is 100% safe to use where now DEBUGASSERT is used? B: Modify checkpatch.sh to catch this on modified files? Just like we gradually fixed licenses, coding style etc? Any other ideas? 3. When the above are ready, move these definitions in nuttx/assert.h > This is an excellent summary! I suggest this should be documented > somewhere in Documentation, with a more summarized version in the > comment block of these macros. This way, the discussion and its > conclusions will not be forgotten in the future!! Where do you propose to add this? In which context? (You mean something like a "guide" when to use one vs the other?) On Tue, Jan 2, 2024 at 10:08 PM Xiang Xiao wrote: > Here is the issue to track debug.h created before: > https://github.com/apache/nuttx/issues/8986 > > On Wed, Jan 3, 2024 at 2:30 AM Gregory Nutt wrote: > > > > On Tue, Jan 2, 2024 at 11:16 AM Fotis Panagiotopoulos > > > wrote: > > >> DEBUGASSERT shall only be used for the kernel. > > >> assert shall only be used for apps. > > >> > > >> Rationale: > > >> > > >> DEBUGASSERT is a custom macro that only exists in the NuttX world. > > >> As such it is best being used in NuttX-specific code. > > >> > > >> assert on the other hand is a standard function, it exists in all > > systems. > > >> It seems better suited for generic and portable code (i.e. apps). > > > > We have no way to enforce this. What we do do is at least make it clear > > that the application is using a kernel-only macro or function > > inappropriately by putting the macro definition or function prototype in > > NuttX private file. For example, > > > > * Remove the DEBUGASSERT (and ASSERT?) macros from include/assert.h > > * Add them to include/nuttx/assert.h > > * All applications that use DEBUGASSERT (or ASSERT) should be changed > > to use assert() > > * Kernel functions that use DEBUGASSERT (or ASSERT) would have to > > explicitly include > > > > Not foolproof, but that is how we have dealt this this in the past. It > > if is in include/assert.h, is is essentially being advertised as OK for > > use by applications. This is logically equivalent to the Linux/LibC > > conventions of putting Linux-specific files in /usr/include/linux vs. > > normal, standard application files in /usr/include. > > > > There may be some issues in common application/kernel LibC files that > > use DEBUGASSERT (or ASSERT) > > > > This same discussion should also apply to include/debug.h which contains > > only nonstandard debug macros. Shouldn't it also be moved to > > include/nuttx/debug.h? > > > > >
Re: debugassert vs assert in apps
Here is the issue to track debug.h created before: https://github.com/apache/nuttx/issues/8986 On Wed, Jan 3, 2024 at 2:30 AM Gregory Nutt wrote: > > On Tue, Jan 2, 2024 at 11:16 AM Fotis Panagiotopoulos > > wrote: > >> DEBUGASSERT shall only be used for the kernel. > >> assert shall only be used for apps. > >> > >> Rationale: > >> > >> DEBUGASSERT is a custom macro that only exists in the NuttX world. > >> As such it is best being used in NuttX-specific code. > >> > >> assert on the other hand is a standard function, it exists in all > systems. > >> It seems better suited for generic and portable code (i.e. apps). > > We have no way to enforce this. What we do do is at least make it clear > that the application is using a kernel-only macro or function > inappropriately by putting the macro definition or function prototype in > NuttX private file. For example, > > * Remove the DEBUGASSERT (and ASSERT?) macros from include/assert.h > * Add them to include/nuttx/assert.h > * All applications that use DEBUGASSERT (or ASSERT) should be changed > to use assert() > * Kernel functions that use DEBUGASSERT (or ASSERT) would have to > explicitly include > > Not foolproof, but that is how we have dealt this this in the past. It > if is in include/assert.h, is is essentially being advertised as OK for > use by applications. This is logically equivalent to the Linux/LibC > conventions of putting Linux-specific files in /usr/include/linux vs. > normal, standard application files in /usr/include. > > There may be some issues in common application/kernel LibC files that > use DEBUGASSERT (or ASSERT) > > This same discussion should also apply to include/debug.h which contains > only nonstandard debug macros. Shouldn't it also be moved to > include/nuttx/debug.h? > >
Re: debugassert vs assert in apps
On Tue, Jan 2, 2024 at 11:16 AM Fotis Panagiotopoulos wrote: DEBUGASSERT shall only be used for the kernel. assert shall only be used for apps. Rationale: DEBUGASSERT is a custom macro that only exists in the NuttX world. As such it is best being used in NuttX-specific code. assert on the other hand is a standard function, it exists in all systems. It seems better suited for generic and portable code (i.e. apps). We have no way to enforce this. What we do do is at least make it clear that the application is using a kernel-only macro or function inappropriately by putting the macro definition or function prototype in NuttX private file. For example, * Remove the DEBUGASSERT (and ASSERT?) macros from include/assert.h * Add them to include/nuttx/assert.h * All applications that use DEBUGASSERT (or ASSERT) should be changed to use assert() * Kernel functions that use DEBUGASSERT (or ASSERT) would have to explicitly include Not foolproof, but that is how we have dealt this this in the past. It if is in include/assert.h, is is essentially being advertised as OK for use by applications. This is logically equivalent to the Linux/LibC conventions of putting Linux-specific files in /usr/include/linux vs. normal, standard application files in /usr/include. There may be some issues in common application/kernel LibC files that use DEBUGASSERT (or ASSERT) This same discussion should also apply to include/debug.h which contains only nonstandard debug macros. Shouldn't it also be moved to include/nuttx/debug.h?
Re: debugassert vs assert in apps
This is an excellent summary! I suggest this should be documented somewhere in Documentation, with a more summarized version in the comment block of these macros. This way, the discussion and its conclusions will not be forgotten in the future!! Happy New Year, Nathan On Tue, Jan 2, 2024 at 11:16 AM Fotis Panagiotopoulos wrote: > > Hello everyone, and happy new year! > > After discussing this with Tim, I would like to present my opinion on this > topic, in the hope we build better code conventions > (or maybe understand better how apps are developed in Nuttx, as I don't use > them often). > > So the idea is: > > DEBUGASSERT shall only be used for the kernel. > assert shall only be used for apps. > > Rationale: > > DEBUGASSERT is a custom macro that only exists in the NuttX world. > As such it is best being used in NuttX-specific code. > > assert on the other hand is a standard function, it exists in all systems. > It seems better suited for generic and portable code (i.e. apps). > > Benefits of this approach: > > 1. Kernel and apps development can be isolated. > It is very common to work only on one of the two parts of the system at a > time. > In some cases, different people (or teams) work on these two different > codebases, > so it is advantageous to only enable what you actually care for. > > 2. assert is more portable. > Generic applications shall remain portable (in the POSIX domain), and not > have dependencies on > "custom things". So one can easily grab a NuttX app and run it on another > system (e.g. Linux). > Or get an external library, not meant for NuttX, and run it directly and > unaltered. > I see minimal value in adding OS-specific macros in generic code. > > 3. The two macros may have different implementations. > Having two different macros with different scopes may allow as in the > future to have them implemented > optimally for their use case. > For example, DEBUGASSERT may provide more OS-specific information about the > time of crash, > reboot the system etc. > On the other hand assert may only terminate the offending task, or provide > more task-specific information etc. > Of course, I know how complicated this is. For the time I only discuss > about keeping the distinction and having > the future ability. > > 4. Code readability / maintainability. > Mixing up assert and DEBUGASSERT in the same code (i.e. in an app), is very > confusing. > I don't understand, why use one or the other? Are they different? Why be > inconsistent? > If they are identical now, will they remain so forever? > What if someone needs to make OS-specific changes to DEBUGASSERT. > > 5. Static code analysis. > There are several good static code analysis tools, and I use some of them. > They can mostly understand assert but not the custom DEBUGASSERT macro. > This causes headaches to re-define correctly DEBUGASSERT in the scope of > the static analysis tool. > Using non-standard macros I have seen checks to fail, or cause false > positives / negatives: > * About unused variables (that are only used in the asserts). > * About asserts with side effects (the tool does not check properly custom > macros) > * About NULL pointer dereferences (that however are checked by asserts). > and more... > > > > On Sun, Dec 17, 2023 at 7:04 AM Daniel Appiagyei > wrote: > > > While we’re on this topic I would also like to encourage authors to put > > only one condition in their asserts instead of multiple. > > So, DEBUGASSERT(cond1) > > ; DEBUGASSERT (cond2); (…) > > > > Instead of DEBUGASSERT(cond1 && cond2 &&..). > > > > When an assertion happens and there’s multiple conditions then I don’t know > > which one triggered it. > > Best, > > Daniel > > > > > > *Daniel Appiagyei | Embedded Software Engineer *Email: > > daniel.appiag...@braincorp.com > > *Brain* > > * Corp™ *10182 Telesis Ct, Suite 100 > > San Diego, CA 92121 > > > > (858)-689-7600 > > www.braincorp.com > > > > > > On Fri, Dec 15, 2023 at 3:40 AM Alan C. Assis wrote: > > > > > I think debugassert() is for finding BUGs during the development/testing > > > phase. > > > > > > In the other hand assert() should be put in case where something really > > > catastrofic is going to happen and cannot be avoid anyway. > > > > > > My personal opinion is that release code shouldn't have (or should have > > the > > > minimum as possible) assert() and PANIC(). You don't wasn't want your > > code > > > to fail in a critical moment, do you? > > > > > > So assert() code
Re: debugassert vs assert in apps
Hello everyone, and happy new year! After discussing this with Tim, I would like to present my opinion on this topic, in the hope we build better code conventions (or maybe understand better how apps are developed in Nuttx, as I don't use them often). So the idea is: DEBUGASSERT shall only be used for the kernel. assert shall only be used for apps. Rationale: DEBUGASSERT is a custom macro that only exists in the NuttX world. As such it is best being used in NuttX-specific code. assert on the other hand is a standard function, it exists in all systems. It seems better suited for generic and portable code (i.e. apps). Benefits of this approach: 1. Kernel and apps development can be isolated. It is very common to work only on one of the two parts of the system at a time. In some cases, different people (or teams) work on these two different codebases, so it is advantageous to only enable what you actually care for. 2. assert is more portable. Generic applications shall remain portable (in the POSIX domain), and not have dependencies on "custom things". So one can easily grab a NuttX app and run it on another system (e.g. Linux). Or get an external library, not meant for NuttX, and run it directly and unaltered. I see minimal value in adding OS-specific macros in generic code. 3. The two macros may have different implementations. Having two different macros with different scopes may allow as in the future to have them implemented optimally for their use case. For example, DEBUGASSERT may provide more OS-specific information about the time of crash, reboot the system etc. On the other hand assert may only terminate the offending task, or provide more task-specific information etc. Of course, I know how complicated this is. For the time I only discuss about keeping the distinction and having the future ability. 4. Code readability / maintainability. Mixing up assert and DEBUGASSERT in the same code (i.e. in an app), is very confusing. I don't understand, why use one or the other? Are they different? Why be inconsistent? If they are identical now, will they remain so forever? What if someone needs to make OS-specific changes to DEBUGASSERT. 5. Static code analysis. There are several good static code analysis tools, and I use some of them. They can mostly understand assert but not the custom DEBUGASSERT macro. This causes headaches to re-define correctly DEBUGASSERT in the scope of the static analysis tool. Using non-standard macros I have seen checks to fail, or cause false positives / negatives: * About unused variables (that are only used in the asserts). * About asserts with side effects (the tool does not check properly custom macros) * About NULL pointer dereferences (that however are checked by asserts). and more... On Sun, Dec 17, 2023 at 7:04 AM Daniel Appiagyei wrote: > While we’re on this topic I would also like to encourage authors to put > only one condition in their asserts instead of multiple. > So, DEBUGASSERT(cond1) > ; DEBUGASSERT (cond2); (…) > > Instead of DEBUGASSERT(cond1 && cond2 &&..). > > When an assertion happens and there’s multiple conditions then I don’t know > which one triggered it. > Best, > Daniel > > > *Daniel Appiagyei | Embedded Software Engineer *Email: > daniel.appiag...@braincorp.com > *Brain* > * Corp™ *10182 Telesis Ct, Suite 100 > San Diego, CA 92121 > > (858)-689-7600 > www.braincorp.com > > > On Fri, Dec 15, 2023 at 3:40 AM Alan C. Assis wrote: > > > I think debugassert() is for finding BUGs during the development/testing > > phase. > > > > In the other hand assert() should be put in case where something really > > catastrofic is going to happen and cannot be avoid anyway. > > > > My personal opinion is that release code shouldn't have (or should have > the > > minimum as possible) assert() and PANIC(). You don't wasn't want your > code > > to fail in a critical moment, do you? > > > > So assert() code could be used during an inicial validation phase in the > > field. But of course, if it is a HW issue, there is nothing your code can > > do except showing a BSOD. > > > > BR, > > > > Alan > > > > On Thu, Dec 14, 2023 at 11:50 AM Tim Hardisty > > wrote: > > > > > Hi, > > > > > > I'm wondering if there is an "approved" usage of assert vs debugassert > > > for files/apps in the nuttx-apps repo? > > > > > > My thinking is: > > > > > > * use debugassert if a function in apps would only fail if the > calling > > > code, probably (or possibly only) if other functions within apps, > > > rather than an unknown higher level app, have done something dumb > > > that should be picked up during nuttx-apps deve
Re: debugassert vs assert in apps
While we’re on this topic I would also like to encourage authors to put only one condition in their asserts instead of multiple. So, DEBUGASSERT(cond1) ; DEBUGASSERT (cond2); (…) Instead of DEBUGASSERT(cond1 && cond2 &&..). When an assertion happens and there’s multiple conditions then I don’t know which one triggered it. Best, Daniel *Daniel Appiagyei | Embedded Software Engineer *Email: daniel.appiag...@braincorp.com *Brain* * Corp™ *10182 Telesis Ct, Suite 100 San Diego, CA 92121 (858)-689-7600 www.braincorp.com On Fri, Dec 15, 2023 at 3:40 AM Alan C. Assis wrote: > I think debugassert() is for finding BUGs during the development/testing > phase. > > In the other hand assert() should be put in case where something really > catastrofic is going to happen and cannot be avoid anyway. > > My personal opinion is that release code shouldn't have (or should have the > minimum as possible) assert() and PANIC(). You don't wasn't want your code > to fail in a critical moment, do you? > > So assert() code could be used during an inicial validation phase in the > field. But of course, if it is a HW issue, there is nothing your code can > do except showing a BSOD. > > BR, > > Alan > > On Thu, Dec 14, 2023 at 11:50 AM Tim Hardisty > wrote: > > > Hi, > > > > I'm wondering if there is an "approved" usage of assert vs debugassert > > for files/apps in the nuttx-apps repo? > > > > My thinking is: > > > > * use debugassert if a function in apps would only fail if the calling > > code, probably (or possibly only) if other functions within apps, > > rather than an unknown higher level app, have done something dumb > > that should be picked up during nuttx-apps development and so should > > not make it through to final code; but if they do, that's the fault > > of the developer for not turning on debug asserts!! > > * use assert if the error would cause the obviously unknown > > higher-level calling app someone's writing to fail if the error was > > allowed to pass unchecked, so the developer of that higher level app > > can deal with it programmatically. > > > > That might almost suggest that debugassert() is for private functions > > and assert() is for public ones? > > > > Probably over-thinking it as usual, but I'm never afraid to ask :) > > > > Thanks! > > > > TimJTi. > > >
debugassert vs assert in apps
I think debugassert() is for finding BUGs during the development/testing phase. In the other hand assert() should be put in case where something really catastrofic is going to happen and cannot be avoid anyway. My personal opinion is that release code shouldn't have (or should have the minimum as possible) assert() and PANIC(). You don't wasn't want your code to fail in a critical moment, do you? So assert() code could be used during an inicial validation phase in the field. But of course, if it is a HW issue, there is nothing your code can do except showing a BSOD. BR, Alan On Thu, Dec 14, 2023 at 11:50 AM Tim Hardisty wrote: > Hi, > > I'm wondering if there is an "approved" usage of assert vs debugassert > for files/apps in the nuttx-apps repo? > > My thinking is: > > * use debugassert if a function in apps would only fail if the calling > code, probably (or possibly only) if other functions within apps, > rather than an unknown higher level app, have done something dumb > that should be picked up during nuttx-apps development and so should > not make it through to final code; but if they do, that's the fault > of the developer for not turning on debug asserts!! > * use assert if the error would cause the obviously unknown > higher-level calling app someone's writing to fail if the error was > allowed to pass unchecked, so the developer of that higher level app > can deal with it programmatically. > > That might almost suggest that debugassert() is for private functions > and assert() is for public ones? > > Probably over-thinking it as usual, but I'm never afraid to ask :) > > Thanks! > > TimJTi. >
debugassert vs assert in apps
Hi, I'm wondering if there is an "approved" usage of assert vs debugassert for files/apps in the nuttx-apps repo? My thinking is: * use debugassert if a function in apps would only fail if the calling code, probably (or possibly only) if other functions within apps, rather than an unknown higher level app, have done something dumb that should be picked up during nuttx-apps development and so should not make it through to final code; but if they do, that's the fault of the developer for not turning on debug asserts!! * use assert if the error would cause the obviously unknown higher-level calling app someone's writing to fail if the error was allowed to pass unchecked, so the developer of that higher level app can deal with it programmatically. That might almost suggest that debugassert() is for private functions and assert() is for public ones? Probably over-thinking it as usual, but I'm never afraid to ask :) Thanks! TimJTi.
