curiosity_rover_ogma: Introduce Ogma demonstration (#159) - #152
curiosity_rover_ogma: Introduce Ogma demonstration (#159)#152ivanperez-keera wants to merge 4 commits into
curiosity_rover_ogma: Introduce Ogma demonstration (#159)#152Conversation
4ae8b1f to
262072e
Compare
5b4e39a to
cf8a255
Compare
curiosity_rover_ogma: Introduce Ogma demonstration (#159).
curiosity_rover_ogma: Introduce Ogma demonstration (#159).curiosity_rover_ogma: Introduce Ogma demonstration (#159)
df782c2 to
099d81f
Compare
8270091 to
2c4ee0c
Compare
41794e3 to
50ef362
Compare
Add dockerfile that builds Ogma. Add build scripts that build both Ogma and the generated app.
50ef362 to
24109c7
Compare
asimonov
left a comment
There was a problem hiding this comment.
Nice addition — having a worked Ogma/Copilot example against an existing demo makes the tool much easier to approach than the standalone examples.
I built and ran this locally on aarch64, and it behaves as documented: build.sh completes, the generated copilotrv node comes up, and driving the rover past |x| = 2.5 starts the stream of Empty messages on /copilot/handlerKeepRoverInCheck, which stops again when the rover returns inside the bound. Good to have confirmation beyond the x86 CI run.
A few things I'd like to see addressed before merge:
1. Leftover debug command in the Dockerfile
Dockerfile, in the generation step:
RUN cd /tmp/space-ros && \
ogma ros --project manifest/project.ogma && \
find && \
cd /tmp/space-ros/monitor/copilot/src/ && \
runhaskell Copilot.hsThe bare find dumps the whole tree into the build log — looks like it was meant to be removed.
2. monitor/ should be in .gitignore
build.sh writes the entire generated ROS package into ./monitor via docker build --output type=local,dest=./monitor. After one build, git status shows:
?? curiosity_rover_ogma/monitor/
The .gitignore added here is generic Haskell boilerplate (dist, .stack-work/, *.hi, …), none of which matches anything in this directory — the one entry it actually needs is monitor/. A .dockerignore for the same path would also help, since once monitor/ exists it gets shipped as build context on every subsequent build.
3. Unpinned dependencies
RUN cabal install --lib copilot copilot-c99 ...
RUN git clone https://github.com/nasa/ogma
RUN cd ogma && cabal install ogma-cli:ogmaNeither the Copilot libraries nor the Ogma checkout are pinned. Because the clone sits in its own layer it's also cached indefinitely, so in practice this is both irreproducible and not tracking upstream. Since .github/workflows/docker-image.yml auto-discovers every */build.sh and runs it on every PR to this repo, an upstream change to Ogma or a Hackage release can break CI for unrelated PRs. A pinned tag/commit for Ogma plus a cabal.project.freeze (or --constraint flags) would make this stable.
Minor / optional:
build.shpasses--build-arg VCS_REF=and--build-arg VERSION=to all three builds, but neither thisDockerfilenor Ogma's generated one declares thoseARGs, so Docker warns about unconsumed build args. Looks like copy-paste from the other demos.- The first
docker build --target builder -t nasa/ogma:latest .appears redundant — the followingdocker build --output ...builds that stage anyway. It also claims thenasa/ogmaname in the local image store, which could be confusing if that image is ever published. - README:
# Compilationand# Executionshould probably be##under the top-level heading; some fenced blocks carry$prompts and others don't, which makes copy-paste inconsistent; anddocument.jsonis referenced without itsmanifest/prefix. - README says "we need at least 4 terminals", but Terminal 1 is
docker compose up -d, which returns immediately — so it's really 3. - Worth adding this demo to the list in the root
README.md(that list is already missing a few others, so no strong feelings).
Things I checked that are fine: the RMW/network wiring works out (the generated image inherits RMW_IMPLEMENTATION=rmw_cyclonedds_cpp from osrf/space-ros:curiosity_demo, and both containers use host networking); the container name in the README matches what run.sh derives via tr '/' '_'; and /model/curiosity_mars_rover/odometry and /move_forward both exist in the rover demo as referenced.
Introduce demonstration of code generated automatically using Ogma to protect against a (fictitious) unwanted situation in the Curiosity rover demo.
Fixes #159.