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.

Sign up for GitHub

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

Jump to bottom

camera: support multiple cameras within one instance #2386

Merged
merged 9 commits into from
Dec 18, 2024
Merged

camera: support multiple cameras within one instance #2386

merged 9 commits into from
Dec 18, 2024

Conversation

julianoes
Copy link
Collaborator

@julianoes julianoes commented Aug 29, 2024 •
edited
Loading

This changes the API to support more than one camera within one camera plugin instance.

This will enable multiple cameras in language wrappers instead of just C++ as it is now.

Contains several changes:

  • Support MavlinkFTP for camera definition files.
  • Disable photo capture list for now (would need server support and system test testing).
  • Move camera integration tests to system tests.
  • Remove leftover logging integration test.
  • Remove camera mavsdk_server tests because it was too hard too adapt it all.

Contains:
mavlink/MAVSDK-Proto#352
mavlink/MAVSDK-Proto#358

@julianoes
Copy link
Collaborator Author

Closes #2365

@julianoes julianoes force-pushed the pr-multiple-cameras branch 3 times, most recently from 981fe67 to 52499eb Compare September 26, 2024 07:34
@julianoes julianoes force-pushed the pr-multiple-cameras branch from 0aeeb3d to e6e3a71 Compare October 12, 2024 04:02
@julianoes julianoes force-pushed the pr-multiple-cameras branch 5 times, most recently from 22cd76f to 1a62ebe Compare October 30, 2024 23:47
@julianoes julianoes force-pushed the pr-multiple-cameras branch 5 times, most recently from ca2cd4e to 3dcf576 Compare November 13, 2024 21:26
@julianoes julianoes force-pushed the pr-multiple-cameras branch 3 times, most recently from 870fde4 to 2f8c5b7 Compare November 21, 2024 00:43
@julianoes julianoes mentioned this pull request Nov 27, 2024
Open
@julianoes julianoes marked this pull request as ready for review November 28, 2024 00:54
@JonasVautherin JonasVautherin force-pushed the pr-multiple-cameras branch 2 times, most recently from 01c7fac to b4efe05 Compare November 29, 2024 13:58
@julianoes julianoes force-pushed the pr-multiple-cameras branch 2 times, most recently from 99db4c1 to 868cbc8 Compare December 2, 2024 02:04
JonasVautherin
JonasVautherin reviewed Dec 2, 2024
View reviewed changes
examples/start_stop_server/CMakeLists.txt Outdated Show resolved Hide resolved
third_party/CMakeLists.txt Outdated Show resolved Hide resolved
@JonasVautherin
Copy link
Collaborator

My guess is that HTTP_ONLY=OFF in this PR makes curl link libnghttp2, and it fails:

-- Checking for module 'libnghttp2'
--   Found libnghttp2, version 1.64.0
-- Found NGHTTP2: /opt/homebrew/Cellar/libnghttp2/1.64.0/include (found version "1.64.0")

Interestingly libnghttp2 is not available on other platforms and that's fine. I wonder why it gets pulled on iOS (it does not seem to be pulled in macOS 🤔)

@JonasVautherin
Copy link
Collaborator

I tried disabling nghttp2. If you really want to enable http2, we need to add nghttp2 properly as a dependency in the superbuild.

@julianoes julianoes force-pushed the pr-multiple-cameras branch from 674dc60 to 6aae142 Compare December 2, 2024 21:14
@julianoes
Copy link
Collaborator Author

@JonasVautherin I think this is ready to go in, and then we can release v3 and I can start using and testing it.

JonasVautherin
JonasVautherin reviewed Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little bit concerned because it seems like quite a bunch of code in the camera plugin has been disabled 🤔.

Is that expected?

src/mavsdk/plugins/camera/camera_impl.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.cpp Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.h Outdated Show resolved Hide resolved
src/mavsdk/plugins/camera/camera_impl.h Show resolved Hide resolved
@jonathanreeves jonathanreeves mentioned this pull request Dec 10, 2024
Merged
@jonathanreeves jonathanreeves mentioned this pull request Dec 13, 2024
Merged
julianoes and others added 9 commits December 17, 2024 14:16
@julianoes
camera: support multiple cameras within one instance
b4a6708 
This changes the API to support more than one camera within one
camera plugin instance.

This will enable multiple cameras in language wrappers instead of just
C++ as it is now.

Contains several changes:
- Support MavlinkFTP for camera definition files.
- Move camera integration tests to system tests.
- Remove leftover logging integration test.
- Remove camera mavsdk_server tests because it was too hard too adapt it
  all.
- Remove Prepare method.
- Add the argument of component_id to most APIs.
- Add a method to list all found cameras.
@julianoes
third_party: build cURL with https support
2f93feb
@julianoes
third_party: update cURL
f123a14
@julianoes
third_party: update openssl
dc0d323
@julianoes
cmake: unit tests need to link to tinyxml2
66ae28e
@julianoes
core: try different types
b557b50
@julianoes
param_server: fixup copy paste
54fc91e
@julianoes
third_party: try curl without idn2
852b9cd
@JonasVautherin @julianoes
curl: disable nghttp2
2e0e121
@julianoes
Copy link
Collaborator Author

@JonasVautherin do the https changes in this PR make sense?

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

@JonasVautherin
Copy link
Collaborator

JonasVautherin commented Dec 17, 2024 •
edited
Loading

You mean disabling nghttp2? I think it is fine. If someone ever needs HTTP2, they can try to enable it themselves later...

@julianoes
Copy link
Collaborator Author

I was just confused why main doesn't need the changes I have here.

@julianoes julianoes merged commit fb5bd98 into main Dec 18, 2024
39 of 40 checks passed
@julianoes julianoes deleted the pr-multiple-cameras branch December 18, 2024 02:13
This was referenced Dec 18, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonasVautherin JonasVautherin JonasVautherin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@julianoes @JonasVautherin