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

nuttx vs posix px4_task_spawn_cmd argc and argv #12104

Open
jbeyerstedt opened this issue May 29, 2019 · 12 comments
Open

nuttx vs posix px4_task_spawn_cmd argc and argv #12104

jbeyerstedt opened this issue May 29, 2019 · 12 comments
Assignees

Comments

@jbeyerstedt
Copy link
Contributor

jbeyerstedt commented May 29, 2019

tl;dr:
px4_task_spawn_cmd sets the argc and argv variables differently on Nuttx and Posix. Nuttx is correct and includes the command name in argv[0]. But the Posix-Implementation only adds the arguments to argv, which is not, what's commonly used.
It's the same on macOS and Linux.

Describe the bug
I took the hello example application as a staring point for my application. Then I noticed, that the argc variable is not the same on nuttx and posix inside a newly spawned task (with px4_task_spawn_cmd).

The task is spawned like this:

px4_task_spawn_cmd("hello",
                   SCHED_DEFAULT, SCHED_PRIORITY_MAX - 5, 2000,
                   PX4_MAIN,
                   (argv) ? (char *const *)&argv[2] : (char *const *)nullptr);

When running hello start, nuttx will set argc and argv to these values inside the task:

  • argc = 1
  • argv[0] = hello

On posix on the other hand:

  • argc = 0
  • argv[0] is empty

When running hello start -h, nuttx will set argc and argv to these values inside the task:

  • argc = 2
  • argv[0] = hello
  • argv[1] = -h

On posix on the other hand:

  • argc = 1
  • argv[0] = -h
  • argv[1] is empty

To Reproduce
Steps to reproduce the behavior:

  1. Take the hello example app
  2. Add some code to output the argc and argv variables
  3. Run the application with different amounts of arguments in Nuttx and Posix
  4. See error

Expected behavior
The argument order and count would be the same on Nuttx and Posix.

@julianoes julianoes added the bug label May 29, 2019
@julianoes
Copy link
Contributor

Thanks for the issue. This sounds familiar. I always believed that NuttX had it wrong and we needed to work around that. @LorenzMeier can you comment?

@ghost
Copy link

ghost commented May 30, 2019

I was under the impression that this was an expected behavior from Nuttx. Nuttx task_create adds the task name as the first argument.

I thought this is why run_trampoline in ModuleBase class has the following code.
#ifdef __PX4_NUTTX argc -= 1; argv += 1; #endif

But I admit, this has bothered me for some time and I had to take extra precaution in my code for arg parsing for Nuttx and posix

@LorenzMeier
Copy link
Member

LorenzMeier commented May 30, 2019

Yes, we worked around non-standard behaviour in NuttX (which might have changed by now). Please also keep in mind that we're internally not storing the task name for the PX4 app classes, as the idea is that the arguments are a list of the arguments and not the POSIX interpretation of it. But it should behave the same on all platforms and it looks like it needs a closer look. Something is definitely off.

@jbeyerstedt
Copy link
Contributor Author

Just to be clear: The argc and argv are consistent in both NuttX and POSIX and the same as "standard" POSIX (application name as first argument), when an application's main function is called. Just the task creation in POSIX does not fit all of the other argument lists.
Therefore I would recommend to change the SITL/ POSIX implementation, because it's the only part, that does not match the other ones. The issue it, that any change will beak some applications, but I think, that breaking SITL is better than breaking code for the FMU.
This would be easier, even if Lorenz said, that the initial design was meant to be something different.

@julianoes
Copy link
Contributor

I can confirm that the behavior is different:

NuttX:

nsh> hello start -h 400 -f 200

argc: 5
0: hello
1: -h
2: 400
3: -f
4: 200

Posix:

pxh> hello start -h 400 -f 200

argc: 4
0: -h
1: 400
2: -f
3: 200

@julianoes
Copy link
Contributor

From what I found we would have to add:

 #ifdef __PX4_NUTTX argc -= 1; argv += 1; #endif

for consistency in all _main() functions where we are using argc, argv. Therefore I tend to agree with @jbeyerstedt that we should just use the POSIX convention for SITL as well.

@julianoes julianoes self-assigned this Jun 6, 2019
@stale stale bot added the Admin: Wont fix label Sep 4, 2019
@PX4 PX4 deleted a comment from stale bot Sep 4, 2019
@stale stale bot removed the Admin: Wont fix label Sep 4, 2019
@stale
Copy link

stale bot commented Dec 3, 2019

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Dec 3, 2019
@jbeyerstedt
Copy link
Contributor Author

still a bug (even with an open pull request)

@stale stale bot removed the stale label Dec 3, 2019
@stale
Copy link

stale bot commented Mar 3, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Mar 3, 2020
@jbeyerstedt
Copy link
Contributor Author

@julianoes Should this still be open, because you PR is also still open?

@stale stale bot removed the stale label Mar 6, 2020
@julianoes
Copy link
Contributor

Yes, still on my todo list 😕

@stale
Copy link

stale bot commented Jun 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants