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

Zed2i -> Zed rename #152

Merged
merged 6 commits into from
Dec 10, 2024
Merged

Zed2i -> Zed rename #152

merged 6 commits into from
Dec 10, 2024

Conversation

m-decoster
Copy link
Contributor

Describe your changes

fix #151

Renamed Zed2i to Zed and zed2i.py to zed.py, but kept the old names as aliases for backwards compatibility.

The Zed2i class now inherits from Zed without making any changes, such that existing code using Zed2i will still work. A @deprecated decorator has been added to the Zed2i class.

Alternatively, we could not deprecate Zed2i, but add a second class ZedMini that also inherits from Zed. But since the Zed SDK itself is generic over the different camera types, I'm a proponent of a generic Zed class in airo-mono and deprecating the old Zed2i, as I've done here.

Thoughts @tlpss @Victorlouisdg ?

Checklist

  • Have you modified the changelog?
  • If you made changes to the hardware interfaces, have you tested them using the manual tests?

@tlpss
Copy link
Contributor

tlpss commented Dec 10, 2024

thanks for making it more generic!

I think this is a good way to handle the change, I had the same in mind.

maybe one suggestion: keep the Zed2i class in the Zed2i file instead of in the zed file? makes more sense imo.

Questions:

  • does our class support all zed cameras? (resolution profiles etc)
  • do we need separate tests for the zed mini?

@tlpss
Copy link
Contributor

tlpss commented Dec 10, 2024

additional question:

do we already want to update all use of the Zed2i in the airo-mono codebase? e.g. https://github.com/airo-ugent/airo-mono/blob/zed_refactor/airo-camera-toolkit/airo_camera_toolkit/cameras/camera_discovery.py

@m-decoster
Copy link
Contributor Author

maybe one suggestion: keep the Zed2i class in the Zed2i file instead of in the zed file? makes more sense imo.

Good idea, will do

Questions:

  • does our class support all zed cameras? (resolution profiles etc)

It supports ZED, ZED 2/2i and ZED Mini. For supporting the GMSL cameras we need to add additional resolution profiles: https://www.stereolabs.com/docs/video/camera-controls#resolution-for-gmsl-cameras-zed-xx-mini

I could add these if we want.

  • do we need separate tests for the zed mini?

The Zed mini works out of the box with the old Zed2i class and all manual tests pass*, so I don't think this is needed per se

@tlpss
Copy link
Contributor

tlpss commented Dec 10, 2024

  • does our class support all zed cameras? (resolution profiles etc)

It supports ZED, ZED 2/2i and ZED Mini. For supporting the GMSL cameras we need to add additional resolution profiles: https://www.stereolabs.com/docs/video/camera-controls#resolution-for-gmsl-cameras-zed-xx-mini

I could add these if we want.

No don't think we will use them in the near future. So if it works for all Zed 1 and zed 2 cameras, we're good I think.

  • do we need separate tests for the zed mini?

The Zed mini works out of the box with the old Zed2i class and all manual tests pass*, so I don't think this is needed per se

ok, great!

@m-decoster m-decoster merged commit e20960e into main Dec 10, 2024
25 checks passed
@m-decoster m-decoster deleted the zed_refactor branch December 10, 2024 13:59
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.

Rename Zed2i to Zed?
2 participants