Make.defs: Fix to exclude loadable apps from builtin app list

[masayuki2009]

Summary

• When an application is built as a loadable app, the application name should not be shown in the builtin app list (i.e. apps/builtin/buitlin_list.h) This PR does not create .bdat as well as .pdat if an application is built as a loadble.

Impact

• This PR affects application build for both builtin and loadable.

Testing

• I checked this PR with spresense:rndis and spresense:wifi which include both builtin apps and loadable apps.

[masayuki2009]

I only tested on Ubuntu. However, I applied the changes to Windows native build as well (please see the changes).

[xiaoxiang781216]

@anchao keep the entry intentionly because .bdat contain one important information: the main stack size. Without this information, some application which need a bigger stack size can't start up correctly.
We discuss a better solution internally: embed this type of information into ELF(maybe a special section), but hasn't time to implemnt.
It's better to keep the current state before we fix this issue.

[masayuki2009]

@anchao keep the entry intentionly because .bdat contain one important information:
the main stack size. Without this information, some application which need a bigger stack size >can't start up correctly.

@xiaoxiang781216 However, what happens if we disable builtin apps (CONFIG_NSH_BUILTIN_APPS=n) but enable loadable apps (CONFIG_NSH_FILE_APPS=y)

[xiaoxiang781216]

@anchao keep the entry intentionly because .bdat contain one important information:
the main stack size. Without this information, some application which need a bigger stack size >can't start up correctly.

@xiaoxiang781216 However, what happens if we disable builtin apps (CONFIG_NSH_BUILTIN_APPS=n) but enable loadable apps (CONFIG_NSH_FILE_APPS=y)

If I remeber correctly: @anchao also modify the launch code a little bit, so nsh will reference this information regardless wether CONFIG_NSH_BUILTIN_APPS iis on or off.

[xiaoxiang781216]

After @anchao review the related code, nuttx side has a bug which block what we want, @masayuki2009 here is the patch fix, please take a look:
apache/nuttx#333

[patacongo]

I started to merge this into the pr80 branch. But it looks like there is some discussion.

What is the resolution? Should I merge this? Or should it be closed with merging?

[xiaoxiang781216]

Yes, this patch may make the application which need the bigger stack size can't launch. Before we get the better solution, it's better to keep the current state.

[patacongo]

It is merged on the pr80 branch, but has not been merged to master. Sorry I did not read the discussion completely. Please advise.

[xiaoxiang781216]

It's better to close without merge before @masayuki2009 find the method resolve the issue I mention in this thread.

[patacongo]

I cannot close without merge, but I can remove the pr80 branch without merging to master. That is effectively the same thing... except that the PR cannot be re-opened.

I need to read comments more carefully. There are more PRs than I can handle and I do not always do a perfect job at merging them.

[masayuki2009]

@xiaoxiang781216 @patacongo I've just read what you discussed while asleep.

After @anchao review the related code, nuttx side has a bug which block what we want, >@masayuki2009 here is the patch fix, please take a look:
apache/nuttx#333

I checked apache/nuttx#333 and I think this would be a workaround to load an application with big stack size.

However, apache/nuttx#333 does not solve this issue (i.e. When an application is built as a loadable app, the application name should not be shown in the builtin app list).

I think my this PR can solve the above issue and now can co-exist with apache/nuttx#333 .

What do you think?

[xiaoxiang781216]

@xiaoxiang781216 @patacongo I've just read what you discussed while asleep.

After @anchao review the related code, nuttx side has a bug which block what we want, >@masayuki2009 here is the patch fix, please take a look:
apache/incubator-nuttx#333

I checked apache/incubator-nuttx#333 and I think this would be a workaround to load an application with big stack size.

However, apache/incubator-nuttx#333 does not solve this issue (i.e. When an application is built as a loadable app, the application name should not be shown in the builtin app list).

I think my this PR can solve the above issue and now can co-exist with apache/incubator-nuttx#333 .

What do you think?

We need a new method which remove loadable apps from help but keep the info in .pdat, so the fix could modify cmd_help to skip the item with null entry pointer.
But there is a side effect I need point out:
since the entry finding match the whole string, this workaround work only when user enter something like:
nsh >hello
The full path name can't work:
nsh >/mnt/hello
With the help output plus the search path(PATH=/mnt), user naturally issue the first format.

[masayuki2009]

Hi, @xiaoxiang781216,

The full path name can't work:
nsh >/mnt/hello

Oh, really?
I tried with my patch and it works as I expected.

nsh> help
help usage: help [-v] []

[ cp exec ifup mksmartfs reboot time
? cmp exit kill mh rm true
addroute dirname false ls mount rmdir uname
arp date free mb mv route umount
basename dd help md5 mw set unset
break delroute hexdump mkdir nslookup sh usleep
cat df ifconfig mkfatfs ps sleep wget
cd echo ifdown mkrd pwd test xd

Builtin Apps:
webserver ping ostest nsh <== no hello apps shown
nsh> hello
Hello, World!! <== hello ELF apps at /mnt/sd0/bin works
nsh> mv /mnt/sd0/bin/hello /mnt/sd0/hello <== move the file to /mnt/sd0
nsh> hello
nsh: hello: command not found <== no command found (because CONFIG_PATH_INITIAL="/mnt/sd0/bin")
nsh> /mnt/sd0/hello
Hello, World!! <== command with full path works
nsh>

[xiaoxiang781216]

BTW, the workaround also require that we add the binary folder to PATH, otherwise we can't launch the program by short name.

[masayuki2009]

BTW, the workaround also require that we add the binary folder to PATH,
otherwise we can't launch the program by short name.

Yes. I know.

nsh> env
PATH=/mnt/sd0/bin <== It's the same as CONFIG_PATH_INITIAL

[xiaoxiang781216]

Hi, @xiaoxiang781216,

The full path name can't work:
nsh >/mnt/hello

Oh, really?
I tried with my patch and it works as I expected.

I mean the workaround which pass the big stack size to posix_spawn can't work. Yes, we can still launch the program but with the default and small stack size.
hello is handled by nsh_builtin(->exec_builtin->posix_spawn)
/mnt/hello is handled by nsh_fileapp(->posix_spawn)
The stacksize workaround happen inside exec_builtin, so the 2nd case can't get the correct stack size.

nsh> help
help usage: help [-v] []

[ cp exec ifup mksmartfs reboot time
? cmp exit kill mh rm true
addroute dirname false ls mount rmdir uname
arp date free mb mv route umount
basename dd help md5 mw set unset
break delroute hexdump mkdir nslookup sh usleep
cat df ifconfig mkfatfs ps sleep wget
cd echo ifdown mkrd pwd test xd

Builtin Apps:
webserver ping ostest nsh <== no hello apps shown
nsh> hello
Hello, World!! <== hello ELF apps at /mnt/sd0/bin works

with your patch, stack size will be ELF_STACKSIZE
without, stack size will be EXAMPLES_HELLO_STACKSIZE

nsh> mv /mnt/sd0/bin/hello /mnt/sd0/hello <== move the file to /mnt/sd0
nsh> hello
nsh: hello: command not found <== no command found (because CONFIG_PATH_INITIAL="/mnt/sd0/bin")
nsh> /mnt/sd0/hello
Hello, World!! <== command with full path works

with/without your patch, ELF_STACKSIZE is used.

nsh>

so the workaround can pass the right stack size(EXAMPLES_HELLO_STACKSIZE) in hello case, but fail with full path(/mnt/hello).

[masayuki2009]

@xiaoxiang781216 Thanks for the detailed explanation on how stack size will be affected with/without the patch. Just for your information. In our products, we added a new environment value to change ELF_STACKSIZE to load an application which needs a large stack.

[masayuki2009]

See #158 as well.