Skip to content

Fix build system for windows targets#38

Open
RobbieG15 wants to merge 2 commits intoNVIDIA-ISAAC-ROS:mainfrom
RobbieG15:rgreenslade-windows-fix
Open

Fix build system for windows targets#38
RobbieG15 wants to merge 2 commits intoNVIDIA-ISAAC-ROS:mainfrom
RobbieG15:rgreenslade-windows-fix

Conversation

@RobbieG15
Copy link
Copy Markdown

This will fix windows build systems.

There is a stricter usage of Chrono with windows and MSVC.

Thank you for all the work on this project.

Let me know if there is anything else I can do if this does not fix it for you all.

I verified that this works on my windows machine.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 6, 2026

Greptile Summary

Adds an explicit std::chrono::duration_cast<std::chrono::system_clock::duration> when constructing the time_point in GetTimestampFromSerializedMessage. On MSVC/Windows, system_clock::duration uses 100-nanosecond ticks rather than nanoseconds, so the implicit conversion is a narrowing operation that MSVC rejects at compile time. The fix is correct and idiomatic with no behavior change on Linux.

Confidence Score: 5/5

Safe to merge — single-line correctness fix with no behavior change on Linux and correct behavior on Windows.

The change is minimal, well-targeted, and uses the standard idiomatic solution for cross-platform chrono portability. No logic or data-integrity concerns.

No files require special attention.

Important Files Changed

Filename Overview
greenwave_monitor/src/greenwave_monitor.cpp Adds duration_cast to fix implicit narrowing conversion error on MSVC/Windows where system_clock::duration is 100ns ticks rather than nanoseconds

Sequence Diagram

sequenceDiagram
    participant Caller
    participant GetTimestamp
    participant chrono
    Caller->>GetTimestamp: GetTimestampFromSerializedMessage(msg)
    GetTimestamp->>GetTimestamp: Extract sec/nsec from buffer[4..11]
    GetTimestamp->>chrono: seconds(sec) + nanoseconds(nsec)
    chrono-->>GetTimestamp: combined nanoseconds duration
    GetTimestamp->>chrono: duration_cast<system_clock::duration>(combined)
    Note over chrono: Linux: no-op (already nanoseconds)<br/>Windows: truncates to 100ns ticks
    chrono-->>GetTimestamp: platform-native duration
    GetTimestamp-->>Caller: time_point<system_clock>
Loading

Reviews (3): Last reviewed commit: "Fix the linting errors" | Re-trigger Greptile

@sgillen
Copy link
Copy Markdown
Collaborator

sgillen commented Apr 7, 2026

Hi @RobbieG15, thanks! Happy to merge this. Looks like some linting errors on older distros and I don't think the commit was signed. Both pre commit hooks for linting and the sign off instructions can be found here: https://github.com/NVIDIA-ISAAC-ROS/greenwave_monitor/blob/main/Contributing.md

@RobbieG15 RobbieG15 force-pushed the rgreenslade-windows-fix branch from ce14bbb to d66a4b7 Compare April 7, 2026 16:34
Robert Greenslade added 2 commits April 7, 2026 12:35
Signed-off-by: Robert Greenslade <robert.greenslade@resonantsciences.com>
Signed-off-by: Robert Greenslade <robbieg1515@gmail.com>
Signed-off-by: Robert Greenslade <robert.greenslade@resonantsciences.com>
Signed-off-by: Robert Greenslade <robbieg1515@gmail.com>
@RobbieG15 RobbieG15 force-pushed the rgreenslade-windows-fix branch from d66a4b7 to c5fe2f3 Compare April 7, 2026 16:36
@RobbieG15
Copy link
Copy Markdown
Author

@sgillen,

Those linting/format errors should be resolved and all tests are passing. I also signed the commits. Let me know if there is anything else I should do.

Thanks!

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