-
Notifications
You must be signed in to change notification settings - Fork 350
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
Formatting and arrays #74
base: master
Are you sure you want to change the base?
Conversation
std::array<uint8_t, 64> packet_buffer; | ||
std::array<int16_t, 4> quaternion_packet; | ||
//this struct is for mpu9250 support | ||
#pragma pack(push, 1) | ||
struct pak { |
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.
this struct corresponds to the packet send by hardware, it needs to have this structure
struct pak {
uint8_t id;
float quat[4];
uint8_t rest[47];
};
changing float[4]
to std::array<float, 4>
may be fine(can't tell until i test it), but putting it at the end of this struct is definitely not
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.
Thank you for the review. I've reverted the float[4]
. Concerning the removal rest
: if the struct was used in the hid_read
call, it would indeed not be fine. But on line 167 it is only used as an overlay on the contents of packet_buffer
, so the trailing data won't matter.
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.
fine by me, except the fact that float[4] quat;
won't compile, it needs to be float quit[4];
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.
Oops. I was on my work laptop, so I used the web editor and clicked save a little too fast.
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.
happens to the best of us
apart from that struct change, everything else looks ok at first glance, didn't check it with actual hardware yet i recommend you compile it, and at least test is with real hardware for while before merging this |
just dropping by, did anyone try this with actual hardware? |
I haven't had the time to source the components for a unit for me, so I wasn't able to test it. I figured the changes weren't radical enough and the reviewer would still need to test it themselves over trusting me on my (proverbial) blue eyes. |
Notable fixes
Fixed array out-of-range in
Normalize
In
Normalize
an attempt is made te read index 3 of a c-array of length 3Replaced the c-arrays of
std::atomic<float>
withstd::array<float>
guarded by locksWith how the quaternions were copied, race conditions were still possible. Especially in
Relativty::HMDDriver::calibrate_quaternion
.Relativty::HMDDriver::qconj
was only written to inRelativty::HMDDriver::calibrate_quaternion
, so locking that is a waste.Replaced assignments to quaternion arrays with
std::copy
s and astd::transform
The compiler can optionally optimise
std::copy
s tomemcpy
s or vector operations.Replaced calls to
Relativty::HmdQuaternion_Init
with braced initializationRename
Relativty::ServerDriver::HMDDriver
tom_HMDDriver
The distinction between the class and the instance couldn't be made otherwise. No idea how MSVC handled that...
Replaced
NULL
s with eithernullptr
s or0
sSome
NULL
s imply the arguments are pointers when they areint
sVarious formatting changes and renamed
Relativty::HMDDriver::new_vector_avaiable
toRelativty::HMDDriver::new_vector_available