| Message ID | 20210212054816.53561-5-paul.elder@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, I think the patch subject is wrong :/ On Fri, Feb 12, 2021 at 02:48:16PM +0900, Paul Elder wrote: > Now that we default the camera sensor location to Unknown, handle it in > cam the same way external is handled. I would not call an unknown camera "External". This is also the second different default we assign to "Unknown" in this series. For the HAL it defaults to Front, here it defaults to "External". Isn't this a bit confusing ? I would rather name it just "Camera" without reporting external/front/back or anything else. Thanks j > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > New in v2 > --- > src/cam/main.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index e01be63a..261ed0c8 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -387,6 +387,7 @@ std::string const CamApp::cameraName(const Camera *camera) > name = "Internal back camera"; > break; > case properties::CameraLocationExternal: > + case properties::CameraLocationUnknown: > name = "External camera"; > if (props.contains(properties::Model)) > name += " '" + props.get(properties::Model) + "'"; > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, On 12/02/2021 09:42, Jacopo Mondi wrote: > Hi Paul, > I think the patch subject is wrong :/ > > On Fri, Feb 12, 2021 at 02:48:16PM +0900, Paul Elder wrote: >> Now that we default the camera sensor location to Unknown, handle it in >> cam the same way external is handled. > > I would not call an unknown camera "External". > This is also the second different default we assign to "Unknown" in > this series. For the HAL it defaults to Front, here it defaults to > "External". Isn't this a bit confusing ? > > I would rather name it just "Camera" without reporting > external/front/back or anything else. > I wouldn't even use Camera, just print the model, and the rest of the string: I.e.: > 1: Camera 'HP Wide Vision FHD Camera: HP W' (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251) vs > 2: 'HP Wide Vision FHD Camera: HP I' (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.2-0408:5251) Otherwise, as this is cam/test tool - we should possibly report that the location is unknown: >> 1: Unknown Location 'HP Wide Vision FHD Camera: HP W' (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251) -- Kieran > Thanks > j > >> >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >> >> --- >> New in v2 >> --- >> src/cam/main.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp >> index e01be63a..261ed0c8 100644 >> --- a/src/cam/main.cpp >> +++ b/src/cam/main.cpp >> @@ -387,6 +387,7 @@ std::string const CamApp::cameraName(const Camera *camera) >> name = "Internal back camera"; >> break; >> case properties::CameraLocationExternal: >> + case properties::CameraLocationUnknown: >> name = "External camera"; >> if (props.contains(properties::Model)) >> name += " '" + props.get(properties::Model) + "'"; >> -- >> 2.27.0 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Jacopo and Kieran, On Fri, Feb 12, 2021 at 07:22:37PM +0000, Kieran Bingham wrote: > Hi Paul, > > On 12/02/2021 09:42, Jacopo Mondi wrote: > > Hi Paul, > > I think the patch subject is wrong :/ > > > > On Fri, Feb 12, 2021 at 02:48:16PM +0900, Paul Elder wrote: > >> Now that we default the camera sensor location to Unknown, handle it in > >> cam the same way external is handled. > > > > I would not call an unknown camera "External". > > This is also the second different default we assign to "Unknown" in > > this series. For the HAL it defaults to Front, here it defaults to > > "External". Isn't this a bit confusing ? tbh I think it's fine for different applications to handle the unknown location differently. > > I would rather name it just "Camera" without reporting > > external/front/back or anything else. > > > > I wouldn't even use Camera, just print the model, and the rest of the > string: > > I.e.: > > 1: Camera 'HP Wide Vision FHD Camera: HP W' (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251) > > > vs > > 2: 'HP Wide Vision FHD Camera: HP I' (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.2-0408:5251) > > Otherwise, as this is cam/test tool - we should possibly report that the > location is unknown: > > >> 1: Unknown Location 'HP Wide Vision FHD Camera: HP W' (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251) But for cam I think this would be the better way. Thanks, Paul > >> > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >> > >> --- > >> New in v2 > >> --- > >> src/cam/main.cpp | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp > >> index e01be63a..261ed0c8 100644 > >> --- a/src/cam/main.cpp > >> +++ b/src/cam/main.cpp > >> @@ -387,6 +387,7 @@ std::string const CamApp::cameraName(const Camera *camera) > >> name = "Internal back camera"; > >> break; > >> case properties::CameraLocationExternal: > >> + case properties::CameraLocationUnknown: > >> name = "External camera"; > >> if (props.contains(properties::Model)) > >> name += " '" + props.get(properties::Model) + "'"; > >> -- > >> 2.27.0 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index e01be63a..261ed0c8 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -387,6 +387,7 @@ std::string const CamApp::cameraName(const Camera *camera) name = "Internal back camera"; break; case properties::CameraLocationExternal: + case properties::CameraLocationUnknown: name = "External camera"; if (props.contains(properties::Model)) name += " '" + props.get(properties::Model) + "'";
Now that we default the camera sensor location to Unknown, handle it in cam the same way external is handled. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- New in v2 --- src/cam/main.cpp | 1 + 1 file changed, 1 insertion(+)