| Message ID | 20190116135949.2097-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-01-16 14:59:45 +0100, Jacopo Mondi wrote: > Make the list-cameras test a little more verbose to better describe > failures. While at there use the Test class defined TestStatus value as > test exit codes, and skip the test if no camera gets registred. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > test/list-cameras.cpp | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp > index e2026c9..c9dc199 100644 > --- a/test/list-cameras.cpp > +++ b/test/list-cameras.cpp > @@ -7,6 +7,7 @@ > > #include <iostream> > > +#include <libcamera/camera.h> > #include <libcamera/camera_manager.h> > > #include "test.h" > @@ -14,27 +15,55 @@ > using namespace std; > using namespace libcamera; > > +/* > + * List all cameras registered in the system, using the CameraManager. > + * > + * In order for the test to run successfully, a pipeline handler supporting > + * the current test platform should be available in the library. > + * Libcamera provides a platform-agnostic pipeline handler for the 'vimc' > + * virtual media device, which can be used for testing purposes. > + * > + * The test tries to list all cameras registered in the system, if no > + * camera is found the test is skipped. If the test gets skipped on a > + * platform where a pipeline handler is known to be available, an error > + * in camera enumeration might get unnoticed. s/get unnoticed/go unnoticed/ With this fix Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Nice update of the TC! > + */ > class ListTest : public Test > { > protected: > int init() > { > cm = CameraManager::instance(); > - cm->start(); > + > + int ret = cm->start(); > + if (ret) { > + cerr << "Failed to start the CameraManager" << endl; > + return TestFail; > + } > > return 0; > } > > int run() > { > - unsigned int count = 0; > + vector<string> cameraList = cm->list(); > + if (cameraList.empty()) { > + cerr << "No cameras registered in the system: test skip" << endl > + << "This might be expected if no pipeline handler supports the testing platform" << endl; > + return TestSkip; > + } > + > + for (auto name : cameraList) { > + Camera *cam = cm->get(name); > + if (!cam) { > + cerr << "Failed to get camera '" << name << "' by name" << endl; > + return TestFail; > + } > > - for (auto name : cm->list()) { > - cout << "- " << name << endl; > - count++; > + cout << "Found camera '" << cam->name() << "'" << endl; > } > > - return count ? 0 : -ENODEV; > + return TestPass; > } > > void cleanup() > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Wed, Jan 16, 2019 at 02:59:45PM +0100, Jacopo Mondi wrote: > Make the list-cameras test a little more verbose to better describe > failures. While at there use the Test class defined TestStatus value as > test exit codes, and skip the test if no camera gets registred. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > test/list-cameras.cpp | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp > index e2026c9..c9dc199 100644 > --- a/test/list-cameras.cpp > +++ b/test/list-cameras.cpp > @@ -7,6 +7,7 @@ > > #include <iostream> > > +#include <libcamera/camera.h> > #include <libcamera/camera_manager.h> > > #include "test.h" > @@ -14,27 +15,55 @@ > using namespace std; > using namespace libcamera; > > +/* > + * List all cameras registered in the system, using the CameraManager. > + * > + * In order for the test to run successfully, a pipeline handler supporting > + * the current test platform should be available in the library. > + * Libcamera provides a platform-agnostic pipeline handler for the 'vimc' > + * virtual media device, which can be used for testing purposes. > + * > + * The test tries to list all cameras registered in the system, if no > + * camera is found the test is skipped. If the test gets skipped on a > + * platform where a pipeline handler is known to be available, an error > + * in camera enumeration might get unnoticed. > + */ > class ListTest : public Test > { > protected: > int init() > { > cm = CameraManager::instance(); > - cm->start(); > + > + int ret = cm->start(); > + if (ret) { > + cerr << "Failed to start the CameraManager" << endl; > + return TestFail; > + } > > return 0; > } > > int run() > { > - unsigned int count = 0; > + vector<string> cameraList = cm->list(); > + if (cameraList.empty()) { > + cerr << "No cameras registered in the system: test skip" << endl > + << "This might be expected if no pipeline handler supports the testing platform" << endl; > + return TestSkip; As commented before, I have some doubts on the value of this test, especially aftere rebasing it on the lifetime management series, as it will then only return TestSkip or TestPass. A test that can't fail has limited use :-) I think listing cameras should be part of a libcamera command line tool, not a test case. I wont block this patch, but please be aware that I plan to remove the list-cameras test. > + } > + > + for (auto name : cameraList) { > + Camera *cam = cm->get(name); > + if (!cam) { > + cerr << "Failed to get camera '" << name << "' by name" << endl; > + return TestFail; > + } > > - for (auto name : cm->list()) { > - cout << "- " << name << endl; > - count++; > + cout << "Found camera '" << cam->name() << "'" << endl; > } > > - return count ? 0 : -ENODEV; > + return TestPass; > } > > void cleanup()
Hi Laurent, On Mon, Jan 21, 2019 at 11:09:56AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Jan 16, 2019 at 02:59:45PM +0100, Jacopo Mondi wrote: > > Make the list-cameras test a little more verbose to better describe > > failures. While at there use the Test class defined TestStatus value as > > test exit codes, and skip the test if no camera gets registred. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > test/list-cameras.cpp | 41 +++++++++++++++++++++++++++++++++++------ > > 1 file changed, 35 insertions(+), 6 deletions(-) > > > > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp > > index e2026c9..c9dc199 100644 > > --- a/test/list-cameras.cpp > > +++ b/test/list-cameras.cpp > > @@ -7,6 +7,7 @@ > > > > #include <iostream> > > > > +#include <libcamera/camera.h> > > #include <libcamera/camera_manager.h> > > > > #include "test.h" > > @@ -14,27 +15,55 @@ > > using namespace std; > > using namespace libcamera; > > > > +/* > > + * List all cameras registered in the system, using the CameraManager. > > + * > > + * In order for the test to run successfully, a pipeline handler supporting > > + * the current test platform should be available in the library. > > + * Libcamera provides a platform-agnostic pipeline handler for the 'vimc' > > + * virtual media device, which can be used for testing purposes. > > + * > > + * The test tries to list all cameras registered in the system, if no > > + * camera is found the test is skipped. If the test gets skipped on a > > + * platform where a pipeline handler is known to be available, an error > > + * in camera enumeration might get unnoticed. > > + */ > > class ListTest : public Test > > { > > protected: > > int init() > > { > > cm = CameraManager::instance(); > > - cm->start(); > > + > > + int ret = cm->start(); > > + if (ret) { > > + cerr << "Failed to start the CameraManager" << endl; > > + return TestFail; > > + } > > > > return 0; > > } > > > > int run() > > { > > - unsigned int count = 0; > > + vector<string> cameraList = cm->list(); > > + if (cameraList.empty()) { > > + cerr << "No cameras registered in the system: test skip" << endl > > + << "This might be expected if no pipeline handler supports the testing platform" << endl; > > + return TestSkip; > > As commented before, I have some doubts on the value of this test, > especially aftere rebasing it on the lifetime management series, as it > will then only return TestSkip or TestPass. A test that can't fail has > limited use :-) I think listing cameras should be part of a libcamera > command line tool, not a test case. > > I wont block this patch, but please be aware that I plan to remove the > list-cameras test. > This was my understanding as well, that's why I have sent an IPU3 pipeline specific test. I'm fine with this being turned into a utility, and encourage pipeline specific tests to verify enumeration and matching works as expected. Thanks j > > + } > > + > > + for (auto name : cameraList) { > > + Camera *cam = cm->get(name); > > + if (!cam) { > > + cerr << "Failed to get camera '" << name << "' by name" << endl; > > + return TestFail; > > + } > > > > - for (auto name : cm->list()) { > > - cout << "- " << name << endl; > > - count++; > > + cout << "Found camera '" << cam->name() << "'" << endl; > > } > > > > - return count ? 0 : -ENODEV; > > + return TestPass; > > } > > > > void cleanup() > > -- > Regards, > > Laurent Pinchart
diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp index e2026c9..c9dc199 100644 --- a/test/list-cameras.cpp +++ b/test/list-cameras.cpp @@ -7,6 +7,7 @@ #include <iostream> +#include <libcamera/camera.h> #include <libcamera/camera_manager.h> #include "test.h" @@ -14,27 +15,55 @@ using namespace std; using namespace libcamera; +/* + * List all cameras registered in the system, using the CameraManager. + * + * In order for the test to run successfully, a pipeline handler supporting + * the current test platform should be available in the library. + * Libcamera provides a platform-agnostic pipeline handler for the 'vimc' + * virtual media device, which can be used for testing purposes. + * + * The test tries to list all cameras registered in the system, if no + * camera is found the test is skipped. If the test gets skipped on a + * platform where a pipeline handler is known to be available, an error + * in camera enumeration might get unnoticed. + */ class ListTest : public Test { protected: int init() { cm = CameraManager::instance(); - cm->start(); + + int ret = cm->start(); + if (ret) { + cerr << "Failed to start the CameraManager" << endl; + return TestFail; + } return 0; } int run() { - unsigned int count = 0; + vector<string> cameraList = cm->list(); + if (cameraList.empty()) { + cerr << "No cameras registered in the system: test skip" << endl + << "This might be expected if no pipeline handler supports the testing platform" << endl; + return TestSkip; + } + + for (auto name : cameraList) { + Camera *cam = cm->get(name); + if (!cam) { + cerr << "Failed to get camera '" << name << "' by name" << endl; + return TestFail; + } - for (auto name : cm->list()) { - cout << "- " << name << endl; - count++; + cout << "Found camera '" << cam->name() << "'" << endl; } - return count ? 0 : -ENODEV; + return TestPass; } void cleanup()
Make the list-cameras test a little more verbose to better describe failures. While at there use the Test class defined TestStatus value as test exit codes, and skip the test if no camera gets registred. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- test/list-cameras.cpp | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)