docs: write down the SharedData convention the SDK already ships - #270
docs: write down the SharedData convention the SDK already ships#270tobymurray wants to merge 3 commits into
Conversation
StrideLut and OutdoorStrideCalibrator both default to ../SharedData/stride.json, and Running and Treadmill are both consumers of it, but the directory appears nowhere in Docs/. A third app has no way to learn the convention exists, or the two rules that make it work: mkdir before opening (f_open does not create parents), and treat every read as optional because a new watch has never written the file. Documents what the source demonstrably does, cited line by line, plus the simulator's unclamped ".." putting the shared files beside Output/ rather than inside it. Five things I could not settle from the SDK source, and so left out of the page rather than guess at. Answers would each turn into a paragraph: 1. Where does SharedData/ sit on the device? deploy.md puts apps under Apps/<AppName>/, which would make it Apps/SharedData/, but the SDK never says so and app paths are sandbox-relative. 2. Does it survive uninstalling the app that created it? If not, removing and reinstalling Running costs the user their calibration. 3. Is it visible and writable over USB mass storage? Decides whether data can be seeded from a desktop, and whether a user can clear a bad calibration by hand. 4. What happens when two apps write the same file at once? Whether the filesystem offers atomic rename, and whether any locking exists. 5. Is the name reserved -- can an app create a directory that collides with it?
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded Architecture documentation for cross-app ChangesSharedData documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Docs/shared-data.md`:
- Around line 75-80: Update the “Expect interrupted writes” guidance so
stride_deleted.json is described only as a pre-deletion backup for deletion
recovery, not as protection during replacement. Document a separate actual
replacement protocol for stride.json that provides atomic replacement or ensures
truncated files are rejected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa1cb156-e14c-40ba-b622-926e99839a7d
📒 Files selected for processing (2)
Docs/index.rstDocs/shared-data.md
The page landed the SDK-visible half of the convention accurately. This adds the half that is not visible from this repo, plus two corrections to claims the SDK source does not support. The load-bearing omission is that "../SharedData/" is a whitelist, not parent traversal. The kernel resolves "../SharedData" and "../SharedData/<name>" and rejects every other ".." segment outright, so "../MyMaps/tiles.bin" never resolves at all. The page previously said only that paths are relative to the app root, which reads as though any sibling would work. The simulator makes that worse: it concatenates prefix and path with no whitelist, so an invented path works there and fails on the watch with no warning. Both are now stated. Corrections: - stride_deleted.json was cited as backup-before-replace. It is not. Treadmill writes it before an explicit user-initiated clear, never before a routine save. The calibrator overwrites stride.json in place with truncation, so a power loss during a save leaves a torn file and no backup at all. The .bak is written later, after a load has already found the store unparseable -- recovery evidence, not protection. RecordingMarker holds the pattern worth copying, and is now cited in its place. - stride.json.bak was missing from the file table. It is a fourth real file in the directory and anyone enumerating it will meet it. Answers to the five open questions, resolved against the kernel and confirmed on a watch over USB mass storage: 1. On device it is Apps/SharedData/, a sibling of Apps/Running/ and the rest. 2. It survives app removal by construction, being outside every app's root. Only a factory reset clears it, and nothing garbage-collects it otherwise -- which is free for calibration and a trap for anything map-sized. 3. Visible and writable over USB mass storage. 4. No atomic rename: rename onto an existing name fails, so write-temp-then- rename does not work. Locking does exist, in two layers -- the volume is serialised by a mutex, and a global table refuses a second open of a file already open for writing. That table is ten entries for the whole watch, shared with the syslog and activity recorder, which matters to any app wanting several files open at once. 5. The name is reserved by the resolver rule in (1). Also documents that IFile::open() flattens absent, locked, timed-out and too-many-open-files into one false, so the "every read is optional" rule can silently misread a concurrent writer as an empty file.
|
Thanks for writing this — the answer to your framing question is "intentional design", and the convention deserved to be written down. I've pushed a commit on top rather than requesting changes, because most of what was missing is in the kernel repo and you have no way to see it. Leaving the five questions open rather than guessing at them was the right call; here are the answers. The one that mattered. The simulator makes that trap worse, so I sharpened that section too: it concatenates its prefix with your path and does no whitelist check at all, so an invented shared path works perfectly in the simulator and fails on the watch, silently. Your "the files are just not where you'd first look" point stands, it's just the less dangerous of the two differences. Your five questions, answered against the kernel and then confirmed on a watch over USB MSD:
Two corrections, both things the SDK source doesn't actually support:
I also added a caveat to "every read is optional", because your two rules interact badly: Everything you'd already written I left alone — the citations all check out line-for-line, which made verifying the rest much quicker than it would otherwise have been. Have a read of the commit and push back on anything that doesn't match what you found; you've been closer to this from the app side than I have. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Docs/shared-data.md`:
- Around line 151-158: Update the shared-data cleanup documentation to say there
is no automatic garbage collection, while acknowledging that apps can delete
files through user-initiated actions. Clarify that users can also reclaim space
by removing files through USB mass-storage access, but there is no automatic or
in-watch shared-file cleanup or ownership mechanism.
- Around line 25-30: Update the shared-path behavior description near the
reserved ../SharedData rule to state that opening ../SomethingElse/file is
rejected, with open() failing or returning false, rather than saying the path
never resolves. Preserve the existing whitelist and rejection rules for all
other .. segments.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 072fffc3-5603-4d20-b68c-0f6c3200a096
📒 Files selected for processing (1)
Docs/shared-data.md
Two fixes to the previous commit, both from CodeRabbit. The rejected-path sentence was wrong. It said "../SomethingElse/file" does not fail when you open it, which is not what happens: the guard clears the path at construction, so fs.file() hands back a non-null object and open() returns false. Corrected, and extended with the part that actually matters -- the failure is indistinguishable from a missing file, and a null check on file() will not catch a bad path. The lifetime section claimed the user has no way to reclaim the space, which contradicts this page's own statement that the directory is visible and writable over USB mass storage. Reworded: no *automatic* cleanup, deletion only on an explicit request (an app's user-initiated clear, or a factory reset), and USB as a real but off-watch escape hatch rather than a substitute for an in-app one.
3 participants
I'm trying to figure out how best to implement some maps, fundamentally the maps themselves should live in a shared space so they can be referenced per app. It's not currently documented, so this PR stands as a, "is this intentional design or coincidence?"
StrideLut and OutdoorStrideCalibrator both default to
../SharedData/stride.json, and Running and Treadmill are both consumers of it, but the directory appears nowhere in Docs/ so a third app has no way to learn the convention exists, or the two rules that make it work: mkdir before opening (f_open does not create parents) and treat every read as optional.This documents what the source demonstrably does, cited line by line, plus the simulator's unclamped ".." putting the shared files beside Output/ rather than inside it.
Summary by CodeRabbit