Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create LcmLogSink, a VectorLogSink analog implemented as an lcm interface #358

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Brian-Acosta
Copy link
Contributor

@Brian-Acosta Brian-Acosta commented Apr 2, 2024

Usage example:

auto lcm_log_sink = LcmLogSink()
auto publishes_to_log = LcmPublisherSystem<lcmt_my_message>::Make(&lcm_log_sink, ...);

// build the diagram and set up the simulation

simulator.AdvanceTo(end_time);

lcm_log_sink.WriteLog("lcmlog-00");

This is similar to Drake's DrakeLcmLog, but avoids writing to disk for every Publish event, instead deferring until the user calls WriteLog


This change is Reviewable

Copy link
Contributor

@yangwill yangwill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @Brian-Acosta)


lcm/lcm_log_sink.cc line 88 at r1 (raw file):

  unsigned long timestamp;
  if (!overwrite_publish_time_with_system_clock_) {
    timestamp = second_to_timestamp(time_sec.value_or(0.0));

is there a better way to deal with the optional?


lcm/lcm_log_sink.cc line 91 at r1 (raw file):

  } else {
    timestamp = std::chrono::steady_clock::now().time_since_epoch() /
        std::chrono::microseconds(1);

it feels like there should be a command that's more direct than dividing by microseconds

Copy link
Contributor

@yangwill yangwill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, just want to confirm that the implementation of the timestamps is as intended

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @Brian-Acosta)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants