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

Bash scripting for posix #5162

Closed
wants to merge 92 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@julianoes
Contributor

julianoes commented Jul 29, 2016

tl;dr

This adds bash scripting for POSIX. The startup file is now evaluated by bash.
Additionally, PX4 commands can be sent from other terminals, allowing commands such as px4-commander status or px4-listener sensor_combined from anywhere on the machine.

Description

This PR adds the possibility to spawn PX4 modules out of bash. Basically,
the main executable can now be started as a server/daemon or as a
client.
The server replaces the existing functionality of the main exe with
the pxh shell, however, it also opens a pipe that clients can talk to.

Clients can run or spawn PX4 modules or commands by connecting to the
server over the pipe. They clients will get the stdout and return value
of their commands via a client specific pipe back.

The main exe starts as a server and runs a startup script using bash. Each command in the startup script such as 'commander start' gets aliased with a prefix as 'px4-commander start' and symlinked to the px4 main executable.
If the main executable is called with the prefix 'px4-' which is passed in argv[0], it will act as a client and therefore send the command 'commander start' to the server. Once this command has been run in the server, the client will receive the return value and the bash script will continue with the next command.

Additionally, this allows commands to be sent from outside pxh, so from other terminals (on the same computer). All that is needed is to have the px4 exe and the symlinks in the PATH.

Thanks to @bkueng for the big help.

Let me know what you think: @jgoppert, @LorenzMeier, @dagar, @mcharleb

Adresses: #4800

Progress to get this PR in

  • Main SITL startup transferred to unified rcS startup script
  • Additional SITL startups into rcS
  • Check CI on jenkins
  • Script Snapdragon startup
  • Script Bebop startup
  • Script Raspberry Pi startup
  • Upload mixers
  • Stress test whole system with some scripts
  • Allow listener to keep looping
  • Add Devguide documentation

Future work after this PR

  • Unify NuttX and Posix startup

How to test

SITL

The normal SITL startup already uses the new unified SITL startup script rcS, it can get started with:

make posix gazebo
make posix jmavsim

To send a command in another terminal window, adapt the PATH:

export PATH=$PATH:~/src/Firmware/build_posix_sitl_default/bin

and send a command:

px4-commander status

Or listen to a uORB message:

px4-listener vehicle_attitude

Press Ctrl+C to exit.

Snapdragon

To test this on Snappy, just build and upload normally:

make eagle_default upload

Open adb shell, and start it with:

cd /home/linaro/bin
export PATH=$PATH:/home/linaro/bin
./px4 rcS

Then choose, the airframe config using the param SYS_AUTOSTART and restart. The current options are:

SYS_AUTOSTART 4100: generic (flight) Quad
SYS_AUTOSTART 4200: Microheli 200QX
@dagar

This comment has been minimized.

Member

dagar commented Jul 29, 2016

Very cool. For startup scripts I wonder if there's a subset of bash and nsh we could safely use and then completely unify startup.

@julianoes

This comment has been minimized.

Contributor

julianoes commented Jul 29, 2016

For startup scripts I wonder if there's a subset of bash and nsh we could safely use and then completely unify startup.

Yer that would be the plan. Most commands of the nsh are already in bash, however, I found set which is "missing".

@julianoes julianoes force-pushed the feature_shell branch to 8a1c88b Jul 29, 2016

@bharath374

This comment has been minimized.

Contributor

bharath374 commented Jul 29, 2016

Julian, this is a great solution!

One comment: The following lines should stay under "if param compare SYS_AUTOSTART 4100" since they're not been tested in other configurations.

qshell gps start -d /dev/tty-4
qshell df_hmc5883_wrapper start
qshell df_trone_wrapper start

@julianoes julianoes force-pushed the feature_shell branch 2 times, most recently from 4b60fd8 to c66315a Aug 1, 2016

@@ -160,6 +177,34 @@ endforeach()
install(TARGETS px4 DESTINATION bin)
install(DIRECTORY ${PROJECT_SOURCE_DIR}/ROMFS DESTINATION ${CMAKE_INSTALL_DATADIR}/${PROJECT_NAME})
install(DIRECTORY ${PROJECT_SOURCE_DIR}/posix-configs DESTINATION ${CMAKE_INSTALL_DATADIR}/${PROJECT_NAME})
install(DIRECTORY ${PROJECT_SOURCE_DIR}/posix-configs/SITL/init DESTINATION ${CMAKE_BINARY_DIR}/rootfs/etc/init)

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

Is this correct? CMAKE_BINARY_DIR seems wrong

This comment has been minimized.

@julianoes

julianoes Aug 9, 2016

Contributor

I'm gonna remove the install stuff again, not sure how this should work.

This comment has been minimized.

@jgoppert

jgoppert Aug 9, 2016

Contributor

@julianoes I think it would be worthwhile to rebase on #5255 now.

add_custom_command(TARGET px4
POST_BUILD
COMMAND cp -R ${PROJECT_SOURCE_DIR}/posix-configs/SITL/init ${CMAKE_BINARY_DIR}/rootfs/etc/

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

We shouldn't use the POST_BUILD for these. It does not handle dependencies & changes to the script files. We should explicitly enumerate these files and use the add_custom_command(OUTPUT command, similar to the .msg files.

Then instead of cp, better would be COMMAND ${CMAKE_COMMAND} -E copy

# Get last argument
for last; do true; done
echo "Creating folder structure..."
#echo "creating folder: $last"

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

debug leftover?

This comment has been minimized.

@julianoes

julianoes Aug 9, 2016

Contributor

Thx, fixed.

POST_BUILD
COMMAND ln -s -f ${TARGET} ${ln_name}
WORKING_DIRECTORY "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}"
USES_TERMINAL

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

I did not find USES_TERMINAL. Is it some magic undocumented variable?

This comment has been minimized.

@dagar

dagar Aug 3, 2016

Member

It's relatively new and only relevant for ninja. Without it ninja buffers all output only showing it once the command has finished.

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

ok, thanks. But then it only makes sense for commands with lots of expected output.

This comment has been minimized.

@julianoes

julianoes Aug 9, 2016

Contributor

Removed it since it's not really needed.

if (apps.find(command) != apps.end()) {
const char *arg[appargs.size() + 2];
/* Remove the prefix by moving argv[0] and 1 more for the 0 termination. */
memmove(argv[0], argv[0] + strlen(prefix), strlen(argv[0]) + 1);

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

how about argv[0] += strlen(prefix) ?

This comment has been minimized.

@julianoes

julianoes Aug 9, 2016

Contributor

Aha, of course, that's easier. 👊

px4::init_once();
px4::init(argc, argv, "px4");
run_startup_bash_script(commands_file);

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

This does not work with chroot for 2 reasons:

  • bash will not be found when chrooting
  • the commands file is opened for test before chrooting and then afterwards again. one of them will probably fail, since chroot changes the root.

do we even need chroot?

This comment has been minimized.

@julianoes

julianoes Aug 3, 2016

Contributor

Also, you don't have permission to chroot without being root, right?

I don't think we need chroot, and @LorenzMeier doesn't think either.

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

Then we can remove it altogether. Except if we have a use-case for the install...

This comment has been minimized.

@julianoes

julianoes Aug 9, 2016

Contributor

Removed.

pthread_t pthread_killed = _client_uuid_to_pthread[client_uuid];
int pipe_fd = _pthread_to_pipe_fd[pthread_killed];
close(pipe_fd);

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

should we unlink the pipe as well here? If I see correctly a client pipe is never deleted.

This comment has been minimized.

@julianoes

julianoes Aug 3, 2016

Contributor

We just unlink before trying to create it, is that not enough?

This comment has been minimized.

@julianoes

julianoes Aug 3, 2016

Contributor

Ah, that's the specific one, yer let's delete it.

return ret;
}
unlink(_recv_pipe_path);

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

I think this is a precaution in case a client with the same uuid exists/ed? I think we have to be careful here (however unlikely the case is): will this kill the other client? If so, we might be better off trying a different uuid.

This comment has been minimized.

@julianoes

julianoes Aug 3, 2016

Contributor

Good point, I'd say just fail in the case it exists already.

This comment has been minimized.

@bkueng
return;
}
_history.push_back(line);

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

use a maximum history length?

This comment has been minimized.

@julianoes

julianoes Aug 3, 2016

Contributor

Why, to limit RAM usage?

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

yes. if there is some automated process feeding in commands, we don't want the history to grow infinitely.

This comment has been minimized.

@julianoes

julianoes Aug 9, 2016

Contributor

Ok, will do.

_send_retval(pipe_fd, retval, client_uuid);
// Clean up before returning.
_instance->_cleanup_thread(client_uuid);

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

This is not thread-safe as it accesses the maps from another thread.

This comment has been minimized.

@julianoes

julianoes Aug 3, 2016

Contributor

So I need to add locking?

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

yes, that's probably the simplest way. or signal the server thread to remove the uuid, but this also requires a queue with synchronization.

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

yes, that's probably the simplest way. or signal the server thread to remove the uuid, but this also requires a queue with synchronization.

This comment has been minimized.

@julianoes

julianoes Aug 9, 2016

Contributor

Done.

// Check if we can write first by writing 0 bytes.
// If we don't do this, we'll get SIGPIPE and be very unhappy
// because the whole process will go down.
int ret = write(pipe_fd, NULL, 0);

This comment has been minimized.

@bkueng

bkueng Aug 3, 2016

Member

Is this still needed, even if we ignore SIGPIPE?

This comment has been minimized.

@julianoes

julianoes Aug 3, 2016

Contributor

Yes, ignoring SIGPIPE did not have the effect we wanted and this is still necessary.

@bkueng

This comment has been minimized.

Member

bkueng commented Aug 3, 2016

Finished reviewing, added some small obvious changes. Comments adress mostly small stuff, I think the cmake needs some more work. But overall really good code!

@julianoes

This comment has been minimized.

Contributor

julianoes commented Aug 3, 2016

@bkueng Thanks for the review!

@bkueng bkueng referenced this pull request Aug 4, 2016

Merged

SITL path cleanup #5181

5 of 5 tasks complete
@lucasdemarchi

This comment has been minimized.

Contributor

lucasdemarchi commented Aug 9, 2016

I think I'm probably late for this, but I was working on the current state of master branch and then I saw this PR. Quick question: why do you want to start the px4 components via arbitrary shell scripts? Wouldn't a declarative approach via configuration file (ini, json or other format) and then probably a single binary to control the internal parts be simpler?

@julianoes julianoes force-pushed the feature_shell branch from 63aedf4 to 38d2b4b Aug 9, 2016

@LorenzMeier

This comment has been minimized.

Member

LorenzMeier commented Aug 9, 2016

Because it allows you to follow the UNIX system design of a collection of small apps and mixing calls to the system with calls to the PX4 middleware.

It's easy to support a JSON file format as well, but that would be rather for fixed vehicle configurations (which is important for products).

I suggest you try this PR when it's done - running the uORB listener in a separate she'll and watching different topics in parallel is an extremely powerful development tool.

@lucasdemarchi

This comment has been minimized.

Contributor

lucasdemarchi commented Aug 17, 2016

Because it allows you to follow the UNIX system design of a collection of small apps and mixing calls to the system with calls to the PX4 middleware.

It's certainly an improvement over the current one. However since the wrappers are actually talking to another daemon (the px4 binary) that responds accordingly, it's more a client/server approach than a classical "collection of small apps". In this case I usually see people using a declarative approach, i.e. a config file, because it avoids tempting hacks to add lots of sleeps here and there, scripting all the startup sequence. It's more or less a similar scenario of init implementation in which everybody went away from pure script init (be it systemd, upstart, android init, MacOS, etc) to a more declarative approach, even if in some of them still exists limited support for scripting.

@julianoes julianoes force-pushed the feature_shell branch from 9c6dbf8 to 5791463 Apr 17, 2017

@julianoes

This comment has been minimized.

Contributor

julianoes commented Apr 17, 2017

Ok, I've rebased this once again. I was able to do make posix jmavsim and get it to takeoff. Most of the rest is presumably broken (again) though.

@dagar

This comment has been minimized.

Member

dagar commented Apr 26, 2017

@julianoes I'm really looking forward to this. What's needed to get it in? The PR is massive, are there smaller pieces we could start breaking off and pulling in?

@julianoes

This comment has been minimized.

Contributor

julianoes commented Apr 27, 2017

@dagar: no, I think we need to consume it in one piece. Next steps is porting all the startup scripts into one. Let me plan this for this weekend.

@TSC21

This comment has been minimized.

Member

TSC21 commented Aug 20, 2017

@julianoes do you have the required bandwidth to proceed with this. This would be a very important update for our interface with the flight controller.

@TSC21 TSC21 closed this Aug 20, 2017

@TSC21 TSC21 reopened this Aug 20, 2017

@LorenzMeier

This comment has been minimized.

Member

LorenzMeier commented Aug 20, 2017

@julianoes I'm closing this as stale. I would love to see this in by all means. But I think it would be good if you could break it down instead into one PR that adds the capability (but leaves the current startup scripts) and then iterate on breaking those down.

That should be much quicker and take much less of your time than re-architecting the complete startup of all targets.

Thanks!

@julianoes

This comment has been minimized.

Contributor

julianoes commented Aug 21, 2017

Agreed. I need a week of holiday for this one.

@julianoes julianoes deleted the feature_shell branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment