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

Added timeout parameter #26

Merged
merged 5 commits into from
Jun 8, 2023
Merged

Added timeout parameter #26

merged 5 commits into from
Jun 8, 2023

Conversation

gsokoll
Copy link
Contributor

@gsokoll gsokoll commented May 21, 2023

Per ROS2 test reports (#15), the node consistently failed with a " Sensor error: Timed out waiting for data, please check device connection and baudrate!" when the sensor polling interval matched the IMU output rate. Investigating the code, this was occuring because the read data function was not correctly checking for the absence of new data. Changes to the underlying witmotion IMU QT library have been made to address this issue (refer ElettraSciComp/witmotion_IMU_QT#7). This PR adds a timeout parameter to complement the changes made as part of the IMU QT PR.

The ROS2 launch mods (#25) have been included in this PR as I neede them during testing of the ROS2 code.

@twdragon twdragon changed the title Added timeout parameter (per https://github.com/ElettraSciComp/witmot… Added timeout parameter May 29, 2023
@twdragon
Copy link
Collaborator

@gsokoll the pull request will be ready for merging after we will agree on my comment on the underlying PR. After the merge, would you create also the PR in ROS1 branch? The contribution itself seems great, so it is important to have it both in ROS1 and ROS2 branches, I see. The code should be very similar, practically the same

@gsokoll
Copy link
Contributor Author

gsokoll commented May 29, 2023

Sure. I have zero experience with ROS1, but I am happy to try.

@gsokoll
Copy link
Contributor Author

gsokoll commented May 30, 2023

I have prepared a ROS1 version in a local clone. Once the underlying PR in IMU_QT is merged in, I will submit the PR for this repo.

@twdragon
Copy link
Collaborator

twdragon commented Jun 6, 2023

@gsokoll the PR in the underlying library is merged, so it is possible to proceed.

@gsokoll
Copy link
Contributor Author

gsokoll commented Jun 7, 2023

Have cleaned up a few things I broke, and also submitted a matching PR for the ROS1 main branch. Both this and the ROS1 PR should be ok for a merge.

@twdragon
Copy link
Collaborator

twdragon commented Jun 7, 2023

@gsokoll can you please update also the underlying library remote submodule commit via git? Now the repository is set to 21e72c0 which is from March 2021, so the version of the library should match the current state of the leading repository. After that I do not see the obstacles to merge both PRs

@gsokoll
Copy link
Contributor Author

gsokoll commented Jun 7, 2023

Ok, try now.

@twdragon twdragon merged commit 43dd343 into ElettraSciComp:ros2 Jun 8, 2023
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