Camera Definition #747

dogmaphobic · 2017-07-07T07:06:07Z

This is in preparation for adding support for a Camera Definition File as discussed in discuss.px4.io.

Flagging messages and commands that are to be removed
Adding an URI for fetching the camera definition file (if any) to the CAMERA_INFORMATION message
Adding a revision number for the definition file (to check against an already cached version)
Adding a capability flag to the CAMERA_INFORMATION.
Adding an enum with a bitmap describing the various capability flags (CAMERA_CAP_FLAGS)
Adding a device_id field to the COMMAND_ACK message to identify target devices that share a component ID (Camera, Storage, etc.)

@julianoes Please check and see if the implementation of the CAMERA_CAP_FLAGS is correct. This is the first time I define a bitmap.

Note that this doesn’t remove any existing functionality yet. Only when the camera definition file is fully implemented and validated will the Experimental messages and commands be removed.

…raDefintion * 'master' of https://github.com/mavlink/mavlink: Support trigger before/after distance trigger start/stop

julianoes · 2017-07-07T08:20:36Z

message_definitions/v1.0/common.xml

@@ -3158,6 +3175,7 @@
      <field type="uint8_t" name="result" enum="MAV_RESULT">See MAV_RESULT enum</field>
      <extensions/>
      <field type="uint8_t" name="progress">WIP: Needs to be set when MAV_RESULT is MAV_RESULT_IN_PROGRESS, values from 0 to 100 for progress percentage, 255 for unknown progress.</field>
+      <field type="uint8_t" name="device_id">WIP: Used to identify targeted device when multiple devices use same component ID (i.e. Camera, Storage, etc.)</field>


Hang on. What is this? There is already system ID and component ID, and there are internal camera IDs. And you also want to add a device ID?

Take for example this message:

MAV_CMD_REQUEST_CAMERA_INFORMATION Param #1 | Camera ID (0 for all cameras, 1 for first, 2 for second, etc.)

I need to know what Camera ID this response is for. On a parallel system, sending messages from arbitrary camera controllers, the system needs to know to which (camera) controller to route the message to. In this case, both System ID and Component ID are the same. Only the Camera ID changes.

Will this change require existing users of COMMAND_ACK to make corresponding changes in their code?

This would be the same changes that occurred when the progress field was added. If you don't use the new field, existing code would simply ignore it.

On a parallel system

For the sake of simplicity, I would not try to parallelize the case of two camera as one mavlink node (with system and component ID). I think this will be confusing for most developers implementing mavlink commands.

The problem I'm trying to solve is to determine which camera is responding to a command when both system and component IDs are the same.

I removed it based on the fact we decided to remove Camera ID from all camera messages. Separate cameras are to be identified using different Component ID instead.

julianoes · 2017-07-07T16:34:49Z

message_definitions/v1.0/common.xml

@@ -4078,33 +4096,36 @@
      <field type="uint8_t" name="camera_count">Number of cameras</field>
      <field type="uint8_t[32]" name="vendor_name">Name of the camera vendor</field>
      <field type="uint8_t[32]" name="model_name">Name of the camera model</field>
-      <field type="uint32_t" name="firmware_version">Version of the camera firmware</field>
+      <field type="uint32_t" name="firmware_version">Version of the camera firmware (v &lt;&lt; 24 &amp; 0xff = Dev, v &lt;&lt; 16 &amp; 0xff = Patch, v &lt;&lt; 8 &amp; 0xff = Minor, v &amp; 0xff = Major)</field>


I wonder if these & will get htmlized anyway.

There is one way to find out :)

Good luck :).

julianoes · 2017-07-07T16:35:35Z

message_definitions/v1.0/common.xml

      <field type="float" name="focal_length" units="mm">Focal length in mm</field>
      <field type="float" name="sensor_size_h" units="mm">Image sensor size horizontal in mm</field>
      <field type="float" name="sensor_size_v" units="mm">Image sensor size vertical in mm</field>
      <field type="uint16_t" name="resolution_h" units="pix">Image resolution in pixels horizontal</field>
      <field type="uint16_t" name="resolution_v" units="pix">Image resolution in pixels vertical</field>
      <field type="uint8_t" name="lens_id">Reserved for a lens ID</field>
+      <field type="uint32_t" name="flags" enum="CAMERA_CAP_FLAGS">CAMERA_CAP_FLAGS enum flags (bitmap) describing camera capabilities.</field>
+      <field type="uint16_t" name="cam_definition_version">Camera definition version (iteration)</field>


That's the schema/protocol version?

No, the content version. It will tell if some parameter or option (or rule, localization, etc.) changed. The GCS would have cached the file and there is no reason to fetch it again if it's the same version.

Ah, I thought the version would be included in the file string. I guess this allows to have one http URI, and then a version number on top.

We decided to use XML instead. Json simply isn't conducive for this. As such, the schema is defined in the file header.

julianoes · 2017-07-07T16:36:14Z

message_definitions/v1.0/common.xml

      <field type="float" name="focal_length" units="mm">Focal length in mm</field>
      <field type="float" name="sensor_size_h" units="mm">Image sensor size horizontal in mm</field>
      <field type="float" name="sensor_size_v" units="mm">Image sensor size vertical in mm</field>
      <field type="uint16_t" name="resolution_h" units="pix">Image resolution in pixels horizontal</field>
      <field type="uint16_t" name="resolution_v" units="pix">Image resolution in pixels vertical</field>
      <field type="uint8_t" name="lens_id">Reserved for a lens ID</field>
+      <field type="uint32_t" name="flags" enum="CAMERA_CAP_FLAGS">CAMERA_CAP_FLAGS enum flags (bitmap) describing camera capabilities.</field>
+      <field type="uint16_t" name="cam_definition_version">Camera definition version (iteration)</field>
+      <field type="uint8_t[140]" name="cam_definition_uri">Camera definition URI (if any, otherwise only basic functions will be available).</field>


You don't want a local and a remote URI?

I don't see a reason to have two separate URIs. If the vehicle/camera is able to serve the file, why have an external URI? If it can't, it can't and there would be only one. Not to mention the lack of space within the message. Can you think of a reason why we would need both local and external?

Having multiple URI might lead to confusion/mismatch etc at some point in time. Better to have a single URI.

lbegani · 2017-07-10T05:01:49Z

message_definitions/v1.0/common.xml

@@ -4078,33 +4096,36 @@
      <field type="uint8_t" name="camera_count">Number of cameras</field>


Not related, what does camera_count mean? Number of sensors in the given camera?

When you request camera information and set Camera ID to 0 (All Cameras), you will receive 0 or more CAMERA_INFORMATION messages in response. If the vehicle has two cameras, you would receive two CAMERA_INFORMATION messages where one would have camera_id set to 1 and the other camera_id would be set to 2. In this case, camera_count would be set to 2. That's how you know how many cameras there are and to make sure you received a CAMERA_INFORMATION for each. If the camera_count is set to 2 and you only get one CAMERA_INFORMATION back, it means one of the messages was lost. That would allow you to request again the missing message (and target the request to the missing camera id.)

This has been removed as we discussed.

lbegani · 2017-07-10T05:04:20Z

message_definitions/v1.0/common.xml

      <field type="float" name="focal_length" units="mm">Focal length in mm</field>
      <field type="float" name="sensor_size_h" units="mm">Image sensor size horizontal in mm</field>
      <field type="float" name="sensor_size_v" units="mm">Image sensor size vertical in mm</field>
      <field type="uint16_t" name="resolution_h" units="pix">Image resolution in pixels horizontal</field>
      <field type="uint16_t" name="resolution_v" units="pix">Image resolution in pixels vertical</field>
      <field type="uint8_t" name="lens_id">Reserved for a lens ID</field>
+      <field type="uint32_t" name="flags" enum="CAMERA_CAP_FLAGS">CAMERA_CAP_FLAGS enum flags (bitmap) describing camera capabilities.</field>
+      <field type="uint16_t" name="cam_definition_version">Camera definition version (iteration)</field>
+      <field type="uint8_t[140]" name="cam_definition_uri">Camera definition URI (if any, otherwise only basic functions will be available).</field>


Having multiple URI might lead to confusion/mismatch etc at some point in time. Better to have a single URI.

lbegani · 2017-07-10T05:07:02Z

message_definitions/v1.0/common.xml

@@ -3158,6 +3175,7 @@
      <field type="uint8_t" name="result" enum="MAV_RESULT">See MAV_RESULT enum</field>
      <extensions/>
      <field type="uint8_t" name="progress">WIP: Needs to be set when MAV_RESULT is MAV_RESULT_IN_PROGRESS, values from 0 to 100 for progress percentage, 255 for unknown progress.</field>
+      <field type="uint8_t" name="device_id">WIP: Used to identify targeted device when multiple devices use same component ID (i.e. Camera, Storage, etc.)</field>


Will this change require existing users of COMMAND_ACK to make corresponding changes in their code?

lbegani · 2017-07-10T05:09:35Z

message_definitions/v1.0/common.xml

+      <entry value="1" name="CAMERA_CAP_FLAGS_CAPTURE_VIDEO">
+        <description>Camera is able to record video.</description>
+      </entry>
+      <entry value="2" name="CAMERA_CAP_FLAGS_CAPTURE_PHOTO">


I think the term IMAGE sounds better than PHOTO (Subjective)

I have no preference. If this what everybody thinks, I will change it.

On the other hand, PHOTO works with PHOTO mode.

That was the reason I used "Photo". But the command to "take pictures" is called "START IMAGE CAPTURE" so we sort of have two different terms for the same thing already.

Lets make them all same

Done. It's now IMAGE wherever appropriate.

lbegani · 2017-07-10T05:13:30Z

message_definitions/v1.0/common.xml

@@ -2586,6 +2585,24 @@
        <description>Landing target represented by a pre-defined visual shape/feature (ex: X-marker, H-marker, square)</description>
      </entry>
    </enum>
+    <enum name="CAMERA_CAP_FLAGS">


Shouldn't all capability definition be subsumed in camera definition file? Or is this only to be used for specific cases where def file isn't available?

The idea is for when you don't have the camera definition file. While implementing this, I realized that most of this info within the definition file is redundant and it should not be there (in the definition file). As this is the first call you make for camera discovery, you will receive this message back with the information within. There is no reason to duplicate it within the camera definition as well.

Rename Photo to Image where appropriate Remove device_id from COMMAND_ACK

dogmaphobic · 2017-07-12T19:08:57Z

Flagged all references to Camera ID from the various camera messages as EXPERIMENTAL: IT WILL BE REMOVED. Each camera should present itself using a different component ID. As far as we could tell, we could not find any concrete use case where one single camera would provide separate interfaces.
For consistency’s sake, I’ve changed all references to “Photo” and replaced with “Image” where appropriate. For example, CAMERA_CAP_FLAGS_CAPTURE_PHOTO is now CAMERA_CAP_FLAGS_CAPTURE_IMAGE.
Removed the device_id field from the COMMAND_ACK message. That was based on the assumption we had multiple cameras using one same Component ID. As that is not the case, we don’t need this device_id field.

Speaking of device_id, I’ve also noticed there is a parameter in MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN for an ID where the description uses Camera ID as an example. I’m assuming that should be renamed to something else? If you want to reboot a camera, wouldn’t you send a MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN command to the camera’s specific Component ID? Unless the autopilot is broadcasting this to all components? Why is it there?

dogmaphobic · 2017-07-12T19:25:16Z

Putting this here for now but it has to be documented somewhere more appropriate at some point.

How to deal with storage (the various STORAGE messages).

When dealing with a camera’s storage unit, we will use the camera’s component ID when sending these messages. That is, if you want to format the storage of a given camera (or query storage status, etc.) you will send that command to the camera’s component ID. If the camera has its own storage, it can just go ahead and format it (faster than erasing files). If the camera shares storage with something else, it’s up to the camera to deal with it. In that case, when it gets a format command, it would erase its files, leaving the storage alone. If it finds the storage is corrupted (and therefore requiring formatting) it would elect to format it instead if appropriate.

(I need to add this to the various STORAGE messages)

julianoes · 2017-07-13T03:11:37Z

message_definitions/v1.0/common.xml

      <field type="float" name="focal_length" units="mm">Focal length in mm</field>
      <field type="float" name="sensor_size_h" units="mm">Image sensor size horizontal in mm</field>
      <field type="float" name="sensor_size_v" units="mm">Image sensor size vertical in mm</field>
      <field type="uint16_t" name="resolution_h" units="pix">Image resolution in pixels horizontal</field>
      <field type="uint16_t" name="resolution_v" units="pix">Image resolution in pixels vertical</field>
      <field type="uint8_t" name="lens_id">Reserved for a lens ID</field>
+      <field type="uint32_t" name="flags" enum="CAMERA_CAP_FLAGS">CAMERA_CAP_FLAGS enum flags (bitmap) describing camera capabilities.</field>
+      <field type="uint16_t" name="cam_definition_version">Camera definition version (iteration)</field>


* master: Specify that you need to use the command's target_component to target a specific component's storage. Extended parameter handling (#752) Common: Add GPS_FIX_TYPE_PPP message_definitions: add white balance lock message_definitions: improve image capture status # Conflicts: # message_definitions/v1.0/common.xml

* Preparing for camera definition * Removing Camera ID (flagging it as to be removed) Rename Photo to Image where appropriate Remove device_id from COMMAND_ACK * Reserving space for 6 cameras.

dogmaphobic added 2 commits July 7, 2017 02:51

Preparing for camera definition

299b47b

Merge branch 'master' of https://github.com/mavlink/mavlink into came…

62af94b

…raDefintion * 'master' of https://github.com/mavlink/mavlink: Support trigger before/after distance trigger start/stop

dogmaphobic requested a review from julianoes July 7, 2017 07:06

julianoes reviewed Jul 7, 2017

View reviewed changes

lbegani reviewed Jul 10, 2017

View reviewed changes

julianoes changed the title ~~Camera Defintion~~ Camera Definition Jul 10, 2017

Removing Camera ID (flagging it as to be removed)

b35d86e

Rename Photo to Image where appropriate Remove device_id from COMMAND_ACK

julianoes approved these changes Jul 13, 2017

View reviewed changes

dogmaphobic added 2 commits July 15, 2017 14:03

Reserving space for 6 cameras.

4032a94

LorenzMeier merged commit e722542 into master Jul 15, 2017

LorenzMeier deleted the cameraDefintion branch July 15, 2017 19:12

julianoes mentioned this pull request Jul 30, 2020

common: remove camera ID #1436

Merged

		@@ -4078,33 +4096,36 @@
		<field type="uint8_t" name="camera_count">Number of cameras</field>

Camera Definition #747

Camera Definition #747

Conversation

dogmaphobic commented Jul 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dogmaphobic Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dogmaphobic commented Jul 12, 2017

dogmaphobic commented Jul 12, 2017 • edited

Choose a reason for hiding this comment

dogmaphobic commented Jul 7, 2017 •

edited

dogmaphobic Jul 10, 2017 •

edited

dogmaphobic commented Jul 12, 2017 •

edited