| Message ID | 20210325134231.1400051-10-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Mar 25, 2021 at 01:42:27PM +0000, Kieran Bingham wrote: > The camera object has a state machine to ensure calls are only made > when in the correct state. It isn't easy to identify where things happen > when assertions fail so add extra information to make this clearer. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera.cpp | 51 +++++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 21 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index b86bff4745b2..336ab8695ab3 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -346,8 +346,9 @@ public: > const std::set<Stream *> &streams); > ~Private(); > > - int isAccessAllowed(State state, bool allowDisconnected = false) const; > - int isAccessAllowed(State low, State high, > + int isAccessAllowed(const char *from, State state, > + bool allowDisconnected = false) const; > + int isAccessAllowed(const char *from, State low, State high, > bool allowDisconnected = false) const; It's not very nice to have to pass the function name on every call :-S Is there any way we could improve this ? The option that first came to my mind was macros, which isn't really nice either, and then I came across this really really nice trick: int isAccessAllowed(State state, bool allowDisconnected = false, const char *from = __builtin_FUNCTION()) const; int isAccessAllowed(State low, State high, bool allowDisconnected = false, const char *from = __builtin_FUNCTION()) const; > > void disconnect(); > @@ -384,7 +385,8 @@ static const char *const camera_state_names[] = { > "Running", > }; > > -int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const > +int Camera::Private::isAccessAllowed(const char *from, State state, > + bool allowDisconnected) const > { > if (!allowDisconnected && disconnected_) > return -ENODEV; > @@ -395,14 +397,16 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const > > ASSERT(static_cast<unsigned int>(state) < std::size(camera_state_names)); > > - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] > - << " state trying operation requiring state " > - << camera_state_names[state]; > + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] For operations coming from applications, should this really be a warning ? I suppose it's good to warn them. Could you mention this change in the commit message ? > + << " state trying operation [" > + << from << "] requiring state " > + << camera_state_names[state]; > > return -EACCES; > } > > -int Camera::Private::isAccessAllowed(State low, State high, > +int Camera::Private::isAccessAllowed(const char *from, > + State low, State high, > bool allowDisconnected) const > { > if (!allowDisconnected && disconnected_) > @@ -415,10 +419,11 @@ int Camera::Private::isAccessAllowed(State low, State high, > ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && > static_cast<unsigned int>(high) < std::size(camera_state_names)); > > - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] > - << " state trying operation requiring state between " > - << camera_state_names[low] << " and " > - << camera_state_names[high]; > + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] > + << " state trying operation [" << from > + << "] requiring state between " > + << camera_state_names[low] << " and " > + << camera_state_names[high]; > > return -EACCES; > } > @@ -646,7 +651,7 @@ int Camera::exportFrameBuffers(Stream *stream, > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraConfigured); > + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); __func__ is standard (since C99), __FUNCTION__ is gcc-specific. OK, __builtin_FUNCTION() is also gcc-specific :-) I've tested it with gcc 7 to 10 and clang 7 to 11, it compiles on all of them. With this change, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > if (ret < 0) > return ret; > > @@ -693,7 +698,7 @@ int Camera::acquire() > * No manual locking is required as PipelineHandler::lock() is > * thread-safe. > */ > - int ret = d->isAccessAllowed(Private::CameraAvailable); > + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraAvailable); > if (ret < 0) > return ret == -EACCES ? -EBUSY : ret; > > @@ -726,7 +731,8 @@ int Camera::release() > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraAvailable, > + int ret = d->isAccessAllowed(__FUNCTION__, > + Private::CameraAvailable, > Private::CameraConfigured, true); > if (ret < 0) > return ret == -EACCES ? -EBUSY : ret; > @@ -805,7 +811,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraAvailable, > + int ret = d->isAccessAllowed(__FUNCTION__, > + Private::CameraAvailable, > Private::CameraRunning); > if (ret < 0) > return nullptr; > @@ -866,7 +873,8 @@ int Camera::configure(CameraConfiguration *config) > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraAcquired, > + int ret = d->isAccessAllowed(__FUNCTION__, > + Private::CameraAcquired, > Private::CameraConfigured); > if (ret < 0) > return ret; > @@ -938,7 +946,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraConfigured, > + int ret = d->isAccessAllowed(__FUNCTION__, > + Private::CameraConfigured, > Private::CameraRunning); > if (ret < 0) > return nullptr; > @@ -972,7 +981,7 @@ int Camera::queueRequest(Request *request) > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraRunning); > + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); > if (ret < 0) > return ret; > > @@ -1022,7 +1031,7 @@ int Camera::start(const ControlList *controls) > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraConfigured); > + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); > if (ret < 0) > return ret; > > @@ -1056,7 +1065,7 @@ int Camera::stop() > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraRunning); > + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); > if (ret < 0) > return ret; > > @@ -1082,7 +1091,7 @@ void Camera::requestComplete(Request *request) > Private *const d = LIBCAMERA_D_PTR(); > > /* Disconnected cameras are still able to complete requests. */ > - int ret = d->isAccessAllowed(Private::CameraRunning); > + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); > if (ret < 0 && ret != -ENODEV) > LOG(Camera, Fatal) << "Trying to complete a request when stopped"; >
Hi Laurent, On 26/03/2021 02:33, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Mar 25, 2021 at 01:42:27PM +0000, Kieran Bingham wrote: >> The camera object has a state machine to ensure calls are only made >> when in the correct state. It isn't easy to identify where things happen >> when assertions fail so add extra information to make this clearer. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/camera.cpp | 51 +++++++++++++++++++++++----------------- >> 1 file changed, 30 insertions(+), 21 deletions(-) >> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index b86bff4745b2..336ab8695ab3 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -346,8 +346,9 @@ public: >> const std::set<Stream *> &streams); >> ~Private(); >> >> - int isAccessAllowed(State state, bool allowDisconnected = false) const; >> - int isAccessAllowed(State low, State high, >> + int isAccessAllowed(const char *from, State state, >> + bool allowDisconnected = false) const; >> + int isAccessAllowed(const char *from, State low, State high, >> bool allowDisconnected = false) const; > > It's not very nice to have to pass the function name on every call :-S > Is there any way we could improve this ? > > The option that first came to my mind was macros, which isn't really > nice either, and then I came across this really really nice trick: Indeed, I was avoiding macros ... > > int isAccessAllowed(State state, bool allowDisconnected = false, > const char *from = __builtin_FUNCTION()) const; > int isAccessAllowed(State low, State high, > bool allowDisconnected = false, > const char *from = __builtin_FUNCTION()) const; > But this I like... trying it now ;-) >> >> void disconnect(); >> @@ -384,7 +385,8 @@ static const char *const camera_state_names[] = { >> "Running", >> }; >> >> -int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const >> +int Camera::Private::isAccessAllowed(const char *from, State state, >> + bool allowDisconnected) const >> { >> if (!allowDisconnected && disconnected_) >> return -ENODEV; >> @@ -395,14 +397,16 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const >> >> ASSERT(static_cast<unsigned int>(state) < std::size(camera_state_names)); >> >> - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] >> - << " state trying operation requiring state " >> - << camera_state_names[state]; >> + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] > > For operations coming from applications, should this really be a warning > ? I suppose it's good to warn them. Could you mention this change in the > commit message ? Yes, I believe applications should know if they have caused an action that was invalid. > >> + << " state trying operation [" >> + << from << "] requiring state " >> + << camera_state_names[state]; >> >> return -EACCES; >> } >> >> -int Camera::Private::isAccessAllowed(State low, State high, >> +int Camera::Private::isAccessAllowed(const char *from, >> + State low, State high, >> bool allowDisconnected) const >> { >> if (!allowDisconnected && disconnected_) >> @@ -415,10 +419,11 @@ int Camera::Private::isAccessAllowed(State low, State high, >> ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && >> static_cast<unsigned int>(high) < std::size(camera_state_names)); >> >> - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] >> - << " state trying operation requiring state between " >> - << camera_state_names[low] << " and " >> - << camera_state_names[high]; >> + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] >> + << " state trying operation [" << from >> + << "] requiring state between " >> + << camera_state_names[low] << " and " >> + << camera_state_names[high]; >> >> return -EACCES; >> } >> @@ -646,7 +651,7 @@ int Camera::exportFrameBuffers(Stream *stream, >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraConfigured); >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); > > __func__ is standard (since C99), __FUNCTION__ is gcc-specific. OK, > __builtin_FUNCTION() is also gcc-specific :-) I've tested it with gcc 7 > to 10 and clang 7 to 11, it compiles on all of them. > > With this change, with __func__ or __builtin_FUNCTION()? if __func__ is standard, should we use that as the default initialiser? ../src/libcamera/camera.cpp:353:27: error: ‘__func__’ is not defined outside of function scope [-Werror] 353 | const char *from = __func__) const; | ^~~~~~~~ Well that answers that then ;-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Works nicely, and now tells us what function caused the issue at least: (Ignore the fact that these are successful, I hacked out the actual conditional to check the output) > [113:13:27.373321847] [1457151] WARN Camera camera.cpp:401 Camera in Running state trying operation [queueRequest] requiring state Running > [113:13:27.408814517] [1457154] WARN Camera camera.cpp:401 Camera in Running state trying operation [requestComplete] requiring state Running It's a shame we don't report the line number of the invalid access as opposed to the line number of the isAccessAllowed() but I think we're ok there. The 'from' tells us how to get back to who made the call. > >> if (ret < 0) >> return ret; >> >> @@ -693,7 +698,7 @@ int Camera::acquire() >> * No manual locking is required as PipelineHandler::lock() is >> * thread-safe. >> */ >> - int ret = d->isAccessAllowed(Private::CameraAvailable); >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraAvailable); >> if (ret < 0) >> return ret == -EACCES ? -EBUSY : ret; >> >> @@ -726,7 +731,8 @@ int Camera::release() >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraAvailable, >> + int ret = d->isAccessAllowed(__FUNCTION__, >> + Private::CameraAvailable, >> Private::CameraConfigured, true); >> if (ret < 0) >> return ret == -EACCES ? -EBUSY : ret; >> @@ -805,7 +811,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraAvailable, >> + int ret = d->isAccessAllowed(__FUNCTION__, >> + Private::CameraAvailable, >> Private::CameraRunning); >> if (ret < 0) >> return nullptr; >> @@ -866,7 +873,8 @@ int Camera::configure(CameraConfiguration *config) >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraAcquired, >> + int ret = d->isAccessAllowed(__FUNCTION__, >> + Private::CameraAcquired, >> Private::CameraConfigured); >> if (ret < 0) >> return ret; >> @@ -938,7 +946,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraConfigured, >> + int ret = d->isAccessAllowed(__FUNCTION__, >> + Private::CameraConfigured, >> Private::CameraRunning); >> if (ret < 0) >> return nullptr; >> @@ -972,7 +981,7 @@ int Camera::queueRequest(Request *request) >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraRunning); >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); >> if (ret < 0) >> return ret; >> >> @@ -1022,7 +1031,7 @@ int Camera::start(const ControlList *controls) >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraConfigured); >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); >> if (ret < 0) >> return ret; >> >> @@ -1056,7 +1065,7 @@ int Camera::stop() >> { >> Private *const d = LIBCAMERA_D_PTR(); >> >> - int ret = d->isAccessAllowed(Private::CameraRunning); >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); >> if (ret < 0) >> return ret; >> >> @@ -1082,7 +1091,7 @@ void Camera::requestComplete(Request *request) >> Private *const d = LIBCAMERA_D_PTR(); >> >> /* Disconnected cameras are still able to complete requests. */ >> - int ret = d->isAccessAllowed(Private::CameraRunning); >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); >> if (ret < 0 && ret != -ENODEV) >> LOG(Camera, Fatal) << "Trying to complete a request when stopped"; >> >
Hi Kieran, On Fri, Mar 26, 2021 at 03:33:40PM +0000, Kieran Bingham wrote: > On 26/03/2021 02:33, Laurent Pinchart wrote: > > On Thu, Mar 25, 2021 at 01:42:27PM +0000, Kieran Bingham wrote: > >> The camera object has a state machine to ensure calls are only made > >> when in the correct state. It isn't easy to identify where things happen > >> when assertions fail so add extra information to make this clearer. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/camera.cpp | 51 +++++++++++++++++++++++----------------- > >> 1 file changed, 30 insertions(+), 21 deletions(-) > >> > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >> index b86bff4745b2..336ab8695ab3 100644 > >> --- a/src/libcamera/camera.cpp > >> +++ b/src/libcamera/camera.cpp > >> @@ -346,8 +346,9 @@ public: > >> const std::set<Stream *> &streams); > >> ~Private(); > >> > >> - int isAccessAllowed(State state, bool allowDisconnected = false) const; > >> - int isAccessAllowed(State low, State high, > >> + int isAccessAllowed(const char *from, State state, > >> + bool allowDisconnected = false) const; > >> + int isAccessAllowed(const char *from, State low, State high, > >> bool allowDisconnected = false) const; > > > > It's not very nice to have to pass the function name on every call :-S > > Is there any way we could improve this ? > > > > The option that first came to my mind was macros, which isn't really > > nice either, and then I came across this really really nice trick: > > Indeed, I was avoiding macros ... > > > > > int isAccessAllowed(State state, bool allowDisconnected = false, > > const char *from = __builtin_FUNCTION()) const; > > int isAccessAllowed(State low, State high, > > bool allowDisconnected = false, > > const char *from = __builtin_FUNCTION()) const; > > > > But this I like... trying it now ;-) > > > > >> > >> void disconnect(); > >> @@ -384,7 +385,8 @@ static const char *const camera_state_names[] = { > >> "Running", > >> }; > >> > >> -int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const > >> +int Camera::Private::isAccessAllowed(const char *from, State state, > >> + bool allowDisconnected) const > >> { > >> if (!allowDisconnected && disconnected_) > >> return -ENODEV; > >> @@ -395,14 +397,16 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const > >> > >> ASSERT(static_cast<unsigned int>(state) < std::size(camera_state_names)); > >> > >> - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] > >> - << " state trying operation requiring state " > >> - << camera_state_names[state]; > >> + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] > > > > For operations coming from applications, should this really be a warning > > ? I suppose it's good to warn them. Could you mention this change in the > > commit message ? > > Yes, I believe applications should know if they have caused an action > that was invalid. > > > > >> + << " state trying operation [" > >> + << from << "] requiring state " > >> + << camera_state_names[state]; > >> > >> return -EACCES; > >> } > >> > >> -int Camera::Private::isAccessAllowed(State low, State high, > >> +int Camera::Private::isAccessAllowed(const char *from, > >> + State low, State high, > >> bool allowDisconnected) const > >> { > >> if (!allowDisconnected && disconnected_) > >> @@ -415,10 +419,11 @@ int Camera::Private::isAccessAllowed(State low, State high, > >> ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && > >> static_cast<unsigned int>(high) < std::size(camera_state_names)); > >> > >> - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] > >> - << " state trying operation requiring state between " > >> - << camera_state_names[low] << " and " > >> - << camera_state_names[high]; > >> + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] > >> + << " state trying operation [" << from > >> + << "] requiring state between " > >> + << camera_state_names[low] << " and " > >> + << camera_state_names[high]; > >> > >> return -EACCES; > >> } > >> @@ -646,7 +651,7 @@ int Camera::exportFrameBuffers(Stream *stream, > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraConfigured); > >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); > > > > __func__ is standard (since C99), __FUNCTION__ is gcc-specific. OK, > > __builtin_FUNCTION() is also gcc-specific :-) I've tested it with gcc 7 > > to 10 and clang 7 to 11, it compiles on all of them. > > > > With this change, > > with __func__ or __builtin_FUNCTION()? > > if __func__ is standard, should we use that as the default initialiser? I'd love if we could. I prefer standard solutions, but __builtin_FUNCTION() is such a neat trick :-) > ../src/libcamera/camera.cpp:353:27: error: ‘__func__’ is not defined > outside of function scope [-Werror] > 353 | const char *from = __func__) const; > | ^~~~~~~~ > > Well that answers that then ;-) > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Works nicely, and now tells us what function caused the issue at least: > > (Ignore the fact that these are successful, I hacked out the actual > conditional to check the output) > > > [113:13:27.373321847] [1457151] WARN Camera camera.cpp:401 Camera in Running state trying operation [queueRequest] requiring state Running > > [113:13:27.408814517] [1457154] WARN Camera camera.cpp:401 Camera in Running state trying operation [requestComplete] requiring state Running > > It's a shame we don't report the line number of the invalid access as > opposed to the line number of the isAccessAllowed() but I think we're ok > there. The 'from' tells us how to get back to who made the call. Note that there's a __builtin_LINE() ;-) But I don't think we need to go that way, the name should be enough. Nitpicking, how about "trying queueRequest()" instead of "trying operation [queueRequest]" ? Up to you. > >> if (ret < 0) > >> return ret; > >> > >> @@ -693,7 +698,7 @@ int Camera::acquire() > >> * No manual locking is required as PipelineHandler::lock() is > >> * thread-safe. > >> */ > >> - int ret = d->isAccessAllowed(Private::CameraAvailable); > >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraAvailable); > >> if (ret < 0) > >> return ret == -EACCES ? -EBUSY : ret; > >> > >> @@ -726,7 +731,8 @@ int Camera::release() > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraAvailable, > >> + int ret = d->isAccessAllowed(__FUNCTION__, > >> + Private::CameraAvailable, > >> Private::CameraConfigured, true); > >> if (ret < 0) > >> return ret == -EACCES ? -EBUSY : ret; > >> @@ -805,7 +811,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraAvailable, > >> + int ret = d->isAccessAllowed(__FUNCTION__, > >> + Private::CameraAvailable, > >> Private::CameraRunning); > >> if (ret < 0) > >> return nullptr; > >> @@ -866,7 +873,8 @@ int Camera::configure(CameraConfiguration *config) > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraAcquired, > >> + int ret = d->isAccessAllowed(__FUNCTION__, > >> + Private::CameraAcquired, > >> Private::CameraConfigured); > >> if (ret < 0) > >> return ret; > >> @@ -938,7 +946,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraConfigured, > >> + int ret = d->isAccessAllowed(__FUNCTION__, > >> + Private::CameraConfigured, > >> Private::CameraRunning); > >> if (ret < 0) > >> return nullptr; > >> @@ -972,7 +981,7 @@ int Camera::queueRequest(Request *request) > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraRunning); > >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); > >> if (ret < 0) > >> return ret; > >> > >> @@ -1022,7 +1031,7 @@ int Camera::start(const ControlList *controls) > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraConfigured); > >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); > >> if (ret < 0) > >> return ret; > >> > >> @@ -1056,7 +1065,7 @@ int Camera::stop() > >> { > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> - int ret = d->isAccessAllowed(Private::CameraRunning); > >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); > >> if (ret < 0) > >> return ret; > >> > >> @@ -1082,7 +1091,7 @@ void Camera::requestComplete(Request *request) > >> Private *const d = LIBCAMERA_D_PTR(); > >> > >> /* Disconnected cameras are still able to complete requests. */ > >> - int ret = d->isAccessAllowed(Private::CameraRunning); > >> + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); > >> if (ret < 0 && ret != -ENODEV) > >> LOG(Camera, Fatal) << "Trying to complete a request when stopped"; > >>
On 26/03/2021 15:54, Laurent Pinchart wrote: > Note that there's a __builtin_LINE() ;-) But I don't think we need to go > that way, the name should be enough. > > Nitpicking, how about "trying queueRequest()" instead of "trying > operation [queueRequest]" ? Up to you. Looks good to me, updated.
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index b86bff4745b2..336ab8695ab3 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -346,8 +346,9 @@ public: const std::set<Stream *> &streams); ~Private(); - int isAccessAllowed(State state, bool allowDisconnected = false) const; - int isAccessAllowed(State low, State high, + int isAccessAllowed(const char *from, State state, + bool allowDisconnected = false) const; + int isAccessAllowed(const char *from, State low, State high, bool allowDisconnected = false) const; void disconnect(); @@ -384,7 +385,8 @@ static const char *const camera_state_names[] = { "Running", }; -int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const +int Camera::Private::isAccessAllowed(const char *from, State state, + bool allowDisconnected) const { if (!allowDisconnected && disconnected_) return -ENODEV; @@ -395,14 +397,16 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const ASSERT(static_cast<unsigned int>(state) < std::size(camera_state_names)); - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] - << " state trying operation requiring state " - << camera_state_names[state]; + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] + << " state trying operation [" + << from << "] requiring state " + << camera_state_names[state]; return -EACCES; } -int Camera::Private::isAccessAllowed(State low, State high, +int Camera::Private::isAccessAllowed(const char *from, + State low, State high, bool allowDisconnected) const { if (!allowDisconnected && disconnected_) @@ -415,10 +419,11 @@ int Camera::Private::isAccessAllowed(State low, State high, ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && static_cast<unsigned int>(high) < std::size(camera_state_names)); - LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] - << " state trying operation requiring state between " - << camera_state_names[low] << " and " - << camera_state_names[high]; + LOG(Camera, Warning) << "Camera in " << camera_state_names[currentState] + << " state trying operation [" << from + << "] requiring state between " + << camera_state_names[low] << " and " + << camera_state_names[high]; return -EACCES; } @@ -646,7 +651,7 @@ int Camera::exportFrameBuffers(Stream *stream, { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraConfigured); + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); if (ret < 0) return ret; @@ -693,7 +698,7 @@ int Camera::acquire() * No manual locking is required as PipelineHandler::lock() is * thread-safe. */ - int ret = d->isAccessAllowed(Private::CameraAvailable); + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraAvailable); if (ret < 0) return ret == -EACCES ? -EBUSY : ret; @@ -726,7 +731,8 @@ int Camera::release() { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraAvailable, + int ret = d->isAccessAllowed(__FUNCTION__, + Private::CameraAvailable, Private::CameraConfigured, true); if (ret < 0) return ret == -EACCES ? -EBUSY : ret; @@ -805,7 +811,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraAvailable, + int ret = d->isAccessAllowed(__FUNCTION__, + Private::CameraAvailable, Private::CameraRunning); if (ret < 0) return nullptr; @@ -866,7 +873,8 @@ int Camera::configure(CameraConfiguration *config) { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraAcquired, + int ret = d->isAccessAllowed(__FUNCTION__, + Private::CameraAcquired, Private::CameraConfigured); if (ret < 0) return ret; @@ -938,7 +946,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraConfigured, + int ret = d->isAccessAllowed(__FUNCTION__, + Private::CameraConfigured, Private::CameraRunning); if (ret < 0) return nullptr; @@ -972,7 +981,7 @@ int Camera::queueRequest(Request *request) { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraRunning); + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); if (ret < 0) return ret; @@ -1022,7 +1031,7 @@ int Camera::start(const ControlList *controls) { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraConfigured); + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraConfigured); if (ret < 0) return ret; @@ -1056,7 +1065,7 @@ int Camera::stop() { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraRunning); + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); if (ret < 0) return ret; @@ -1082,7 +1091,7 @@ void Camera::requestComplete(Request *request) Private *const d = LIBCAMERA_D_PTR(); /* Disconnected cameras are still able to complete requests. */ - int ret = d->isAccessAllowed(Private::CameraRunning); + int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning); if (ret < 0 && ret != -ENODEV) LOG(Camera, Fatal) << "Trying to complete a request when stopped";
The camera object has a state machine to ensure calls are only made when in the correct state. It isn't easy to identify where things happen when assertions fail so add extra information to make this clearer. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/camera.cpp | 51 +++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 21 deletions(-)