Conversation
|
Claude finished @ericmlujan's task in 1m 29s —— View job SummaryThis PR adds a new Rust example demonstrating the The PR has gone through multiple rounds of review with substantive feedback from cursor, gasmith, and dave-foxglove, with most issues addressed in subsequent commits. BlockersNone Suggestions1. Unanswered review question about playback speed in
|
Co-authored-by: Eric Lujan <ericmlujan@users.noreply.github.com>
- Add check in play() to prevent transitioning from Ended to Playing - Reset status to Paused when seeking from Ended state - Reorder listener to handle seek before play/pause for looping support Co-authored-by: Eric Lujan <ericmlujan@users.noreply.github.com>
Move current_time assignment after fallible reader creation to avoid inconsistent state if IndexedReader::new_with_options fails Co-authored-by: Eric Lujan <ericmlujan@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Prevent pause() from transitioning out of Ended state, which would bypass the guard in play() and create a confusing state cycle Co-authored-by: Eric Lujan <ericmlujan@users.noreply.github.com>
| self.status | ||
| } | ||
|
|
||
| fn current_time(&self) -> u64 { |
There was a problem hiding this comment.
For all references to a u64 as the player time, can we define and use a named type instead of a raw u64.
Maybe a "newtype" like McapTimestamp and McapDuration (similar to rust standard timestamps and durations).
P.S. I'm also up for a "type alias" like type McapNanos = u64 but it doesn't offer the whoops-protection a newtype does.
| self.status | ||
| } | ||
|
|
||
| fn current_time(&self) -> u64 { |
There was a problem hiding this comment.
For all references to a u64 as the player time, can we define and use a named type instead of a raw u64.
Maybe a "newtype" like McapTimestamp and McapDuration (similar to rust standard timestamps and durations).
P.S. I'm also up for a "type alias" like type McapNanos = u64 but it doesn't offer the whoops-protection a newtype does.
There was a problem hiding this comment.
Addressed in 25abf13. I decided against introducing a full tuple struct for nanoseconds and duration; I found it added a bit too much boilerplate in the context of a code example.
| /// Used to send a PlaybackState to Foxglove | ||
| fn playback_speed(&self) -> f32; | ||
|
|
||
| /// Logs the next message for playback if it's ready, or returns a duration to wait. |
There was a problem hiding this comment.
Sorry if I'm missing something, but could/should this mention something about playback speed? E.e. if the playback speed is 10x should I sleep for 1/10 the returned Duration (or is it already accounting for that?)
There was a problem hiding this comment.
Addressed in 22218a6. It is already accounting for that
| use foxglove::{websocket::PlaybackStatus, WebSocketServerHandle}; | ||
|
|
||
| /// A data source that supports ranged playback with play/pause, seek, and variable speed. | ||
| /// |
There was a problem hiding this comment.
It looks like log_next_message is expected to be called in a loop(?). If true, let's mention that in the overall doc comment.
| /// | ||
| /// The caller should sleep outside of any lock to allow control requests to be processed. | ||
| /// This method also broadcasts time updates via `server.broadcast_time()`. | ||
| fn log_next_message(&mut self, server: &WebSocketServerHandle) -> Result<Option<Duration>>; |
There was a problem hiding this comment.
I'm likely missing some context in the wider code base, but in just this interface/api - I'm not clear on why this functions has a "log_*" prefix on it. If it's just pushing a message to a log file, then it's perfectly named, but I have an impression that it's doing something more or different(?)
There was a problem hiding this comment.
Good question. "Logging" is the general term for sending your robotics data over to Foxglove. We have the notion of a "channel", which you can think of as a topic that data messages can be logged to/sent over, and a channel can be backed by one or more sinks, which handle actually doing something with that data (i.e. saving to an MCAP file or sending over a WebSocket). In this case, this function calls a Channel::log() on a bunch of channels that are set up to publish the data over a WebSocket; hence the name.
| pending_message: Option<(mcap::records::MessageHeader, Vec<u8>)>, | ||
| } | ||
|
|
||
| impl McapPlayer { |
There was a problem hiding this comment.
Can we describe what an McapPlayer is and what it is used for (asking for doc comments), either here, or on the struct (line 19) or at the top of the file (i.e. //! comments).
Changelog
RangedPlaybackcapabilityDocs
Description
Add a Rust example using the RangedPlayback websocket capability to load and play back an mcap file. This example introduces a documented
PlaybackSourcetrait, which others can pattern match to when implementing their own loaders for custom data formats.This PR supersedes and simplifies #772.
A Python version of this example exists at #862.
A C++ version of this example exists at #861.
Fixes: FLE-131