Repository navigation
SIM-37: bake the AirSim ROS 2 wrapper into the image - #63
Merged
Merged
Conversation
It was built into a RUNNING container by build_airsim_wrapper.sh, at /airsim_root, so it did not survive a teardown -- and sim_up.sh never rebuilt it. A freshly brought-up stack therefore had no airsim_ros_pkgs at all: perception.launch.py died with "package not found", and there were no camera, depth, LiDAR, AirSim-IMU, magnetometer or odometry topics. Measured before: verify_sensors.py on a fresh CitySample stack reported 9 of 9 checks FAILED, "no messages" -- indistinguishable from a world whose sensors are broken. Measured after, on a cold stack with NO build_airsim_wrapper.sh and no manual source lines: airsim_ros_pkgs present, "AirsimROSWrapper Initialized!", and all 14 checks passing. The image is the right home because the wrapper is version-locked to the pinned Cosys-AirSim SHA (5.8-v3.4.1 / a552dd6c) -- the same argument px4_msgs and px4_ros_com already make in that Dockerfile -- and because building it per bring-up would cost ~90 s a seed, an hour across a 40-seed gate. Only the five subtrees the build reads are copied, ~52 MB of a 2.4 GB vendored tree; the image went 4.40 GB to 4.60 GB. Patch routing in the Dockerfile matches scripts/vendor_patches.sh: the DIFF HEADER, never the prose, because 0005 names the Unreal plugin path four times in its explanation and twice in a +++ b/ line. The build fails if zero ROS-side patches applied, and asserts the same three post-conditions the shell script checks. ros-profile.sh sources the overlay too, which was nearly missed: the package can be present and `ros2 launch bringup perception.launch.py` still fail with "package not found", because docker exec bypasses the entrypoint -- the exact gap that profile exists to close. Guarded on the file so an older image still opens a usable shell. build_airsim_wrapper.sh is kept: it still builds into a running container, which is how a wrapper patch gets tested without rebuilding the image.
…at was the point
Seven findings. The first is a self-inflicted break of the goal this ticket claims
to advance.
vendor/ is GITIGNORED and reconstructed by `vcs import` in quickstart step 0.2,
while the images build in step 0.1. So COPYing vendor/Cosys-AirSim into the image
meant a fresh clone died at "not found in build context" -- the image stopped
building from the repo alone, in a change whose stated purpose was to make a fresh
machine reach a working stack.
The first fix for that was also wrong: cloning at the pinned SHA failed in colcon,
because external/rpclib, AirLib/deps and AirLib/lib are not in git. Chasing that
down produced the actual shape of the problem, and it is much smaller than it
looked: the wrapper does NOT link the prebuilt libAirLib.a. CMakeLists.txt:31 does
add_subdirectory(${AIRSIM_ROOT}/cmake/AirLib), so colcon compiles AirLib from
source with this image's gcc, exactly as it already did when the wrapper was built
inside a running container. AirLib/include, AirLib/src and cmake/ are all tracked.
Only two DOWNLOADS are missing -- rpclib and eigen -- and setup.sh fetches them.
So the image now clones at the pinned SHA, sparse to the five subtrees the build
reads, and fetches rpclib 2.3.1 and eigen 3.4.1r from the URLs setup.sh names,
asserting each unpack. Fresh clone builds, no ordering rule, no cache, no hidden
state, and no second toolchain.
Cloning also makes the pin CHECKABLE: test "$(git rev-parse HEAD)" = "$COSYS_SHA",
the same assertion the XRCE block already makes. A COPY of a working tree carries
no .git, so the SHA echoed into /etc/drone-sim-versions asserted a provenance
nothing could verify -- an operator with local edits would ship an image whose
manifest named a commit it did not contain.
The Dockerfile no longer inlines the patch-routing predicate. It COPYs
scripts/vendor_patches.sh and calls vp_list and vp_is_unreal_side, so SIM-25's
single ownership holds across the image boundary instead of gaining a fourth copy.
It also asserts patch 0003's post-condition, which the PR description claimed and
the code omitted: without it, parking 0003 into experimental/ would silently ship a
wrapper with no per-timer callback groups.
build_airsim_wrapper.sh does `rm -rf $ROOT` unconditionally, so five documents
telling readers to run it as a routine step would now DESTROY the baked wrapper --
and if the script then failed, leave the container with no sensor graph where the
image had a working one. All five corrected, plus a runtime notice in the script
itself. It is kept, because it is still how a wrapper patch gets tested without a
four-minute image rebuild.
Adds .dockerignore: the full 2.4 GB vendor/ tree was being shipped to the daemon as
build context for every image build, which the earlier comment about build context
had half wrong.
VERIFIED: image builds exit 0 from the clone path, /etc/drone-sim-versions carries
the verified SHA, and a cold stack with no build_airsim_wrapper.sh and no manual
source lines passes 14 of 14 verify_sensors checks. Image 4.59 GB.
Two things: the second review pass on the wrapper-in-the-image change, and a
measurement that closes SIM-32's open question.
REVIEW FIXES
I claimed more than landed, in eight places: "a stack brought up by sim_up.sh
alone has the sensor graph" is false. Nothing starts airsim_node, so a cold stack
has the PACKAGE and zero /airsim_node/* topics until someone runs
perception.launch.py. The follow-on claim -- that this lets a scenario record
sensor topics and get data -- did not follow either; you would record empty
topics. Corrected everywhere to "installed, still launch it".
Seven doc notes were inserted by regex and never read. They landed inside a
callout about PX4 image size, between a lead-in and the bullet list it introduced,
under a "Known gap" heading about a different subsystem, and in a file that never
mentions the script -- so "this script" had no antecedent -- while the text they
contradicted stayed put, leaving several files asserting both things at once. All
seven reverted and rewritten by hand, against the specific sentence that was
actually wrong in each.
A safeguard that destroyed what it guarded: build_airsim_wrapper.sh printed
"Ctrl-C now if that is not what you meant" and then rm -rf'd /airsim_root within a
second. It refuses now unless FORCE=1.
WORKDIR had been left at /airsim_root, silently changing the image's default
working directory from /ros2_ws -- an unintended change to the container's public
interface. Restored.
The fetched rpclib and eigen versions were hand-copied from upstream setup.sh with
nothing asserting they still match the pinned tree. rpclib fails loudly (the
directory name is hardcoded in CommonSetup.cmake); eigen would drift SILENTLY.
Both asserted, verified against the built image first.
Also: docs/vendor/cosys-airsim.md now records the Dockerfile as the primary
applier of the ROS-side patches, the login-shell comment moved back to the COPY it
describes, and the stale 4.39 GB figure is corrected to 4.59 GB in quickstart.md,
quickstart.html and architecture.html (row and SVG label). The historical
size-reduction table in todo.md is deliberately left alone -- it records what the
image was.
Adds the worklog and render the branch was missing, and an end-to-end flight,
because ros-profile.sh now changes EVERY login shell including the bag recorder's
and the controller's: 1-seed Blocks gate PASS, worst 0.787 m, chase video
recorded.
SIM-32: THE STORAGE HYPOTHESIS IS WRONG
CitySample copied to the internal NVMe (119 GB), one seed of
citysample-updown-30m flown from each disk, same scenario and seed:
spinning disk NVMe
max landing split +107.41 m +107.56 m
samples split >0.5 m 65% 65%
collisions 1343 1337
seed wall time 890 s 331 s 2.7x faster
0.15 m on 107 m is noise and the split percentage is identical, WHILE the world
loaded 2.7x faster. The far-field surrogate is not proxy geometry lingering
because the disk is slow -- the integrator refuses that contact as ground however
quickly the real cells arrive. So "slow storage sets the blast radius" is refuted
along with the rest; the entry now says so and tells the next reader not to repeat
the experiment. The NVMe copy is deleted.
Worth keeping from it: CitySample work is 2.7x faster from the NVMe. That is a
scheduling choice, not a correctness one.
…he builder
Eight findings. The two HIGHs are the safeguard I added last round turning into a
regression.
build_airsim_wrapper.sh now dies unless FORCE=1 when a wrapper is already present
-- which, after this branch, is ALWAYS true on a stack from drone-sim/ros2. Three
scripts call it unconditionally right after sim_up.sh and treat non-zero as fatal:
run_park_tour.sh || die "wrapper build failed"
soak_full_stack.sh || { log "wrapper build FAILED"; exit 1; }
measure_sensor_rates.sh || { log "wrapper build failed"; return 1; }
So the project's documented end-to-end path aborted immediately after bring-up and
never flew, the soak exited before starting, and every measure() iteration
returned 1 with no measurements. The call is removed from all three rather than
given FORCE=1: the wrapper is in the image, and the perception launch each of them
already runs on the next line is the only step still needed.
The Reentrant post-condition could never fail the build. `! grep -q …` is exempt
from set -e -- bash explicitly exempts a command whose status is inverted with `!`
-- so a partially applied 0001, or upstream adding a second callback group, would
have shipped green with the data race 0001 exists to prevent. Verified the
exemption empirically, then replaced it with a grep -c comparison.
README section 4 still told readers to run the builder, which now exits FATAL, so
a fresh reader could not get past step 4. hello_drone.py's runtime hint sent users
to the same failing command, and its docstring and ros2_ws/README.md both still
described the build-after-every-bring-up workflow. perception.launch.py's own
docstring required a build step that no longer exists -- in the same package whose
README this branch had just rewritten to point at that launch file.
Smaller: docs/quickstart.html's table row kept the old description after the size
was corrected, and sim_up.sh's vendor mount comment justified the mount by a
reason that is no longer true, which would invite a future cleanup to remove a
mount that build_airsim_wrapper.sh (FORCE=1) and soak_full_stack.sh still need.
192 tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The wrapper was built into a running container by
build_airsim_wrapper.shat/airsim_root, so it did not survive a teardown — andsim_up.shnever rebuilt it. A freshly brought-up stack therefore had noairsim_ros_pkgsat all:perception.launch.pydied with "package not found", and there were no camera, depth, LiDAR, AirSim-IMU, magnetometer or odometry topics.The failure mode is why this is a ticket
Before:
verify_sensors.pyon a fresh City Sample stack reported 9 of 9 checks FAILED, "no messages" — indistinguishable from a world whose sensors are broken. They were fine; the graph was never running.After, on a cold stack with no
build_airsim_wrapper.shand no manual source lines:Why the image, not a bring-up step
5.8-v3.4.1/a552dd6c) — the same argumentpx4_msgsandpx4_ros_comalready make in that Dockerfile.Only the five subtrees the build reads are copied — ~52 MB of a 2.4 GB vendored tree. Image: 4.40 → 4.60 GB.
Patch routing in the Dockerfile matches
scripts/vendor_patches.sh: the diff header, never the prose, because0005names the Unreal plugin path four times in its explanation and twice in a+++ b/line. The build fails if zero ROS-side patches applied, and asserts the same three post-conditions the shell script checks.The part I nearly missed
ros-profile.shsources jazzy and/ros2_wsbut not the new overlay. The package can be present andros2 launch bringup perception.launch.pystill fail with "package not found", becausedocker execbypasses the entrypoint — which is the exact gap that profile exists to close. Added, guarded on the file so an older image still opens a usable shell.build_airsim_wrapper.shis kept: it still builds into a running container, which is how a wrapper patch gets tested without rebuilding the image.What this unblocks
A scenario can now list sensor topics in
record_topicsand actually get data —record_topicsis already generic, and its own comment anticipated this: "a scenario with obstacles will want depth and the planner's trajectory, and that should not require editing the harness."