Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Stretch (hello-robot) #409
Add support for Stretch (hello-robot) #409
Changes from 31 commits
0e4b9e2
7992d2e
bea022f
90e7895
bf5b6e1
e5a4fa3
2e36039
9a6150c
38f6be8
f2269c1
8b28a3d
e9682af
4219527
aaf7fd4
2bf4dcb
f5651a5
e755d10
a98556b
6bc1c6b
bd4fdc8
12a2f2c
16dd73f
88526d0
9e2d98c
1053845
652eb01
3fbd680
42d9556
db09ebf
ddd8059
d1fc630
7c87025
e0fa5e4
d5fbfd4
78e8435
37572eb
eb48452
722092a
56d3845
306a1ac
c24b137
2c0feed
d28f556
d210d2e
d716511
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should there be a
huggingface-cli login
step?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.
Yes you're correct. Technically, it's already pointed out in the 7_get_started_with_real_robot.md as some other details and I didn't want to add to avoid too much redundancy. I've added a word on that (7c87025)
Eventually we might rearrange these markdown as the number of robots grow (cc @Cadene)
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 am not fan of modifying this function, to stay consistent with the one in opencv.py
Instead, could we have another function that given camera_indices / serial number retrieves the camera 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.
The issue is that we would still need to iterate through
rs.context().query_devices()
in that function and comparedevice.get_info(rs.camera_info(SERIAL_NUMBER_INDEX))
against the given serial_number before accessing the name. We can do it but: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.
What can we do to unify opencv and intelrealsense on this function?
I would imagine
find_cameras_info(raise_when_empty=True) -> list[dict]:
for both, withcamera_infos[0]["index"]
being the camera index port for opencv or intelrealsense serial, andcamera_infos[0]["name"]
not existing for opencv.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.
Nit:
camera_infos
instead ofcameras_info
for consistency withcamera_ids
and how we usually add as
at the end for our variable names of typelist
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.
As we discussed, I think we should eventually converge to a single
Camera
class with different backends (realsense, opencv...). This would probably simplify the design and avoid having to monkeypatch those classes in the tests. Then we could think about the design of this function (which would be a single one). Wdyt?I wondered about the exact same thing but the plural of "info" or "information" in English is without a s.
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 agree, but I think we should homogenise opencv and intelrealsense at this level before merging. What do you think of my suggestion?
I know but we dont write in english, we write in python (which is an english dialect) so the english grammar rules dont apply the same way ^^
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.
IMO, the variable should be renamed to
cameras_info
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 agree, this was because it was copied from a very similar function for opencv cameras (see this discussion).
I'll wait for that discussion to resolve before changing 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.
Definitely nice to keep width/height consistent with the rotated image