Skip to content

RSDK-6574: Fix use of time_point #335

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 4, 2024
Merged

RSDK-6574: Fix use of time_point #335

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f50a3f0
fix chrono type
lia-viam Dec 3, 2024
997c6c6
use duration_cast
lia-viam Dec 3, 2024
4d6a30a
fix comment
lia-viam Dec 4, 2024
116e644
Merge branch 'main' into chrono-fix
lia-viam Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions src/viam/sdk/common/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
namespace viam {
namespace sdk {

using time_point = std::chrono::time_point<long long, std::chrono::nanoseconds>;

std::vector<unsigned char> string_to_bytes(const std::string& s) {
std::vector<unsigned char> bytes(s.begin(), s.end());
return bytes;
Expand All @@ -34,21 +32,23 @@ std::string bytes_to_string(const std::vector<unsigned char>& b) {
return img_string;
};

time_point timestamp_to_time_pt(const google::protobuf::Timestamp& timestamp) {
const std::chrono::seconds seconds(timestamp.seconds());
const std::chrono::nanoseconds nanos(timestamp.nanos());
return time_point(std::chrono::duration_cast<std::chrono::system_clock::duration>(seconds) +
nanos);
time_pt timestamp_to_time_pt(const google::protobuf::Timestamp& timestamp) {
Copy link
Member

@acmorrow acmorrow Dec 3, 2024

Choose a reason for hiding this comment

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

Would this later be unified with #332 ?

Copy link
Collaborator Author

@lia-viam lia-viam Dec 4, 2024

Choose a reason for hiding this comment

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

yes I look forward to doing so haha

return time_pt{std::chrono::seconds{timestamp.seconds()} +
std::chrono::nanoseconds{timestamp.nanos()}};
}

google::protobuf::Timestamp time_pt_to_timestamp(const time_point& time_pt) {
const std::chrono::seconds duration_s =
std::chrono::duration_cast<std::chrono::seconds>(time_pt.time_since_epoch());
const std::chrono::nanoseconds duration_ns = time_pt.time_since_epoch() - duration_s;
google::protobuf::Timestamp timestamp;
timestamp.set_seconds(duration_s.count());
timestamp.set_nanos(static_cast<int32_t>(duration_ns.count()));
return timestamp;
google::protobuf::Timestamp time_pt_to_timestamp(time_pt tp) {
const std::chrono::nanoseconds since_epoch = tp.time_since_epoch();

const auto sec_floor = std::chrono::duration_cast<std::chrono::seconds>(since_epoch);
const std::chrono::nanoseconds nano_part = since_epoch - sec_floor;

google::protobuf::Timestamp result;

result.set_seconds(sec_floor.count());
result.set_nanos(static_cast<int32_t>(nano_part.count()));

return result;
}

response_metadata response_metadata::from_proto(const viam::common::v1::ResponseMetadata& proto) {
Expand Down Expand Up @@ -80,7 +80,7 @@ std::chrono::microseconds from_proto(const google::protobuf::Duration& proto) {
return from_seconds + from_nanos;
}

google::protobuf::Duration to_proto(const std::chrono::microseconds& duration) {
google::protobuf::Duration to_proto(std::chrono::microseconds duration) {
namespace sc = std::chrono;

const sc::seconds seconds = sc::duration_cast<sc::seconds>(duration);
Expand Down
18 changes: 8 additions & 10 deletions src/viam/sdk/common/utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,30 +18,28 @@ const std::string kService = "service";
const std::string kRDK = "rdk";
const std::string kBuiltin = "builtin";

using time_pt = std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>;
Copy link
Member

@acmorrow acmorrow Dec 3, 2024

Choose a reason for hiding this comment

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

What motivated the switch from calling it time_point to time_pt?

Copy link
Collaborator Author

@lia-viam lia-viam Dec 4, 2024

Choose a reason for hiding this comment

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

I find people do 'using namespace std::chrono' pretty often so I think this would be a name clash with our time_point and std::chrono::time_point<Clock, Duration>


struct response_metadata {
std::chrono::time_point<long long, std::chrono::nanoseconds> captured_at;
time_pt captured_at;

static response_metadata from_proto(const viam::common::v1::ResponseMetadata& proto);
static viam::common::v1::ResponseMetadata to_proto(const response_metadata& metadata);
};

bool operator==(const response_metadata& lhs, const response_metadata& rhs);

/// @brief convert a google::protobuf::Timestamp to
/// std::chrono::time_point<long long, std::chrono::nanoseconds>
std::chrono::time_point<long long, std::chrono::nanoseconds> timestamp_to_time_pt(
const google::protobuf::Timestamp& timestamp);
/// @brief convert a google::protobuf::Timestamp to time_point
Copy link
Member

@stuqdog stuqdog Dec 3, 2024

Choose a reason for hiding this comment

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

(nit) s/time_point/time_pt

time_pt timestamp_to_time_pt(const google::protobuf::Timestamp& timestamp);

/// @brief convert a std::chrono::time_point<long long, std::chrono::nanoseconds> to
/// a google::protobuf::Timestamp.
google::protobuf::Timestamp time_pt_to_timestamp(
const std::chrono::time_point<long long, std::chrono::nanoseconds>& time_pt);
/// @brief convert a time_point to a google::protobuf::Timestamp.
Copy link
Member

@stuqdog stuqdog Dec 3, 2024

Choose a reason for hiding this comment

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

(nit) s/time_point/time_pt

google::protobuf::Timestamp time_pt_to_timestamp(time_pt);

std::vector<unsigned char> string_to_bytes(std::string const& s);
std::string bytes_to_string(std::vector<unsigned char> const& b);

std::chrono::microseconds from_proto(const google::protobuf::Duration& proto);
google::protobuf::Duration to_proto(const std::chrono::microseconds& duration);
google::protobuf::Duration to_proto(std::chrono::microseconds duration);

// the authority on a grpc::ClientContext is sometimes set to an invalid uri on mac, causing
// `rust-utils` to fail to process gRPC requests. This class provides a convenience wrapper around a
Expand Down
7 changes: 2 additions & 5 deletions src/viam/sdk/robot/client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,7 @@ class RobotClient {
struct status {
boost::optional<Name> name;
ProtoStruct status_map;
// TODO: RSDK-6574: revisit time_point
boost::optional<std::chrono::time_point<long long, std::chrono::nanoseconds>>
last_reconfigured;
boost::optional<time_pt> last_reconfigured;
friend bool operator==(const status& lhs, const status& rhs);
};

Expand All @@ -68,8 +66,7 @@ class RobotClient {
std::string method;
boost::optional<std::string> session_id;
ProtoStruct arguments;
// TODO: RSDK-6574: revisit time_point
boost::optional<std::chrono::time_point<long long, std::chrono::nanoseconds>> started;
boost::optional<time_pt> started;
friend bool operator==(const operation& lhs, const operation& rhs);
};

Expand Down
2 changes: 1 addition & 1 deletion src/viam/sdk/services/motion.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class Motion : public Service {
plan_state state;

/// @brief The time the executing plan transitioned to the state.
std::chrono::time_point<long long, std::chrono::nanoseconds> timestamp;
time_pt timestamp;

/// @brief The reason for the state change. The error message if the plan failed, or the
/// re-plan reason if re-planning was necessary.
Expand Down
4 changes: 2 additions & 2 deletions src/viam/sdk/tests/mocks/camera_mocks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ Camera::image_collection fake_raw_images() {
std::chrono::seconds seconds(12345);
std::chrono::nanoseconds nanos(0);
collection.images = images;
collection.metadata.captured_at = std::chrono::time_point<long long, std::chrono::nanoseconds>(
std::chrono::duration_cast<std::chrono::system_clock::duration>(seconds) + nanos);
collection.metadata.captured_at =
time_pt{std::chrono::duration_cast<std::chrono::system_clock::duration>(seconds) + nanos};
return collection;
}

Expand Down
5 changes: 2 additions & 3 deletions src/viam/sdk/tests/mocks/mock_motion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,8 @@ pose_in_frame fake_pose() {
}

Motion::plan_status MockMotion::fake_plan_status() {
return {Motion::plan_state::k_succeeded,
std::chrono::time_point<long long, std::chrono::nanoseconds>::max(),
boost::optional<std::string>("reason")};
return {
Motion::plan_state::k_succeeded, time_pt::max(), boost::optional<std::string>("reason")};
}

Motion::plan_status_with_id MockMotion::fake_plan_status_with_id() {
Expand Down
Loading