[PATCH 0/4] Add direction field to ControlId

[Laurent Pinchart]

Hi Paul,

Thank you for the patches.

On Tue, Nov 26, 2024 at 12:29:59AM +0900, Paul Elder wrote:
> This patch series add support for querying the ControlId for the
> direction that it can be passed.
> 
> This used to only be mentioned in the control id definitions as "This
> control can only be returned in metadata" so this codifies it and allows
> this information to be queried by applications.
> 
> This is an ABI breaking change, so I really want to sneak it in before
> the 0.4.0 release that's coming imminently...
> 
> Patches 1 and 2 prepare control definitions and parsing, while patch 3
> adds the actual support. Patch 4 enables visualization via cam.

Here's a simplification for the use of the direction flags:

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index cd338ac0d653..9af903733439 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -245,7 +245,7 @@ public:
 
 	ControlId(unsigned int id, const std::string &name, const std::string &vendor,
 		  ControlType type, std::size_t size = 0,
-		  const DirectionFlags &direction =
+		  DirectionFlags direction =
 			  static_cast<DirectionFlags>(Direction::In) |
 			  static_cast<DirectionFlags>(Direction::Out),
 		  const std::map<std::string, int32_t> &enumStrMap = {});
@@ -258,13 +258,11 @@ public:
 	std::size_t size() const { return size_; }
 	bool isInput() const
 	{
-		return static_cast<bool>(
-			direction_ & static_cast<DirectionFlags>(Direction::In));
+		return !!(direction_ & Direction::In);
 	}
 	bool isOutput() const
 	{
-		return static_cast<bool>(
-			direction_ & static_cast<DirectionFlags>(Direction::Out));
+		return !!(direction_ & Direction::Out);
 	}
 	const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; }
 
@@ -281,6 +279,8 @@ private:
 	std::map<int32_t, std::string> reverseMap_;
 };
 
+LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)
+
 static inline bool operator==(unsigned int lhs, const ControlId &rhs)
 {
 	return lhs == rhs.id();
@@ -308,9 +308,8 @@ public:
 	using type = T;
 
 	Control(unsigned int id, const char *name, const char *vendor,
-		const ControlId::DirectionFlags &direction =
-			static_cast<ControlId::DirectionFlags>(ControlId::Direction::In) |
-			static_cast<ControlId::DirectionFlags>(ControlId::Direction::Out),
+		ControlId::DirectionFlags direction = ControlId::Direction::In
+						    | ControlId::Direction::Out,
 		const std::map<std::string, int32_t> &enumStrMap = {})
 		: ControlId(id, name, vendor, details::control_type<std::remove_cv_t<T>>::value,
 			    details::control_type<std::remove_cv_t<T>>::size, direction, enumStrMap)
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 30eb17e7f064..1f78de1aaaed 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -402,7 +402,7 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
  */
 ControlId::ControlId(unsigned int id, const std::string &name,
 		     const std::string &vendor, ControlType type,
-		     std::size_t size, const DirectionFlags &direction,
+		     std::size_t size, DirectionFlags direction,
 		     const std::map<std::string, int32_t> &enumStrMap)
 	: id_(id), name_(name), vendor_(vendor), type_(type), size_(size),
 	  direction_(direction), enumStrMap_(enumStrMap)
diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py
index bc1c655f1b9b..2fd20ec674f4 100644
--- a/utils/codegen/controls.py
+++ b/utils/codegen/controls.py
@@ -122,8 +122,8 @@ class Control(object):
 
     @property
     def direction(self):
-        in_flag = 'static_cast<ControlId::DirectionFlags>(ControlId::Direction::In)'
-        out_flag = 'static_cast<ControlId::DirectionFlags>(ControlId::Direction::Out)'
+        in_flag = 'ControlId::Direction::In'
+        out_flag = 'ControlId::Direction::Out'
 
         if self.__direction == 'inout':
             return f'{in_flag} | {out_flag}'


Unfortunately the static_cast in the ControlId constructor have to stay.
An alternative would be to require the direction argument, which may not
be a bad idea given that all controls should have a direction.

> Paul Elder (4):
>   libcamera: controls: Populate direction field in control definitions
>   utils: codegen: controls.py: Parse direction information
>   libcamera: controls: Add support for querying direction information
>   apps: cam: Print control direction information
> 
>  include/libcamera/controls.h         | 27 +++++++++++++++++-
>  src/apps/cam/camera_session.cpp      | 10 +++++--
>  src/libcamera/control_ids.cpp.in     |  4 +--
>  src/libcamera/control_ids_core.yaml  | 12 ++++++++
>  src/libcamera/control_ids_draft.yaml |  7 +++++
>  src/libcamera/controls.cpp           | 42 ++++++++++++++++++++++++++--
>  utils/codegen/controls.py            | 21 ++++++++++++++
>  7 files changed, 116 insertions(+), 7 deletions(-)

-- 
Regards,

Laurent Pinchart