| Message ID | 20200806130937.2991606-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > Instead of only printing the camera ID which is not intended for humans > to read and parse create a more user friendly string when prating camera > names. The ID is still printed as it is one option used to select camera > using the --camera option. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 4 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index cc3facd5a5b22092..5af76f6965ef2387 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -21,6 +21,39 @@ > > using namespace libcamera; > > +std::string cameraName(const Camera *camera) > +{ > + const ControlList &props = camera->properties(); > + std::string name; > + > + if (props.contains(properties::Model)) > + name += props.get(properties::Model) + " "; > + > + if (props.contains(properties::Location)) { > + switch (props.get(properties::Location)) { > + case properties::CameraLocationFront: > + name += "facing front "; > + break; > + case properties::CameraLocationBack: > + name += "facing back "; > + break; > + case properties::CameraLocationExternal: > + name += "external "; > + break; > + } > + } > + > + if (props.contains(properties::Rotation)) > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > + > + if (!name.empty()) > + name += "with id "; Just a quick question while skimming through the series. cam can printout camera properties, do we need to make 'friendly' names 100 characters to repeat what's already available there ? > + > + name += camera->id(); > + > + return name; > +} > + > class CamApp > { > public: > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > return -EINVAL; > } > > - std::cout << "Using camera " << camera_->id() << std::endl; > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > ret = prepareConfig(); > if (ret) { > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > { > - std::cout << "Camera Added: " << cam->id() << std::endl; > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > } > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > { > - std::cout << "Camera Removed: " << cam->id() << std::endl; > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > } > > int CamApp::run() > @@ -340,7 +373,7 @@ int CamApp::run() > > unsigned int index = 1; > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > - std::cout << index << ": " << cam->id() << std::endl; > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > index++; > } > } > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > > Instead of only printing the camera ID which is not intended for humans > > to read and parse create a more user friendly string when prating camera > > names. The ID is still printed as it is one option used to select camera > > using the --camera option. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 37 insertions(+), 4 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index cc3facd5a5b22092..5af76f6965ef2387 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -21,6 +21,39 @@ > > > > using namespace libcamera; > > > > +std::string cameraName(const Camera *camera) > > +{ > > + const ControlList &props = camera->properties(); > > + std::string name; > > + > > + if (props.contains(properties::Model)) > > + name += props.get(properties::Model) + " "; > > + > > + if (props.contains(properties::Location)) { > > + switch (props.get(properties::Location)) { > > + case properties::CameraLocationFront: > > + name += "facing front "; > > + break; > > + case properties::CameraLocationBack: > > + name += "facing back "; > > + break; > > + case properties::CameraLocationExternal: > > + name += "external "; > > + break; > > + } > > + } > > + > > + if (props.contains(properties::Rotation)) > > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > > + > > + if (!name.empty()) > > + name += "with id "; > > Just a quick question while skimming through the series. cam can > printout camera properties, do we need to make 'friendly' names > 100 characters to repeat what's already available there ? I know it can print properties :-) I'm happy to change this to contain more or less information, my main goal of throwing in everything here is to showcase with cam how applications can create names. What properties would you like to see make up the user-friendly name? > > > > + > > + name += camera->id(); > > + > > + return name; > > +} > > + > > class CamApp > > { > > public: > > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > > return -EINVAL; > > } > > > > - std::cout << "Using camera " << camera_->id() << std::endl; > > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > > > ret = prepareConfig(); > > if (ret) { > > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > > { > > - std::cout << "Camera Added: " << cam->id() << std::endl; > > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > > } > > > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > > { > > - std::cout << "Camera Removed: " << cam->id() << std::endl; > > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > > } > > > > int CamApp::run() > > @@ -340,7 +373,7 @@ int CamApp::run() > > > > unsigned int index = 1; > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > - std::cout << index << ": " << cam->id() << std::endl; > > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > > index++; > > } > > } > > -- > > 2.28.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On 06/08/2020 14:32, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your feedback. > > On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: >> Hi Niklas, >> >> On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: >>> Instead of only printing the camera ID which is not intended for humans >>> to read and parse create a more user friendly string when prating camera >>> names. The ID is still printed as it is one option used to select camera >>> using the --camera option. >>> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>> --- >>> src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 37 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp >>> index cc3facd5a5b22092..5af76f6965ef2387 100644 >>> --- a/src/cam/main.cpp >>> +++ b/src/cam/main.cpp >>> @@ -21,6 +21,39 @@ >>> >>> using namespace libcamera; >>> >>> +std::string cameraName(const Camera *camera) >>> +{ >>> + const ControlList &props = camera->properties(); >>> + std::string name; >>> + >>> + if (props.contains(properties::Model)) >>> + name += props.get(properties::Model) + " "; >>> + >>> + if (props.contains(properties::Location)) { >>> + switch (props.get(properties::Location)) { >>> + case properties::CameraLocationFront: >>> + name += "facing front "; >>> + break; >>> + case properties::CameraLocationBack: >>> + name += "facing back "; >>> + break; >>> + case properties::CameraLocationExternal: >>> + name += "external "; >>> + break; >>> + } >>> + } >>> + >>> + if (props.contains(properties::Rotation)) >>> + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; >>> + >>> + if (!name.empty()) >>> + name += "with id "; >> >> Just a quick question while skimming through the series. cam can >> printout camera properties, do we need to make 'friendly' names >> 100 characters to repeat what's already available there ? > > I know it can print properties :-) > > I'm happy to change this to contain more or less information, my main > goal of throwing in everything here is to showcase with cam how > applications can create names. > > What properties would you like to see make up the user-friendly name? I would say model and location would be enough. I don't think rotation or 'with id' should be there (though rendering the id in the string is useful itself) -- Kieran > >> >> >>> + >>> + name += camera->id(); >>> + >>> + return name; >>> +} >>> + >>> class CamApp >>> { >>> public: >>> @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) >>> return -EINVAL; >>> } >>> >>> - std::cout << "Using camera " << camera_->id() << std::endl; >>> + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; >>> >>> ret = prepareConfig(); >>> if (ret) { >>> @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() >>> >>> void CamApp::cameraAdded(std::shared_ptr<Camera> cam) >>> { >>> - std::cout << "Camera Added: " << cam->id() << std::endl; >>> + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; >>> } >>> >>> void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) >>> { >>> - std::cout << "Camera Removed: " << cam->id() << std::endl; >>> + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; >>> } >>> >>> int CamApp::run() >>> @@ -340,7 +373,7 @@ int CamApp::run() >>> >>> unsigned int index = 1; >>> for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { >>> - std::cout << index << ": " << cam->id() << std::endl; >>> + std::cout << index << ": " << cameraName(cam.get()) << std::endl; >>> index++; >>> } >>> } >>> -- >>> 2.28.0 >>> >>> _______________________________________________ >>> libcamera-devel mailing list >>> libcamera-devel@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Niklas, On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your feedback. > > On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > > > Instead of only printing the camera ID which is not intended for humans > > > to read and parse create a more user friendly string when prating camera > > > names. The ID is still printed as it is one option used to select camera > > > using the --camera option. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > > > 1 file changed, 37 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > index cc3facd5a5b22092..5af76f6965ef2387 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -21,6 +21,39 @@ > > > > > > using namespace libcamera; > > > > > > +std::string cameraName(const Camera *camera) > > > +{ > > > + const ControlList &props = camera->properties(); > > > + std::string name; > > > + > > > + if (props.contains(properties::Model)) > > > + name += props.get(properties::Model) + " "; > > > + > > > + if (props.contains(properties::Location)) { > > > + switch (props.get(properties::Location)) { > > > + case properties::CameraLocationFront: > > > + name += "facing front "; > > > + break; > > > + case properties::CameraLocationBack: > > > + name += "facing back "; > > > + break; > > > + case properties::CameraLocationExternal: > > > + name += "external "; > > > + break; > > > + } > > > + } > > > + > > > + if (props.contains(properties::Rotation)) > > > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > > > + > > > + if (!name.empty()) > > > + name += "with id "; > > > > Just a quick question while skimming through the series. cam can > > printout camera properties, do we need to make 'friendly' names > > 100 characters to repeat what's already available there ? > > I know it can print properties :-) > > I'm happy to change this to contain more or less information, my main > goal of throwing in everything here is to showcase with cam how > applications can create names. > > What properties would you like to see make up the user-friendly name? None if not useful to distinguish between cameras with the same name ? ie 1- ov5670 (front) 2- ov5670 (back) I know we could have 1- ov5670 (external) 2- ov5670 (external) but at that point, I think we've done the best we could > > > > > > > > + > > > + name += camera->id(); > > > + > > > + return name; > > > +} > > > + > > > class CamApp > > > { > > > public: > > > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > > > return -EINVAL; > > > } > > > > > > - std::cout << "Using camera " << camera_->id() << std::endl; > > > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > > > > > ret = prepareConfig(); > > > if (ret) { > > > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > > > > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > > > { > > > - std::cout << "Camera Added: " << cam->id() << std::endl; > > > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > > > } > > > > > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > > > { > > > - std::cout << "Camera Removed: " << cam->id() << std::endl; > > > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > > > } > > > > > > int CamApp::run() > > > @@ -340,7 +373,7 @@ int CamApp::run() > > > > > > unsigned int index = 1; > > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > > - std::cout << index << ": " << cam->id() << std::endl; > > > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > > > index++; > > > } > > > } > > > -- > > > 2.28.0 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your feedback. > > > > On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > > > Hi Niklas, > > > > > > On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > > > > Instead of only printing the camera ID which is not intended for humans > > > > to read and parse create a more user friendly string when prating camera > > > > names. The ID is still printed as it is one option used to select camera > > > > using the --camera option. > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > --- > > > > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > > > > 1 file changed, 37 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > > index cc3facd5a5b22092..5af76f6965ef2387 100644 > > > > --- a/src/cam/main.cpp > > > > +++ b/src/cam/main.cpp > > > > @@ -21,6 +21,39 @@ > > > > > > > > using namespace libcamera; > > > > > > > > +std::string cameraName(const Camera *camera) > > > > +{ > > > > + const ControlList &props = camera->properties(); > > > > + std::string name; > > > > + > > > > + if (props.contains(properties::Model)) > > > > + name += props.get(properties::Model) + " "; > > > > + > > > > + if (props.contains(properties::Location)) { > > > > + switch (props.get(properties::Location)) { > > > > + case properties::CameraLocationFront: > > > > + name += "facing front "; > > > > + break; > > > > + case properties::CameraLocationBack: > > > > + name += "facing back "; > > > > + break; > > > > + case properties::CameraLocationExternal: > > > > + name += "external "; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (props.contains(properties::Rotation)) > > > > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > > > > + > > > > + if (!name.empty()) > > > > + name += "with id "; > > > > > > Just a quick question while skimming through the series. cam can > > > printout camera properties, do we need to make 'friendly' names > > > 100 characters to repeat what's already available there ? > > > > I know it can print properties :-) > > > > I'm happy to change this to contain more or less information, my main > > goal of throwing in everything here is to showcase with cam how > > applications can create names. > > > > What properties would you like to see make up the user-friendly name? > > None if not useful to distinguish between cameras with the same name ? > > ie > 1- ov5670 (front) > 2- ov5670 (back) > > I know we could have > 1- ov5670 (external) > 2- ov5670 (external) I like this and will use it for next version. But I really think we need to print the id as well as it is one of two possible augments to --camera to select which one is used. How about 1: ov5695 (front) - /base/i2c@ff160000/camera@36 2: ov2685 (back) - /base/i2c@ff160000/camera@3c ? > > but at that point, I think we've done the best we could > > > > > > > > > > > > > + > > > > + name += camera->id(); > > > > + > > > > + return name; > > > > +} > > > > + > > > > class CamApp > > > > { > > > > public: > > > > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > > > > return -EINVAL; > > > > } > > > > > > > > - std::cout << "Using camera " << camera_->id() << std::endl; > > > > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > > > > > > > ret = prepareConfig(); > > > > if (ret) { > > > > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > > > > > > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > > > > { > > > > - std::cout << "Camera Added: " << cam->id() << std::endl; > > > > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > > > > } > > > > > > > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > > > > { > > > > - std::cout << "Camera Removed: " << cam->id() << std::endl; > > > > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > > > > } > > > > > > > > int CamApp::run() > > > > @@ -340,7 +373,7 @@ int CamApp::run() > > > > > > > > unsigned int index = 1; > > > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > > > - std::cout << index << ": " << cam->id() << std::endl; > > > > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > > > > index++; > > > > } > > > > } > > > > -- > > > > 2.28.0 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On 06/08/2020 14:46, Niklas Söderlund wrote: > Hi Jacopo, > > On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote: >> Hi Niklas, >> >> On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: >>> Hi Jacopo, >>> >>> Thanks for your feedback. >>> >>> On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: >>>> Hi Niklas, >>>> >>>> On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: >>>>> Instead of only printing the camera ID which is not intended for humans >>>>> to read and parse create a more user friendly string when prating camera >>>>> names. The ID is still printed as it is one option used to select camera >>>>> using the --camera option. >>>>> >>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>>>> --- >>>>> src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- >>>>> 1 file changed, 37 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp >>>>> index cc3facd5a5b22092..5af76f6965ef2387 100644 >>>>> --- a/src/cam/main.cpp >>>>> +++ b/src/cam/main.cpp >>>>> @@ -21,6 +21,39 @@ >>>>> >>>>> using namespace libcamera; >>>>> >>>>> +std::string cameraName(const Camera *camera) >>>>> +{ >>>>> + const ControlList &props = camera->properties(); >>>>> + std::string name; >>>>> + >>>>> + if (props.contains(properties::Model)) >>>>> + name += props.get(properties::Model) + " "; >>>>> + >>>>> + if (props.contains(properties::Location)) { >>>>> + switch (props.get(properties::Location)) { >>>>> + case properties::CameraLocationFront: >>>>> + name += "facing front "; >>>>> + break; >>>>> + case properties::CameraLocationBack: >>>>> + name += "facing back "; >>>>> + break; >>>>> + case properties::CameraLocationExternal: >>>>> + name += "external "; >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + if (props.contains(properties::Rotation)) >>>>> + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; >>>>> + >>>>> + if (!name.empty()) >>>>> + name += "with id "; >>>> >>>> Just a quick question while skimming through the series. cam can >>>> printout camera properties, do we need to make 'friendly' names >>>> 100 characters to repeat what's already available there ? >>> >>> I know it can print properties :-) >>> >>> I'm happy to change this to contain more or less information, my main >>> goal of throwing in everything here is to showcase with cam how >>> applications can create names. >>> >>> What properties would you like to see make up the user-friendly name? >> >> None if not useful to distinguish between cameras with the same name ? >> >> ie >> 1- ov5670 (front) >> 2- ov5670 (back) >> >> I know we could have >> 1- ov5670 (external) >> 2- ov5670 (external) > > I like this and will use it for next version. > > But I really think we need to print the id as well as it is one of two > possible augments to --camera to select which one is used. How about > > 1: ov5695 (front) - /base/i2c@ff160000/camera@36 > 2: ov2685 (back) - /base/i2c@ff160000/camera@3c I know we can reference cameras by index, but when referencing by name, does this mean the user would have to type/paste the whole line? or will it match on first unique match? cam -C -c "ov2685 (back)" is almost friendly enough to type ... cam -C -c "ov2685 (back) - /base/i2c@ff160000/camera@3c" is not something anyone is going to be willing to type by hand. Maybe that's not a problem if it's not a design consideration anyway. -- Kieran > > ? > >> >> but at that point, I think we've done the best we could > >> >>> >>>> >>>> >>>>> + >>>>> + name += camera->id(); >>>>> + >>>>> + return name; >>>>> +} >>>>> + >>>>> class CamApp >>>>> { >>>>> public: >>>>> @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) >>>>> return -EINVAL; >>>>> } >>>>> >>>>> - std::cout << "Using camera " << camera_->id() << std::endl; >>>>> + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; >>>>> >>>>> ret = prepareConfig(); >>>>> if (ret) { >>>>> @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() >>>>> >>>>> void CamApp::cameraAdded(std::shared_ptr<Camera> cam) >>>>> { >>>>> - std::cout << "Camera Added: " << cam->id() << std::endl; >>>>> + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; >>>>> } >>>>> >>>>> void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) >>>>> { >>>>> - std::cout << "Camera Removed: " << cam->id() << std::endl; >>>>> + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; >>>>> } >>>>> >>>>> int CamApp::run() >>>>> @@ -340,7 +373,7 @@ int CamApp::run() >>>>> >>>>> unsigned int index = 1; >>>>> for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { >>>>> - std::cout << index << ": " << cam->id() << std::endl; >>>>> + std::cout << index << ": " << cameraName(cam.get()) << std::endl; >>>>> index++; >>>>> } >>>>> } >>>>> -- >>>>> 2.28.0 >>>>> >>>>> _______________________________________________ >>>>> libcamera-devel mailing list >>>>> libcamera-devel@lists.libcamera.org >>>>> https://lists.libcamera.org/listinfo/libcamera-devel >>> >>> -- >>> Regards, >>> Niklas Söderlund >
Hi Kieran, On 2020-08-06 15:35:34 +0100, Kieran Bingham wrote: > Hi Niklas, > > > On 06/08/2020 14:46, Niklas Söderlund wrote: > > Hi Jacopo, > > > > On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote: > >> Hi Niklas, > >> > >> On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: > >>> Hi Jacopo, > >>> > >>> Thanks for your feedback. > >>> > >>> On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > >>>> Hi Niklas, > >>>> > >>>> On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > >>>>> Instead of only printing the camera ID which is not intended for humans > >>>>> to read and parse create a more user friendly string when prating camera > >>>>> names. The ID is still printed as it is one option used to select camera > >>>>> using the --camera option. > >>>>> > >>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>>>> --- > >>>>> src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > >>>>> 1 file changed, 37 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp > >>>>> index cc3facd5a5b22092..5af76f6965ef2387 100644 > >>>>> --- a/src/cam/main.cpp > >>>>> +++ b/src/cam/main.cpp > >>>>> @@ -21,6 +21,39 @@ > >>>>> > >>>>> using namespace libcamera; > >>>>> > >>>>> +std::string cameraName(const Camera *camera) > >>>>> +{ > >>>>> + const ControlList &props = camera->properties(); > >>>>> + std::string name; > >>>>> + > >>>>> + if (props.contains(properties::Model)) > >>>>> + name += props.get(properties::Model) + " "; > >>>>> + > >>>>> + if (props.contains(properties::Location)) { > >>>>> + switch (props.get(properties::Location)) { > >>>>> + case properties::CameraLocationFront: > >>>>> + name += "facing front "; > >>>>> + break; > >>>>> + case properties::CameraLocationBack: > >>>>> + name += "facing back "; > >>>>> + break; > >>>>> + case properties::CameraLocationExternal: > >>>>> + name += "external "; > >>>>> + break; > >>>>> + } > >>>>> + } > >>>>> + > >>>>> + if (props.contains(properties::Rotation)) > >>>>> + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > >>>>> + > >>>>> + if (!name.empty()) > >>>>> + name += "with id "; > >>>> > >>>> Just a quick question while skimming through the series. cam can > >>>> printout camera properties, do we need to make 'friendly' names > >>>> 100 characters to repeat what's already available there ? > >>> > >>> I know it can print properties :-) > >>> > >>> I'm happy to change this to contain more or less information, my main > >>> goal of throwing in everything here is to showcase with cam how > >>> applications can create names. > >>> > >>> What properties would you like to see make up the user-friendly name? > >> > >> None if not useful to distinguish between cameras with the same name ? > >> > >> ie > >> 1- ov5670 (front) > >> 2- ov5670 (back) > >> > >> I know we could have > >> 1- ov5670 (external) > >> 2- ov5670 (external) > > > > I like this and will use it for next version. > > > > But I really think we need to print the id as well as it is one of two > > possible augments to --camera to select which one is used. How about > > > > 1: ov5695 (front) - /base/i2c@ff160000/camera@36 > > 2: ov2685 (back) - /base/i2c@ff160000/camera@3c > > I know we can reference cameras by index, but when referencing by name, > does this mean the user would have to type/paste the whole line? or will > it match on first unique match? > > cam -C -c "ov2685 (back)" > > is almost friendly enough to type ... > > cam -C -c "ov2685 (back) - /base/i2c@ff160000/camera@3c" is not > something anyone is going to be willing to type by hand. > > Maybe that's not a problem if it's not a design consideration anyway. No the two ways we can specify cameras today using the --camera option are not changed in by this series, so the options are by index or id. So to select the "ov2685 (back)" from the example above there are two options, $ cam --camera 2 ... $ cam --camera /base/i2c@ff160000/camera@3c ... Where doing it by index is not stable as if we plug in a third camera the ordering may change while using the id is guaranteed to be stable, even between reboots of the system. One of the ides of the id is that it could be save to persistent storage and reread later and expect to get the same camera. In my automated tests for example I use the id to make sure I always test the camera I expect to test. > > -- > Kieran > > > > > ? > > > >> > >> but at that point, I think we've done the best we could > > > >> > >>> > >>>> > >>>> > >>>>> + > >>>>> + name += camera->id(); > >>>>> + > >>>>> + return name; > >>>>> +} > >>>>> + > >>>>> class CamApp > >>>>> { > >>>>> public: > >>>>> @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > >>>>> return -EINVAL; > >>>>> } > >>>>> > >>>>> - std::cout << "Using camera " << camera_->id() << std::endl; > >>>>> + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > >>>>> > >>>>> ret = prepareConfig(); > >>>>> if (ret) { > >>>>> @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > >>>>> > >>>>> void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > >>>>> { > >>>>> - std::cout << "Camera Added: " << cam->id() << std::endl; > >>>>> + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > >>>>> } > >>>>> > >>>>> void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > >>>>> { > >>>>> - std::cout << "Camera Removed: " << cam->id() << std::endl; > >>>>> + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > >>>>> } > >>>>> > >>>>> int CamApp::run() > >>>>> @@ -340,7 +373,7 @@ int CamApp::run() > >>>>> > >>>>> unsigned int index = 1; > >>>>> for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > >>>>> - std::cout << index << ": " << cam->id() << std::endl; > >>>>> + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > >>>>> index++; > >>>>> } > >>>>> } > >>>>> -- > >>>>> 2.28.0 > >>>>> > >>>>> _______________________________________________ > >>>>> libcamera-devel mailing list > >>>>> libcamera-devel@lists.libcamera.org > >>>>> https://lists.libcamera.org/listinfo/libcamera-devel > >>> > >>> -- > >>> Regards, > >>> Niklas Söderlund > > > > -- > Regards > -- > Kieran
Hi Niklas, On Thu, Aug 06, 2020 at 03:46:01PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: > > > Hi Jacopo, > > > > > > Thanks for your feedback. > > > > > > On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > > > > Hi Niklas, > > > > > > > > On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > > > > > Instead of only printing the camera ID which is not intended for humans > > > > > to read and parse create a more user friendly string when prating camera > > > > > names. The ID is still printed as it is one option used to select camera > > > > > using the --camera option. > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > --- > > > > > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > > > > > 1 file changed, 37 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > > > index cc3facd5a5b22092..5af76f6965ef2387 100644 > > > > > --- a/src/cam/main.cpp > > > > > +++ b/src/cam/main.cpp > > > > > @@ -21,6 +21,39 @@ > > > > > > > > > > using namespace libcamera; > > > > > > > > > > +std::string cameraName(const Camera *camera) > > > > > +{ > > > > > + const ControlList &props = camera->properties(); > > > > > + std::string name; > > > > > + > > > > > + if (props.contains(properties::Model)) > > > > > + name += props.get(properties::Model) + " "; > > > > > + > > > > > + if (props.contains(properties::Location)) { > > > > > + switch (props.get(properties::Location)) { > > > > > + case properties::CameraLocationFront: > > > > > + name += "facing front "; > > > > > + break; > > > > > + case properties::CameraLocationBack: > > > > > + name += "facing back "; > > > > > + break; > > > > > + case properties::CameraLocationExternal: > > > > > + name += "external "; > > > > > + break; > > > > > + } > > > > > + } > > > > > + > > > > > + if (props.contains(properties::Rotation)) > > > > > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > > > > > + > > > > > + if (!name.empty()) > > > > > + name += "with id "; > > > > > > > > Just a quick question while skimming through the series. cam can > > > > printout camera properties, do we need to make 'friendly' names > > > > 100 characters to repeat what's already available there ? > > > > > > I know it can print properties :-) > > > > > > I'm happy to change this to contain more or less information, my main > > > goal of throwing in everything here is to showcase with cam how > > > applications can create names. > > > > > > What properties would you like to see make up the user-friendly name? > > > > None if not useful to distinguish between cameras with the same name ? > > > > ie > > 1- ov5670 (front) > > 2- ov5670 (back) > > > > I know we could have > > 1- ov5670 (external) > > 2- ov5670 (external) > > I like this and will use it for next version. > > But I really think we need to print the id as well as it is one of two > possible augments to --camera to select which one is used. How about > > 1: ov5695 (front) - /base/i2c@ff160000/camera@36 > 2: ov2685 (back) - /base/i2c@ff160000/camera@3c > > ? I now wonder if an application like cam should expose machine-friendly id at all. But I guess it's fine, I like the above proposal > > > > > but at that point, I think we've done the best we could > > > > > > > > > > > > > > > > > > > + > > > > > + name += camera->id(); > > > > > + > > > > > + return name; > > > > > +} > > > > > + > > > > > class CamApp > > > > > { > > > > > public: > > > > > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > > > > > return -EINVAL; > > > > > } > > > > > > > > > > - std::cout << "Using camera " << camera_->id() << std::endl; > > > > > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > > > > > > > > > ret = prepareConfig(); > > > > > if (ret) { > > > > > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > > > > > > > > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > > > > > { > > > > > - std::cout << "Camera Added: " << cam->id() << std::endl; > > > > > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > > > > > } > > > > > > > > > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > > > > > { > > > > > - std::cout << "Camera Removed: " << cam->id() << std::endl; > > > > > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > > > > > } > > > > > > > > > > int CamApp::run() > > > > > @@ -340,7 +373,7 @@ int CamApp::run() > > > > > > > > > > unsigned int index = 1; > > > > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > > > > - std::cout << index << ": " << cam->id() << std::endl; > > > > > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > > > > > index++; > > > > > } > > > > > } > > > > > -- > > > > > 2.28.0 > > > > > > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > -- > Regards, > Niklas Söderlund
Hello, On Thu, Aug 06, 2020 at 05:06:34PM +0200, Jacopo Mondi wrote: > On Thu, Aug 06, 2020 at 03:46:01PM +0200, Niklas Söderlund wrote: > > On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote: > > > On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: > > > > On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > > > > > On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > > > > > > Instead of only printing the camera ID which is not intended for humans > > > > > > to read and parse create a more user friendly string when prating camera > > > > > > names. The ID is still printed as it is one option used to select camera > > > > > > using the --camera option. > > > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > --- > > > > > > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > > > > > > 1 file changed, 37 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > > > > index cc3facd5a5b22092..5af76f6965ef2387 100644 > > > > > > --- a/src/cam/main.cpp > > > > > > +++ b/src/cam/main.cpp > > > > > > @@ -21,6 +21,39 @@ > > > > > > > > > > > > using namespace libcamera; > > > > > > > > > > > > +std::string cameraName(const Camera *camera) > > > > > > +{ > > > > > > + const ControlList &props = camera->properties(); > > > > > > + std::string name; > > > > > > + > > > > > > + if (props.contains(properties::Model)) > > > > > > + name += props.get(properties::Model) + " "; > > > > > > + > > > > > > + if (props.contains(properties::Location)) { > > > > > > + switch (props.get(properties::Location)) { > > > > > > + case properties::CameraLocationFront: > > > > > > + name += "facing front "; > > > > > > + break; > > > > > > + case properties::CameraLocationBack: > > > > > > + name += "facing back "; > > > > > > + break; > > > > > > + case properties::CameraLocationExternal: > > > > > > + name += "external "; > > > > > > + break; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + if (props.contains(properties::Rotation)) > > > > > > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > > > > > > + > > > > > > + if (!name.empty()) > > > > > > + name += "with id "; > > > > > > > > > > Just a quick question while skimming through the series. cam can > > > > > printout camera properties, do we need to make 'friendly' names > > > > > 100 characters to repeat what's already available there ? > > > > > > > > I know it can print properties :-) > > > > > > > > I'm happy to change this to contain more or less information, my main > > > > goal of throwing in everything here is to showcase with cam how > > > > applications can create names. > > > > > > > > What properties would you like to see make up the user-friendly name? > > > > > > None if not useful to distinguish between cameras with the same name ? > > > > > > ie > > > 1- ov5670 (front) > > > 2- ov5670 (back) > > > > > > I know we could have > > > 1- ov5670 (external) > > > 2- ov5670 (external) > > > > I like this and will use it for next version. > > > > But I really think we need to print the id as well as it is one of two > > possible augments to --camera to select which one is used. How about > > > > 1: ov5695 (front) - /base/i2c@ff160000/camera@36 > > 2: ov2685 (back) - /base/i2c@ff160000/camera@3c > > > > ? > > I now wonder if an application like cam should expose machine-friendly > id at all. But I guess it's fine, I like the above proposal I'm happy to join the bikeshedding :-) Jokes aside, I think this is important. Ideally, for internal cameras, I'd like to see something along the lines of 1: Internal front camera (/base/i2c@ff160000/camera@36) 2. Internal back camera (/base/i2c@ff160000/camera@3c) reported by cam, or possibly 1: Internal front 5MP camera (/base/i2c@ff160000/camera@36) 2. Internal back 12MP camera (/base/i2c@ff160000/camera@3c) if we want to make this more marketing-friendly :-) I know that currently we'll get "Internal front camera" twice, and I think that's OK. I believe the ID is important to print in cam, less so in qcam. For external camera, I've been dreaming of 3: Logitech Webcam C930e on USB port 2.2 (\_SB_.PCI0.RP05.PXSX-2.2:1.0-046d:0843) 4: Logitech Webcam C930e on USB port 2.4 (\_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843) but getting a sensible port number may be difficult (in theory we could get the physical location from ACPI tables, but that's really the theory). I think reporting extended physical location through properties would be useful, but it doesn't have to be part of this series. > > > but at that point, I think we've done the best we could > > > > > > > > > + > > > > > > + name += camera->id(); > > > > > > + > > > > > > + return name; > > > > > > +} > > > > > > + > > > > > > class CamApp > > > > > > { > > > > > > public: > > > > > > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > > > - std::cout << "Using camera " << camera_->id() << std::endl; > > > > > > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > > > > > > > > > > > ret = prepareConfig(); > > > > > > if (ret) { > > > > > > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > > > > > > > > > > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > > > > > > { > > > > > > - std::cout << "Camera Added: " << cam->id() << std::endl; > > > > > > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > > > > > > } > > > > > > > > > > > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > > > > > > { > > > > > > - std::cout << "Camera Removed: " << cam->id() << std::endl; > > > > > > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > > > > > > } > > > > > > > > > > > > int CamApp::run() > > > > > > @@ -340,7 +373,7 @@ int CamApp::run() > > > > > > > > > > > > unsigned int index = 1; > > > > > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > > > > > - std::cout << index << ": " << cam->id() << std::endl; > > > > > > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > > > > > > index++; > > > > > > } > > > > > > }
Hi Laurent, Thanks for your feedback. On 2020-08-08 15:10:34 +0300, Laurent Pinchart wrote: > Hello, > > On Thu, Aug 06, 2020 at 05:06:34PM +0200, Jacopo Mondi wrote: > > On Thu, Aug 06, 2020 at 03:46:01PM +0200, Niklas Söderlund wrote: > > > On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote: > > > > On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: > > > > > On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > > > > > > On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > > > > > > > Instead of only printing the camera ID which is not intended for humans > > > > > > > to read and parse create a more user friendly string when prating camera > > > > > > > names. The ID is still printed as it is one option used to select camera > > > > > > > using the --camera option. > > > > > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > --- > > > > > > > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > > > > > > > 1 file changed, 37 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > > > > > index cc3facd5a5b22092..5af76f6965ef2387 100644 > > > > > > > --- a/src/cam/main.cpp > > > > > > > +++ b/src/cam/main.cpp > > > > > > > @@ -21,6 +21,39 @@ > > > > > > > > > > > > > > using namespace libcamera; > > > > > > > > > > > > > > +std::string cameraName(const Camera *camera) > > > > > > > +{ > > > > > > > + const ControlList &props = camera->properties(); > > > > > > > + std::string name; > > > > > > > + > > > > > > > + if (props.contains(properties::Model)) > > > > > > > + name += props.get(properties::Model) + " "; > > > > > > > + > > > > > > > + if (props.contains(properties::Location)) { > > > > > > > + switch (props.get(properties::Location)) { > > > > > > > + case properties::CameraLocationFront: > > > > > > > + name += "facing front "; > > > > > > > + break; > > > > > > > + case properties::CameraLocationBack: > > > > > > > + name += "facing back "; > > > > > > > + break; > > > > > > > + case properties::CameraLocationExternal: > > > > > > > + name += "external "; > > > > > > > + break; > > > > > > > + } > > > > > > > + } > > > > > > > + > > > > > > > + if (props.contains(properties::Rotation)) > > > > > > > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > > > > > > > + > > > > > > > + if (!name.empty()) > > > > > > > + name += "with id "; > > > > > > > > > > > > Just a quick question while skimming through the series. cam can > > > > > > printout camera properties, do we need to make 'friendly' names > > > > > > 100 characters to repeat what's already available there ? > > > > > > > > > > I know it can print properties :-) > > > > > > > > > > I'm happy to change this to contain more or less information, my main > > > > > goal of throwing in everything here is to showcase with cam how > > > > > applications can create names. > > > > > > > > > > What properties would you like to see make up the user-friendly name? > > > > > > > > None if not useful to distinguish between cameras with the same name ? > > > > > > > > ie > > > > 1- ov5670 (front) > > > > 2- ov5670 (back) > > > > > > > > I know we could have > > > > 1- ov5670 (external) > > > > 2- ov5670 (external) > > > > > > I like this and will use it for next version. > > > > > > But I really think we need to print the id as well as it is one of two > > > possible augments to --camera to select which one is used. How about > > > > > > 1: ov5695 (front) - /base/i2c@ff160000/camera@36 > > > 2: ov2685 (back) - /base/i2c@ff160000/camera@3c > > > > > > ? > > > > I now wonder if an application like cam should expose machine-friendly > > id at all. But I guess it's fine, I like the above proposal > > I'm happy to join the bikeshedding :-) Jokes aside, I think this is > important. > > Ideally, for internal cameras, I'd like to see something along the lines > of > > 1: Internal front camera (/base/i2c@ff160000/camera@36) > 2. Internal back camera (/base/i2c@ff160000/camera@3c) > > reported by cam, or possibly > > 1: Internal front 5MP camera (/base/i2c@ff160000/camera@36) > 2. Internal back 12MP camera (/base/i2c@ff160000/camera@3c) > > if we want to make this more marketing-friendly :-) I know that > currently we'll get "Internal front camera" twice, and I think that's > OK. I believe the ID is important to print in cam, less so in qcam. > > For external camera, I've been dreaming of > > 3: Logitech Webcam C930e on USB port 2.2 (\_SB_.PCI0.RP05.PXSX-2.2:1.0-046d:0843) > 4: Logitech Webcam C930e on USB port 2.4 (\_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843) > > but getting a sensible port number may be difficult (in theory we could > get the physical location from ACPI tables, but that's really the > theory). I think reporting extended physical location through properties > would be useful, but it doesn't have to be part of this series. I like the above but to be able to implement all of it more properties needs to be added to lib camera core. As a first step to make cam user-friendly again would the following output be sufficient for this series in your opinion? 1: Internal front camera (/base/i2c@ff160000/camera@36) 2. Internal back camera (/base/i2c@ff160000/camera@3c) 3: Logitech Webcam C930e (\_SB_.PCI0.RP05.PXSX-2.2:1.0-046d:0843) As camera selection is only possible by `index` or `id` modifying the name as we gain more information will not break any scripts. > > > > > but at that point, I think we've done the best we could > > > > > > > > > > > + > > > > > > > + name += camera->id(); > > > > > > > + > > > > > > > + return name; > > > > > > > +} > > > > > > > + > > > > > > > class CamApp > > > > > > > { > > > > > > > public: > > > > > > > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > > > > > > > return -EINVAL; > > > > > > > } > > > > > > > > > > > > > > - std::cout << "Using camera " << camera_->id() << std::endl; > > > > > > > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > > > > > > > > > > > > > ret = prepareConfig(); > > > > > > > if (ret) { > > > > > > > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > > > > > > > > > > > > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > > > > > > > { > > > > > > > - std::cout << "Camera Added: " << cam->id() << std::endl; > > > > > > > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > > > > > > > } > > > > > > > > > > > > > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > > > > > > > { > > > > > > > - std::cout << "Camera Removed: " << cam->id() << std::endl; > > > > > > > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > > > > > > > } > > > > > > > > > > > > > > int CamApp::run() > > > > > > > @@ -340,7 +373,7 @@ int CamApp::run() > > > > > > > > > > > > > > unsigned int index = 1; > > > > > > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > > > > > > - std::cout << index << ": " << cam->id() << std::endl; > > > > > > > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > > > > > > > index++; > > > > > > > } > > > > > > > } > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Sat, Aug 08, 2020 at 02:22:35PM +0200, Niklas Söderlund wrote: > On 2020-08-08 15:10:34 +0300, Laurent Pinchart wrote: > > On Thu, Aug 06, 2020 at 05:06:34PM +0200, Jacopo Mondi wrote: > >> On Thu, Aug 06, 2020 at 03:46:01PM +0200, Niklas Söderlund wrote: > >>> On 2020-08-06 15:40:02 +0200, Jacopo Mondi wrote: > >>>> On Thu, Aug 06, 2020 at 03:32:49PM +0200, Niklas Söderlund wrote: > >>>>> On 2020-08-06 15:28:04 +0200, Jacopo Mondi wrote: > >>>>>> On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > >>>>>>> Instead of only printing the camera ID which is not intended for humans > >>>>>>> to read and parse create a more user friendly string when prating camera > >>>>>>> names. The ID is still printed as it is one option used to select camera > >>>>>>> using the --camera option. > >>>>>>> > >>>>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>>>>>> --- > >>>>>>> src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > >>>>>>> 1 file changed, 37 insertions(+), 4 deletions(-) > >>>>>>> > >>>>>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp > >>>>>>> index cc3facd5a5b22092..5af76f6965ef2387 100644 > >>>>>>> --- a/src/cam/main.cpp > >>>>>>> +++ b/src/cam/main.cpp > >>>>>>> @@ -21,6 +21,39 @@ > >>>>>>> > >>>>>>> using namespace libcamera; > >>>>>>> > >>>>>>> +std::string cameraName(const Camera *camera) > >>>>>>> +{ > >>>>>>> + const ControlList &props = camera->properties(); > >>>>>>> + std::string name; > >>>>>>> + > >>>>>>> + if (props.contains(properties::Model)) > >>>>>>> + name += props.get(properties::Model) + " "; > >>>>>>> + > >>>>>>> + if (props.contains(properties::Location)) { > >>>>>>> + switch (props.get(properties::Location)) { > >>>>>>> + case properties::CameraLocationFront: > >>>>>>> + name += "facing front "; > >>>>>>> + break; > >>>>>>> + case properties::CameraLocationBack: > >>>>>>> + name += "facing back "; > >>>>>>> + break; > >>>>>>> + case properties::CameraLocationExternal: > >>>>>>> + name += "external "; > >>>>>>> + break; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + > >>>>>>> + if (props.contains(properties::Rotation)) > >>>>>>> + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > >>>>>>> + > >>>>>>> + if (!name.empty()) > >>>>>>> + name += "with id "; > >>>>>> > >>>>>> Just a quick question while skimming through the series. cam can > >>>>>> printout camera properties, do we need to make 'friendly' names > >>>>>> 100 characters to repeat what's already available there ? > >>>>> > >>>>> I know it can print properties :-) > >>>>> > >>>>> I'm happy to change this to contain more or less information, my main > >>>>> goal of throwing in everything here is to showcase with cam how > >>>>> applications can create names. > >>>>> > >>>>> What properties would you like to see make up the user-friendly name? > >>>> > >>>> None if not useful to distinguish between cameras with the same name ? > >>>> > >>>> ie > >>>> 1- ov5670 (front) > >>>> 2- ov5670 (back) > >>>> > >>>> I know we could have > >>>> 1- ov5670 (external) > >>>> 2- ov5670 (external) > >>> > >>> I like this and will use it for next version. > >>> > >>> But I really think we need to print the id as well as it is one of two > >>> possible augments to --camera to select which one is used. How about > >>> > >>> 1: ov5695 (front) - /base/i2c@ff160000/camera@36 > >>> 2: ov2685 (back) - /base/i2c@ff160000/camera@3c > >>> > >>> ? > >> > >> I now wonder if an application like cam should expose machine-friendly > >> id at all. But I guess it's fine, I like the above proposal > > > > I'm happy to join the bikeshedding :-) Jokes aside, I think this is > > important. > > > > Ideally, for internal cameras, I'd like to see something along the lines > > of > > > > 1: Internal front camera (/base/i2c@ff160000/camera@36) > > 2. Internal back camera (/base/i2c@ff160000/camera@3c) > > > > reported by cam, or possibly > > > > 1: Internal front 5MP camera (/base/i2c@ff160000/camera@36) > > 2. Internal back 12MP camera (/base/i2c@ff160000/camera@3c) > > > > if we want to make this more marketing-friendly :-) I know that > > currently we'll get "Internal front camera" twice, and I think that's > > OK. I believe the ID is important to print in cam, less so in qcam. > > > > For external camera, I've been dreaming of > > > > 3: Logitech Webcam C930e on USB port 2.2 (\_SB_.PCI0.RP05.PXSX-2.2:1.0-046d:0843) > > 4: Logitech Webcam C930e on USB port 2.4 (\_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843) > > > > but getting a sensible port number may be difficult (in theory we could > > get the physical location from ACPI tables, but that's really the > > theory). I think reporting extended physical location through properties > > would be useful, but it doesn't have to be part of this series. > > I like the above but to be able to implement all of it more properties > needs to be added to lib camera core. As a first step to make cam > user-friendly again would the following output be sufficient for this > series in your opinion? > > 1: Internal front camera (/base/i2c@ff160000/camera@36) > 2. Internal back camera (/base/i2c@ff160000/camera@3c) > 3: Logitech Webcam C930e (\_SB_.PCI0.RP05.PXSX-2.2:1.0-046d:0843) Absolutely. I tried to convey that in my previous e-mail, we'll need to expose more information about camera location, and it's fine to start with what we have. > As camera selection is only possible by `index` or `id` modifying the > name as we gain more information will not break any scripts. > > >>>> but at that point, I think we've done the best we could > >>>> > >>>>>>> + > >>>>>>> + name += camera->id(); > >>>>>>> + > >>>>>>> + return name; > >>>>>>> +} > >>>>>>> + > >>>>>>> class CamApp > >>>>>>> { > >>>>>>> public: > >>>>>>> @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > >>>>>>> return -EINVAL; > >>>>>>> } > >>>>>>> > >>>>>>> - std::cout << "Using camera " << camera_->id() << std::endl; > >>>>>>> + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > >>>>>>> > >>>>>>> ret = prepareConfig(); > >>>>>>> if (ret) { > >>>>>>> @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > >>>>>>> > >>>>>>> void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > >>>>>>> { > >>>>>>> - std::cout << "Camera Added: " << cam->id() << std::endl; > >>>>>>> + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > >>>>>>> } > >>>>>>> > >>>>>>> void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > >>>>>>> { > >>>>>>> - std::cout << "Camera Removed: " << cam->id() << std::endl; > >>>>>>> + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > >>>>>>> } > >>>>>>> > >>>>>>> int CamApp::run() > >>>>>>> @@ -340,7 +373,7 @@ int CamApp::run() > >>>>>>> > >>>>>>> unsigned int index = 1; > >>>>>>> for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > >>>>>>> - std::cout << index << ": " << cam->id() << std::endl; > >>>>>>> + std::cout << index << ": " << cameraName(cam.get()) << std::endl; > >>>>>>> index++; > >>>>>>> } > >>>>>>> }
Hi Niklas, Thank you for the patch. On Thu, Aug 06, 2020 at 03:09:36PM +0200, Niklas Söderlund wrote: > Instead of only printing the camera ID which is not intended for humans > to read and parse create a more user friendly string when prating camera s/prating/printing/ > names. The ID is still printed as it is one option used to select camera > using the --camera option. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 4 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index cc3facd5a5b22092..5af76f6965ef2387 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -21,6 +21,39 @@ > > using namespace libcamera; > > +std::string cameraName(const Camera *camera) > +{ > + const ControlList &props = camera->properties(); > + std::string name; > + > + if (props.contains(properties::Model)) > + name += props.get(properties::Model) + " "; > + > + if (props.contains(properties::Location)) { > + switch (props.get(properties::Location)) { > + case properties::CameraLocationFront: > + name += "facing front "; > + break; > + case properties::CameraLocationBack: > + name += "facing back "; > + break; > + case properties::CameraLocationExternal: > + name += "external "; > + break; > + } > + } > + > + if (props.contains(properties::Rotation)) > + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; > + > + if (!name.empty()) > + name += "with id "; > + > + name += camera->id(); > + > + return name; > +} > + > class CamApp > { > public: > @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) > return -EINVAL; > } > > - std::cout << "Using camera " << camera_->id() << std::endl; > + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; > > ret = prepareConfig(); > if (ret) { > @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() > > void CamApp::cameraAdded(std::shared_ptr<Camera> cam) > { > - std::cout << "Camera Added: " << cam->id() << std::endl; > + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; > } > > void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) > { > - std::cout << "Camera Removed: " << cam->id() << std::endl; > + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; > } > > int CamApp::run() > @@ -340,7 +373,7 @@ int CamApp::run() > > unsigned int index = 1; > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > - std::cout << index << ": " << cam->id() << std::endl; > + std::cout << index << ": " << cameraName(cam.get()) << std::endl; Here I think it makes complete sense, but above, I'd keep using id(), as cam is a low-level test tool. It would be different with a more end-user-oriented application. > index++; > } > }
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index cc3facd5a5b22092..5af76f6965ef2387 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -21,6 +21,39 @@ using namespace libcamera; +std::string cameraName(const Camera *camera) +{ + const ControlList &props = camera->properties(); + std::string name; + + if (props.contains(properties::Model)) + name += props.get(properties::Model) + " "; + + if (props.contains(properties::Location)) { + switch (props.get(properties::Location)) { + case properties::CameraLocationFront: + name += "facing front "; + break; + case properties::CameraLocationBack: + name += "facing back "; + break; + case properties::CameraLocationExternal: + name += "external "; + break; + } + } + + if (props.contains(properties::Rotation)) + name += "rotated " + std::to_string(props.get(properties::Rotation)) + " degrees "; + + if (!name.empty()) + name += "with id "; + + name += camera->id(); + + return name; +} + class CamApp { public: @@ -117,7 +150,7 @@ int CamApp::init(int argc, char **argv) return -EINVAL; } - std::cout << "Using camera " << camera_->id() << std::endl; + std::cout << "Using camera " << cameraName(camera_.get()) << std::endl; ret = prepareConfig(); if (ret) { @@ -323,12 +356,12 @@ int CamApp::infoConfiguration() void CamApp::cameraAdded(std::shared_ptr<Camera> cam) { - std::cout << "Camera Added: " << cam->id() << std::endl; + std::cout << "Camera Added: " << cameraName(cam.get()) << std::endl; } void CamApp::cameraRemoved(std::shared_ptr<Camera> cam) { - std::cout << "Camera Removed: " << cam->id() << std::endl; + std::cout << "Camera Removed: " << cameraName(cam.get()) << std::endl; } int CamApp::run() @@ -340,7 +373,7 @@ int CamApp::run() unsigned int index = 1; for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { - std::cout << index << ": " << cam->id() << std::endl; + std::cout << index << ": " << cameraName(cam.get()) << std::endl; index++; } }
Instead of only printing the camera ID which is not intended for humans to read and parse create a more user friendly string when prating camera names. The ID is still printed as it is one option used to select camera using the --camera option. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/main.cpp | 41 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-)