Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Dudecake
Copy link

@Dudecake Dudecake commented Feb 3, 2021

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 3

Replaced the c-arrays of std::atomic<float> with std::array<float> guarded by locks

With how the quaternions were copied, race conditions were still possible. Especially in Relativty::HMDDriver::calibrate_quaternion.
Relativty::HMDDriver::qconj was only written to in Relativty::HMDDriver::calibrate_quaternion, so locking that is a waste.

Replaced assignments to quaternion arrays with std::copys and a std::transform

The compiler can optionally optimise std::copys to memcpys or vector operations.

Replaced calls to Relativty::HmdQuaternion_Init with braced initialization

Rename Relativty::ServerDriver::HMDDriver to m_HMDDriver

The distinction between the class and the instance couldn't be made otherwise. No idea how MSVC handled that...

Replaced NULLs with either nullptrs or 0s

Some NULLs imply the arguments are pointers when they are ints

Various formatting changes and renamed Relativty::HMDDriver::new_vector_avaiable to Relativty::HMDDriver::new_vector_available

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 {
Copy link
Collaborator

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

Copy link
Author

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.

Copy link
Collaborator

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];

Copy link
Author

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.

Copy link
Collaborator

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

@okawo80085
Copy link
Collaborator

okawo80085 commented Feb 3, 2021

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

@okawo80085
Copy link
Collaborator

just dropping by, did anyone try this with actual hardware?

@Dudecake
Copy link
Author

Dudecake commented Mar 3, 2021

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.

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.

None yet

2 participants