-
Notifications
You must be signed in to change notification settings - Fork 14
✨ QDMI Environment Implementation #154
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: develop
Are you sure you want to change the base?
Changes from 46 commits
0887df6
302c416
3aab49e
52bd8d7
49cc0bf
3ba8a92
6f658f6
25abdd4
98971ba
98a8659
c2b2935
f88b60c
c1591fe
4e7b8f8
bd849ae
3858563
3929d0a
16afe77
9bbc775
7b3e8df
ba5a676
e67355f
67312db
94e4b25
20a2841
e239964
7474cbe
83a6b18
c71b2c3
d0db550
0e9bcb2
d2218d5
889b27c
a83cbf4
709ac33
4572c14
123d2e6
f562d03
64ea415
6df7dee
e3e6694
04a8218
0bda557
56ba00d
aff6f1e
10710d0
e305c46
9f38ad9
248a112
aba1df1
80cd577
ab49391
3234155
2f2e03e
4ef85b5
0202474
34718bc
5a121c5
f8eb819
d2f3321
8c747b3
3a3576d
35dc853
c7ffdc3
e3467e6
b4c416d
d61e4e7
abe1b30
891abf3
abbbbd5
c7432aa
ecc8fcd
b88ff2f
72cf9eb
ddd73a1
80628dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,10 +26,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||||||||
|
|
||||||||||
| #include <algorithm> | ||||||||||
| #include <array> | ||||||||||
| #include <chrono> | ||||||||||
| #include <cmath> | ||||||||||
| #include <complex> | ||||||||||
| #include <cstddef> | ||||||||||
| #include <cstdint> | ||||||||||
| #include <cstdlib> | ||||||||||
| #include <cstring> | ||||||||||
| #include <ctime> | ||||||||||
| #include <functional> | ||||||||||
| #include <iterator> | ||||||||||
| #include <limits> | ||||||||||
|
|
@@ -77,6 +81,24 @@ struct CXX_QDMI_Operation_impl_d { | |||||||||
| std::string name; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| struct CXX_QDMI_TelemetrySensor_impl_d { | ||||||||||
| std::string id; | ||||||||||
| std::string unit; | ||||||||||
| std::chrono::seconds sampling_rate; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| struct CXX_QDMI_Device_TelemetrySensor_Query_impl_d { | ||||||||||
| std::chrono::time_point<std::chrono::system_clock> start_time; | ||||||||||
| std::chrono::time_point<std::chrono::system_clock> end_time; | ||||||||||
| size_t timeout{}; | ||||||||||
|
||||||||||
| size_t timeout{}; | |
| [size_t](std::chrono::seconds) timeout{}; |
?
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.
| size_t timeout{}; | |
| size_t timeout; |
and size_t (same as std::chrono::seconds) does not need a {} initialiser.
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 believe it cannot. However, we can std::chrono::seconds instead of size_t, leading to additional casting in the CXX_QDMI_device_telemetrysensor_query_wait. If you suggest that it will improve the readability of the code, we can change it.
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.
✅ Done
Outdated
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.
Is there a specific reason for float? For most others floating point values, we use double. I do not have a strong opinion here, just want to raise it.
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.
Let's make it consistent ✅
Outdated
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.
| size_t result_length{}; | |
| QDMI_TelemetrySensor_Query_Status status{}; | |
| size_t result_length; | |
| QDMI_TelemetrySensor_Query_Status status; |
If I am not mistaken, if you want to initialize those members, you need to assign values, i.e., 0 for the length and some enum value for the status. However, I think it should be just fine without {}.
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.
Even though they need {}, clang-tidy raises a cppcoreguidelines-pro-type-member-init warning.
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.
✅ Done
kayaercument marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
ystade marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
Is the attribute result_length required, or would result_values.size() suffice in the end?
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.
Removed from CXX_QDMI_Device_TelemetrySensor_Query_impl_d as well. ✅
Outdated
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 think for the random number generation of float in a given interval, there is a more elegant way:
// Create a random device and seed the generator
std::random_device rd;
std::mt19937 gen(rd());
// Define the range: e.g., between 0.0 and 1.0
std::uniform_real_distribution<> dis(0.0, 1.0);
// all variables above could be implemented centrally as part of the `device_session`.
// Generate a random float
double random_value = dis(gen);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.
Done ✅
Outdated
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.
Very minor: Maybe it is obvious but I cannot decipher what req stands for. Is there a more descriptive name?
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.
✅ Renamed as required_size
Uh oh!
There was an error while loading. Please reload this page.