| Message ID | 20210211085527.44667-2-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, On 11/02/2021 08:55, Paul Elder wrote: > If a camera's location is unknown, it should be set so, and not > defaulted to another location. Add such a value to the Location property > enum. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/property_ids.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > index 104e9aaf..66deaa84 100644 > --- a/src/libcamera/property_ids.yaml > +++ b/src/libcamera/property_ids.yaml > @@ -25,6 +25,10 @@ controls: > description: | > The camera is attached to the device in a way that allows it to > be moved freely > + - name: CameraLocationUnknown > + value: 3 Would it make sense to set the Unknown value as 0 (and put it first in the list?). Of course that would mean adjusting the other valuees in this enum too, but while we do not have a stable ABI, I don't see that as being a problem. That would mean that if this property were read as uninitialised (or defaulted?) it would be read as 'unknown' rather than 'Front' ? It probably doesn't matter and doesn't make a difference as if it's set, it would be expected to be set correctly though. Whichever you decide: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + description: | > + The camera location is unknown > > - Rotation: > type: int32_t >
Hi Paul, Thank you for the patch. On Thu, Feb 11, 2021 at 01:50:37PM +0000, Kieran Bingham wrote: > On 11/02/2021 08:55, Paul Elder wrote: > > If a camera's location is unknown, it should be set so, and not > > defaulted to another location. Add such a value to the Location property > > enum. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/libcamera/property_ids.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > index 104e9aaf..66deaa84 100644 > > --- a/src/libcamera/property_ids.yaml > > +++ b/src/libcamera/property_ids.yaml > > @@ -25,6 +25,10 @@ controls: > > description: | > > The camera is attached to the device in a way that allows it to > > be moved freely > > + - name: CameraLocationUnknown > > + value: 3 > > Would it make sense to set the Unknown value as 0 (and put it first in > the list?). > > Of course that would mean adjusting the other valuees in this enum too, > but while we do not have a stable ABI, I don't see that as being a problem. > > That would mean that if this property were read as uninitialised (or > defaulted?) it would be read as 'unknown' rather than 'Front' ? > > It probably doesn't matter and doesn't make a difference as if it's set, > it would be expected to be set correctly though. I think it's a good idea. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Whichever you decide: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > + description: | > > + The camera location is unknown > > > > - Rotation: > > type: int32_t > >
Hi Kieran, On Thu, Feb 11, 2021 at 01:50:37PM +0000, Kieran Bingham wrote: > Hi Paul, > > On 11/02/2021 08:55, Paul Elder wrote: > > If a camera's location is unknown, it should be set so, and not > > defaulted to another location. Add such a value to the Location property > > enum. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/libcamera/property_ids.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > index 104e9aaf..66deaa84 100644 > > --- a/src/libcamera/property_ids.yaml > > +++ b/src/libcamera/property_ids.yaml > > @@ -25,6 +25,10 @@ controls: > > description: | > > The camera is attached to the device in a way that allows it to > > be moved freely > > + - name: CameraLocationUnknown > > + value: 3 > > Would it make sense to set the Unknown value as 0 (and put it first in > the list?). > > Of course that would mean adjusting the other valuees in this enum too, > but while we do not have a stable ABI, I don't see that as being a problem. That's what I was concerned about, but I guess if it's not a concern then I'll reorder them. Thanks, Paul > That would mean that if this property were read as uninitialised (or > defaulted?) it would be read as 'unknown' rather than 'Front' ? > > It probably doesn't matter and doesn't make a difference as if it's set, > it would be expected to be set correctly though. > > Whichever you decide: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > + description: | > > + The camera location is unknown > > > > - Rotation: > > type: int32_t > >
diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml index 104e9aaf..66deaa84 100644 --- a/src/libcamera/property_ids.yaml +++ b/src/libcamera/property_ids.yaml @@ -25,6 +25,10 @@ controls: description: | The camera is attached to the device in a way that allows it to be moved freely + - name: CameraLocationUnknown + value: 3 + description: | + The camera location is unknown - Rotation: type: int32_t
If a camera's location is unknown, it should be set so, and not defaulted to another location. Add such a value to the Location property enum. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/libcamera/property_ids.yaml | 4 ++++ 1 file changed, 4 insertions(+)