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

add support for 32bit/m depth encoding #6

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

Luca-Pozzi
Copy link

This PR implements the changes described in #5 to add support for depth data encoded as 32bit floats.

The code checks for the encoding on the depth topic (if use_depth is true) by waiting 10s for a depth message. If the 10s timeout is reached, the encoding defaults to the original 16UC1.
I am not a huge fan of rospy.wait_for_message but I find more reasonable to add a little overhead than re-reading the (same) encoding from each and every Image msg.

@LorenzoFerriniCodes
Copy link

Hi @Luca-Pozzi, thanks for your contribution!

In general, the idea of checking the message format and set a parameter to represent it is perfectly fine. However, why waiting a message for (maximum) 10 seconds when we will receive it and read it anyway later in the callback? I think the best option is to check in the callback whether the parameter has been set or not and setting it in the latter case.

@Luca-Pozzi
Copy link
Author

Thank you for the feedback @LorenzoFerriniCodes!

The answer to your question, is that I have not thought about it 😅 Definitely a smarter implementation than mine.

I hope to implement the changes later this afternoon

@Luca-Pozzi
Copy link
Author

Done, the check for depth encoding is now moved to the callback(s).

Thank you for the advice!

@severin-lemaignan severin-lemaignan merged commit 7ff3f46 into ros4hri:master Feb 5, 2024
@Luca-Pozzi Luca-Pozzi deleted the depth_32bits branch February 5, 2024 15:43
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.

3 participants