Astrobud atomic save - #166
Merged
Merged
Conversation
…Positions() - SaveActuatorPosFile now writes to a temp file, fsyncs, backs up the current canonical file via copy (not rename), then atomically renames temp over the canonical file. The canonical fastcat_saved_positions.yaml is never left missing or partially written, even if killed mid-save. - Add public SaveActuatorPositions() that locks parameter_mutex_ (shared with Process()) then captures + writes positions, so it is safe to call while the process loop is still running (e.g. from a ROS pre-shutdown callback). - Shutdown() now routes through SaveActuatorPositions().
- Add AllBrakesEngaged() (all incremental GOLD/PLATINUM actuators motor_on==0). - UpdatePositionFileOnBrakeState() runs at end of Process() (under lock): rising edge (motion stopped, brakes engaged) saves positions; falling edge (motion started) invalidates (removes) the canonical file so stale positions are never loaded. - InvalidateActuatorPosFile() removes fastcat_saved_positions.yaml, keeps _prev. - SaveActuatorPositions() (shutdown path) now gated on AllBrakesEngaged(): a save requested mid-motion invalidates instead of writing in-motion positions.
The brake-edge save ran fsync/rename/backup-copy synchronously on the RT Process() thread under parameter_mutex_, causing a ~60ms (6-cycle @100hz) cycle slip. Split the save: - BuildActuatorPosYaml(): cheap serialization on the RT thread under the lock. - WritePosFileToDisk(): all disk I/O, runs only on a dedicated writer thread. A single-slot coalescing mailbox (mutex+cv) hands the pre-built string to the writer; RT thread never blocks on disk. Brake edges are fire-and-forget; the shutdown save waits for the writer to drain (durability preserved). fsync and _prev backup are kept, just off the RT path. Writer starts in InitHardware, stops/joins in ~Manager. Falls back to inline write if writer not running.
#1 — Position-file bypass now honored on the save/invalidate paths Added pos_file_enabled_ (manager.h:324), set in LoadActuatorPosFile() only after both bypass checks pass (manager.cc:1148), and checked at the top of SaveActuatorPositions() and UpdatePositionFileOnBrakeState(). The bug was real and I reproduced it: with the pre-fix binary, a no-actuator topology that logged "bypassing saved positions file functions" went on to delete /tmp/fastcat_saved_positions.yaml anyway — first during InitHardware()'s Process() calls, then again on Shutdown(). Post-fix the sentinel file survives untouched with no invalidate logged. #2 — RT loop no longer stalls on fsync SaveActuatorPositions() (manager.cc:106) now releases parameter_mutex_ before blocking on the writer. Getting this right needed one more step than I first wrote: my initial version released the lock before posting, which opened a window where the RT loop could post its falling-edge invalidate first and have our later write supersede it — leaving a file full of pre-motion positions while the arm was actually moving. That reintroduces exactly the staleness the invalidate exists to prevent. The final version posts under parameter_mutex_ (cheap — mailbox mutex only, no disk) and defers only the wait, so ordering against the brake edges is preserved and the RT thread never blocks on I/O. PostPosWriteRequest/PostPosInvalidateRequest now return the request sequence, and a new WaitForPosWriter(seq) does the blocking. #3 — Header doc corrected manager.h:47 now states plainly that this must not be called from a signal handler, explains why (mutex + allocation aren't async-signal-safe; self-deadlock if the signal lands on the thread already inside Process()), and points to the flag-and-poll pattern test_cli.cc already uses. #5 — umask no longer clobbered process-wide Both umask(000) calls are gone. Files are chmod'd individually to kPosFileMode (0666) — the temp file before the rename, so the canonical file lands with final permissions already set and is never briefly unwritable. Verified with the shell umask at 022: a pre-existing 644 file was replaced by a 666 one, and the _prev backup is 666 too. The comment claiming umask is per-thread was wrong and is now corrected in place.
JosephBowkett
approved these changes
Aug 29, 2026
JosephBowkett
left a comment
JosephBowkett
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, but lacks description of the new parameter actuator_position_save_settle_sec inside doc/fastcat_device_config_parameters.md
StartPosWriter() seems to set the running flag before the thread actually runs, which might create an edge case if caught between those two things. Should probably flip the order
… atomic to block compiler from changing the order
Contributor
Author
|
both issues fixed
|
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
2 participants
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.
Fix some issues with save positions we had in Astrobud. We had save positions fail few times. It does not happen every time so it is hard to track. Once, it created the file but it was empty, meaning the process was killed in after it created the file but before it saved to file. In addition, previous save position keep invalid file while the arm was moving, and the positions would be lost if power was lost or the processs was forcefully closed. This PR minimizes the time we lose calibration to only when we lose power while the arm is moving.
This has been tested for hundreds of commands in Astrobud, initially killing the node to check behaviour was as expected, and then sometimes watching the saved positions file was destroyed and created.
Some edges cases that might require a different platform to test:
@nasa-jpl/nasa-jpl-ethercat-devs