-
Notifications
You must be signed in to change notification settings - Fork 11
Deadline monitoring CPP API #48
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
base: main
Are you sure you want to change the base?
Deadline monitoring CPP API #48
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a C++ FFI (Foreign Function Interface) API for deadline monitoring functionality in the health monitoring library. The changes enable C++ code to interact with the Rust-based deadline monitoring system through a well-defined interface.
Changes:
- Added FFI bindings in Rust to expose deadline monitoring functionality to C++
- Implemented C++ wrapper classes (
HealthMonitor,DeadlineMonitor,Deadline) that interface with the Rust backend - Created build configuration for compiling both Rust static library and C++ library components
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/health_monitoring_lib/src/lib.rs | Added HealthMonitor and HealthMonitorBuilder structs to manage deadline monitors |
| src/health_monitoring_lib/src/ffi.rs | New FFI functions for health monitor builder creation, destruction, and deadline monitor management |
| src/health_monitoring_lib/src/deadline/ffi.rs | New FFI functions for deadline monitor operations including builder, monitor, and deadline lifecycle management |
| src/health_monitoring_lib/src/deadline/deadline_monitor.rs | Refactored deadline monitoring to support FFI usage with internal helper methods |
| src/health_monitoring_lib/src/common.rs | Changed IdentTag to FFI-compatible representation and added FFI helper utilities |
| src/health_monitoring_lib/cpp/include/score/hm/health_monitor.h | C++ header defining HealthMonitor and HealthMonitorBuilder classes |
| src/health_monitoring_lib/cpp/include/score/hm/deadline/deadline_monitor.h | C++ header defining deadline monitoring classes (DeadlineMonitor, Deadline, DeadlineHandle) |
| src/health_monitoring_lib/cpp/include/score/hm/common.h | C++ header with common types including IdentTag, TimeRange, and FFI helpers |
| src/health_monitoring_lib/cpp/health_monitor.cpp | Implementation of C++ HealthMonitor classes |
| src/health_monitoring_lib/cpp/deadline/deadline_monitor.cpp | Implementation of C++ deadline monitoring classes |
| src/health_monitoring_lib/cpp/common.cpp | Implementation of DroppableFFIHandle utility class |
| src/health_monitoring_lib/cpp/ffi_helpers.h | Helper function to convert Rust error codes to C++ Error enum |
| src/health_monitoring_lib/cpp/tests/health_monitor_test.cpp | Basic test demonstrating the C++ API usage |
| src/health_monitoring_lib/BUILD | Updated build configuration to include Rust static library and C++ library targets |
Comments suppressed due to low confidence (1)
src/health_monitoring_lib/src/deadline/ffi.rs:1
- Corrected grammar from 'This function is provides' to 'This function is provided'.
use crate::common::ffi::*;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/health_monitoring_lib/cpp/include/score/hm/health_monitor.h
Outdated
Show resolved
Hide resolved
|
The created documentation from the pull request is available at: docu-html |
c1dd35a to
28fbfd4
Compare
28fbfd4 to
b9c92b8
Compare
|
I will fix QNX8 after first review rounds |
arkjedrz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicks, but error handling can definitely be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing empty line.
|
|
||
| ## IDE support | ||
|
|
||
| ### C++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend markdownlint for Markdown linting.
- Missing newline after section.
- Prefer single sentence per line.
|
|
||
| bazel_dep(name = "score_baselibs_rust", version = "0.0.3") | ||
| bazel_dep(name = "score_baselibs", version = "0.2.2") | ||
| # git_override( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary?
| use_repo(toolchains_qnx, "toolchains_qnx_ifs") | ||
|
|
||
| bazel_dep(name = "score_baselibs_rust", version = "0.0.3") | ||
| bazel_dep(name = "score_baselibs", version = "0.2.2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why moved here?
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| ********************************************************************************/ | ||
| #ifndef SCORE_HM_DEADLINE_DEADLINE_MONITOR_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not #pragma once?
|
|
||
| auto deadline_mon = std::move(*deadline_monitor_res); | ||
|
|
||
| // std::cout << "Getting deadline" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover.
| unsafe { self.start_internal().map(|_| DeadlineHandle(self)) } | ||
| } | ||
|
|
||
| /// Starts the deadline - it will be monitored by health monitoring system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Starts the deadline - it will be monitored by health monitoring system.
/// This function is for FFI usage only!
///
/// # Safety
///
/// Caller must ensure that deadline is not used until it's stopped.
/// After this call You shall assure there's only a single owner of the `Deadline` instance and it does not call start before stopping.
| public: | ||
| HealthMonitor(const HealthMonitor&) = delete; | ||
| HealthMonitor& operator=(const HealthMonitor&) = delete; | ||
| HealthMonitor(HealthMonitor&& other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move impl to cpp.
| } | ||
| } | ||
|
|
||
| mod ffi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge mod and use blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this file to parent and remove deadline/ directory. .cpp files don't necessarily have to follow Rust module structure, and it's already messy with include/ dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I'm not against committing editor config files into repo, but this makes sense if we all use the same editor.
Maybe this file should be on the git ignore list?
Closes #14