Use target camera in image capture start/stop messages #23115
Conversation
| @@ -453,7 +453,7 @@ void ManualControl::send_camera_mode_command(CameraMode camera_mode) | |||
| command.command = vehicle_command_s::VEHICLE_CMD_SET_CAMERA_MODE; | |||
| command.param2 = static_cast<float>(camera_mode); | |||
| command.target_system = _system_id; | |||
| command.target_component = 100; // any camera | |||
| command.target_component = 100; // MAV_COMP_ID_CAMERA | |||
There was a problem hiding this comment.
I marked this case up because:
- It isn't "any camera" - that would be '0'
- Is there any way we can set this so that a manual control can actually set the camera ID to control? Maybe not worth it since if you only have one camera you can probably set its ID to 100.
There was a problem hiding this comment.
Right, I remember discussing this for various commands in the past.
It turns out that some commands encode it in param1, e.g.
https://mavlink.io/en/messages/common.html#MAV_CMD_IMAGE_START_CAPTURE
But SET_MODE does not:
https://mavlink.io/en/messages/common.html#MAV_CMD_SET_CAMERA_MODE
We could add it, but it's a one by one process of adding it.
There was a problem hiding this comment.
Yes. The update in the PR is to add the id for those commands that support it now - those were added in a few months ago. I think we should add it to camera mode, and if you agree, will add a PR to add for the missing cases next week (?).
Previous discussions have suggested having a generic target ID setter that you change before calling an API. This mostly makes sense because some commands have few free params. I don't like that design as it makes the API more complicated. We'll address it when we come to it.
There was a problem hiding this comment.
@julianoes OK, so I added support for MAV_CMD_SET_CAMERA_SOURCE as well, everywhere except the caching code - for example in MissionBase::cacheItem(const mission_item_s &mission_item) - I was wondering if you could tidy that bit because I am concerned I would screw it up. Will do it if you are not happy to take that on.
That is the thing that sets an IR camera or a Visible light camera - I think it is a completely independent setting so propably has its own cache entry.
There was a problem hiding this comment.
PS Finding out the other commands to add this to in mavlink/mavlink#2111
There was a problem hiding this comment.
OK, as per discussion in mavlink/mavlink#2111 the only other command to add for this PR was SET_CAMERA_MODE (DONE)
| @@ -47,7 +47,7 @@ | |||
| * | |||
| * @value 1 GPIO | |||
| * @value 2 Seagull MAP2 (over PWM) | |||
| * @value 3 MAVLink (forward via MAV_CMD_IMAGE_START_CAPTURE) | |||
| * @value 3 MAVLink (Camera Protocol v1) | |||
There was a problem hiding this comment.
FYI, this comment was meaningless - new one reflects the protocol version where is relevant
|
@julianoes OK, so I added code to set the ID for SET_CAMERA_MODE too. So for this to be "rock solid" we may need to add support for caching of MAV_CMD_SET_CAMERA_SOURCE. As per #23115 (comment) "can you do this?" If not, let me know. A complete aside:
|
|
@hamishwillee make sure to run |
MAV_CMD_IMAGE_START_CAPTURE and MAV_CMD_IMAGE_STOP_CAPTURE now take the component ID of the target camera in
param1.This allows the commands to be addressed to a particular camera when used in a mission.
This change modifies the code that publishes the commands to a mavlink camera to use the id if it is between 1 and 255 (inclusive) and otherwise use
100(the allocated first ID in for cameras).Note that range 1 to 6 (inclusive) are supposed to be reserved for cameras connected to FCs, so if we ever support that case, we'd need to change this code.
Previously the command was always addressed to 100. This is a little poor, since IDs cannot be guaranteed to be reserved.