[libcamera-devel,RFC,v1,2/7] py: Add Python logging category
diff mbox series

Message ID 20220623144736.78537-3-tomi.valkeinen@ideasonboard.com
State Superseded
Headers show
Series
  • Python bindings event handling
Related show

Commit Message

Tomi Valkeinen June 23, 2022, 2:47 p.m. UTC 
Add Python logging category, and use it in handleRequestCompleted().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 src/py/libcamera/py_main.cpp |  8 ++++++--
 src/py/libcamera/py_main.h   | 12 ++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)
 create mode 100644 src/py/libcamera/py_main.h

Comments

Kieran Bingham June 24, 2022, 8:15 a.m. UTC | #1 
Quoting Tomi Valkeinen (2022-06-23 15:47:31)
> Add Python logging category, and use it in handleRequestCompleted().

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  src/py/libcamera/py_main.cpp |  8 ++++++--
>  src/py/libcamera/py_main.h   | 12 ++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>  create mode 100644 src/py/libcamera/py_main.h
> 
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 505cc3dc..17b17f60 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -5,6 +5,8 @@
>   * Python bindings
>   */
>  
> +#include "py_main.h"
> +
>  #include <mutex>
>  #include <stdexcept>
>  #include <sys/eventfd.h>
> @@ -23,6 +25,8 @@ namespace py = pybind11;
>  
>  using namespace libcamera;
>  
> +LOG_DEFINE_CATEGORY(Python)
> +
>  template<typename T>
>  static py::object valueOrTuple(const ControlValue &cv)
>  {
> @@ -120,10 +124,10 @@ static void handleRequestCompleted(Request *req)
>         size_t s = write(gEventfd, &v, 8);
>         /*
>          * We should never fail, and have no simple means to manage the error,
> -        * so let's use LOG(Fatal).
> +        * so let's use LOG(Python, Fatal).
>          */
>         if (s != 8)
> -               LOG(Fatal) << "Unable to write to eventfd";
> +               LOG(Python, Fatal) << "Unable to write to eventfd";
>  }
>  
>  void init_py_enums(py::module &m);
> diff --git a/src/py/libcamera/py_main.h b/src/py/libcamera/py_main.h
> new file mode 100644
> index 00000000..1b543a55
> --- /dev/null
> +++ b/src/py/libcamera/py_main.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + */
> +
> +#pragma once
> +
> +#include <libcamera/base/log.h>
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(Python)
> -- 
> 2.34.1
>
Laurent Pinchart June 24, 2022, 9:50 a.m. UTC | #2 
Hi Tomi,

Thank you for the patch.

On Thu, Jun 23, 2022 at 05:47:31PM +0300, Tomi Valkeinen wrote:
> Add Python logging category, and use it in handleRequestCompleted().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  src/py/libcamera/py_main.cpp |  8 ++++++--
>  src/py/libcamera/py_main.h   | 12 ++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>  create mode 100644 src/py/libcamera/py_main.h
> 
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 505cc3dc..17b17f60 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -5,6 +5,8 @@
>   * Python bindings
>   */
>  
> +#include "py_main.h"
> +
>  #include <mutex>
>  #include <stdexcept>
>  #include <sys/eventfd.h>
> @@ -23,6 +25,8 @@ namespace py = pybind11;
>  
>  using namespace libcamera;
>  
> +LOG_DEFINE_CATEGORY(Python)
> +
>  template<typename T>
>  static py::object valueOrTuple(const ControlValue &cv)
>  {
> @@ -120,10 +124,10 @@ static void handleRequestCompleted(Request *req)
>  	size_t s = write(gEventfd, &v, 8);
>  	/*
>  	 * We should never fail, and have no simple means to manage the error,
> -	 * so let's use LOG(Fatal).
> +	 * so let's use LOG(Python, Fatal).

You could also write "so let's log a fatal error".

>  	 */
>  	if (s != 8)
> -		LOG(Fatal) << "Unable to write to eventfd";
> +		LOG(Python, Fatal) << "Unable to write to eventfd";
>  }
>  
>  void init_py_enums(py::module &m);
> diff --git a/src/py/libcamera/py_main.h b/src/py/libcamera/py_main.h
> new file mode 100644
> index 00000000..1b543a55
> --- /dev/null
> +++ b/src/py/libcamera/py_main.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + */
> +
> +#pragma once
> +
> +#include <libcamera/base/log.h>
> +
> +using namespace libcamera;

"using namespace" directives are discouraged in header files, as they
will propagate to all compilation units. Could this be avoided ? With
that fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +LOG_DECLARE_CATEGORY(Python)
Tomi Valkeinen June 27, 2022, 6:48 a.m. UTC | #3 
On 24/06/2022 12:50, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Jun 23, 2022 at 05:47:31PM +0300, Tomi Valkeinen wrote:
>> Add Python logging category, and use it in handleRequestCompleted().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   src/py/libcamera/py_main.cpp |  8 ++++++--
>>   src/py/libcamera/py_main.h   | 12 ++++++++++++
>>   2 files changed, 18 insertions(+), 2 deletions(-)
>>   create mode 100644 src/py/libcamera/py_main.h
>>
>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>> index 505cc3dc..17b17f60 100644
>> --- a/src/py/libcamera/py_main.cpp
>> +++ b/src/py/libcamera/py_main.cpp
>> @@ -5,6 +5,8 @@
>>    * Python bindings
>>    */
>>   
>> +#include "py_main.h"
>> +
>>   #include <mutex>
>>   #include <stdexcept>
>>   #include <sys/eventfd.h>
>> @@ -23,6 +25,8 @@ namespace py = pybind11;
>>   
>>   using namespace libcamera;
>>   
>> +LOG_DEFINE_CATEGORY(Python)
>> +
>>   template<typename T>
>>   static py::object valueOrTuple(const ControlValue &cv)
>>   {
>> @@ -120,10 +124,10 @@ static void handleRequestCompleted(Request *req)
>>   	size_t s = write(gEventfd, &v, 8);
>>   	/*
>>   	 * We should never fail, and have no simple means to manage the error,
>> -	 * so let's use LOG(Fatal).
>> +	 * so let's use LOG(Python, Fatal).
> 
> You could also write "so let's log a fatal error".

Yep.

>>   	 */
>>   	if (s != 8)
>> -		LOG(Fatal) << "Unable to write to eventfd";
>> +		LOG(Python, Fatal) << "Unable to write to eventfd";
>>   }
>>   
>>   void init_py_enums(py::module &m);
>> diff --git a/src/py/libcamera/py_main.h b/src/py/libcamera/py_main.h
>> new file mode 100644
>> index 00000000..1b543a55
>> --- /dev/null
>> +++ b/src/py/libcamera/py_main.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> + */
>> +
>> +#pragma once
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +using namespace libcamera;
> 
> "using namespace" directives are discouraged in header files, as they
> will propagate to all compilation units. Could this be avoided ? With
> that fixed,

LOG_DECLARE_CATEGORY() requires either using namespace or being inside 
libcamera namespace.

I guess I could move all the python bindings code to be inside libcamera 
namespace. Then again, these are bindings internal headers and as such 
the using namespace should cause no issues.

  Tomi
Laurent Pinchart June 27, 2022, 8:18 a.m. UTC | #4 
Hi Tomi,

On Mon, Jun 27, 2022 at 09:48:34AM +0300, Tomi Valkeinen wrote:
> On 24/06/2022 12:50, Laurent Pinchart wrote:
> > On Thu, Jun 23, 2022 at 05:47:31PM +0300, Tomi Valkeinen wrote:
> >> Add Python logging category, and use it in handleRequestCompleted().
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   src/py/libcamera/py_main.cpp |  8 ++++++--
> >>   src/py/libcamera/py_main.h   | 12 ++++++++++++
> >>   2 files changed, 18 insertions(+), 2 deletions(-)
> >>   create mode 100644 src/py/libcamera/py_main.h
> >>
> >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> >> index 505cc3dc..17b17f60 100644
> >> --- a/src/py/libcamera/py_main.cpp
> >> +++ b/src/py/libcamera/py_main.cpp
> >> @@ -5,6 +5,8 @@
> >>    * Python bindings
> >>    */
> >>   
> >> +#include "py_main.h"
> >> +
> >>   #include <mutex>
> >>   #include <stdexcept>
> >>   #include <sys/eventfd.h>
> >> @@ -23,6 +25,8 @@ namespace py = pybind11;
> >>   
> >>   using namespace libcamera;
> >>   
> >> +LOG_DEFINE_CATEGORY(Python)
> >> +
> >>   template<typename T>
> >>   static py::object valueOrTuple(const ControlValue &cv)
> >>   {
> >> @@ -120,10 +124,10 @@ static void handleRequestCompleted(Request *req)
> >>   	size_t s = write(gEventfd, &v, 8);
> >>   	/*
> >>   	 * We should never fail, and have no simple means to manage the error,
> >> -	 * so let's use LOG(Fatal).
> >> +	 * so let's use LOG(Python, Fatal).
> > 
> > You could also write "so let's log a fatal error".
> 
> Yep.
> 
> >>   	 */
> >>   	if (s != 8)
> >> -		LOG(Fatal) << "Unable to write to eventfd";
> >> +		LOG(Python, Fatal) << "Unable to write to eventfd";
> >>   }
> >>   
> >>   void init_py_enums(py::module &m);
> >> diff --git a/src/py/libcamera/py_main.h b/src/py/libcamera/py_main.h
> >> new file mode 100644
> >> index 00000000..1b543a55
> >> --- /dev/null
> >> +++ b/src/py/libcamera/py_main.h
> >> @@ -0,0 +1,12 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +using namespace libcamera;
> > 
> > "using namespace" directives are discouraged in header files, as they
> > will propagate to all compilation units. Could this be avoided ? With
> > that fixed,
> 
> LOG_DECLARE_CATEGORY() requires either using namespace or being inside 
> libcamera namespace.
> 
> I guess I could move all the python bindings code to be inside libcamera 
> namespace.

You can also do

namespace libcamera {

LOG_DECLARE_CATEGORY(Python)

}

> Then again, these are bindings internal headers and as such 
> the using namespace should cause no issues.

That's true, but it's still not a great practice, and if it's easy to
fix, it could prevent problems later (and it's also nice not to give any
excuse for cargo-cult misuse of "using namespace" directives :-)).
Tomi Valkeinen June 27, 2022, 12:14 p.m. UTC | #5 
On 27/06/2022 11:18, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Jun 27, 2022 at 09:48:34AM +0300, Tomi Valkeinen wrote:
>> On 24/06/2022 12:50, Laurent Pinchart wrote:
>>> On Thu, Jun 23, 2022 at 05:47:31PM +0300, Tomi Valkeinen wrote:
>>>> Add Python logging category, and use it in handleRequestCompleted().
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    src/py/libcamera/py_main.cpp |  8 ++++++--
>>>>    src/py/libcamera/py_main.h   | 12 ++++++++++++
>>>>    2 files changed, 18 insertions(+), 2 deletions(-)
>>>>    create mode 100644 src/py/libcamera/py_main.h
>>>>
>>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>>>> index 505cc3dc..17b17f60 100644
>>>> --- a/src/py/libcamera/py_main.cpp
>>>> +++ b/src/py/libcamera/py_main.cpp
>>>> @@ -5,6 +5,8 @@
>>>>     * Python bindings
>>>>     */
>>>>    
>>>> +#include "py_main.h"
>>>> +
>>>>    #include <mutex>
>>>>    #include <stdexcept>
>>>>    #include <sys/eventfd.h>
>>>> @@ -23,6 +25,8 @@ namespace py = pybind11;
>>>>    
>>>>    using namespace libcamera;
>>>>    
>>>> +LOG_DEFINE_CATEGORY(Python)
>>>> +
>>>>    template<typename T>
>>>>    static py::object valueOrTuple(const ControlValue &cv)
>>>>    {
>>>> @@ -120,10 +124,10 @@ static void handleRequestCompleted(Request *req)
>>>>    	size_t s = write(gEventfd, &v, 8);
>>>>    	/*
>>>>    	 * We should never fail, and have no simple means to manage the error,
>>>> -	 * so let's use LOG(Fatal).
>>>> +	 * so let's use LOG(Python, Fatal).
>>>
>>> You could also write "so let's log a fatal error".
>>
>> Yep.
>>
>>>>    	 */
>>>>    	if (s != 8)
>>>> -		LOG(Fatal) << "Unable to write to eventfd";
>>>> +		LOG(Python, Fatal) << "Unable to write to eventfd";
>>>>    }
>>>>    
>>>>    void init_py_enums(py::module &m);
>>>> diff --git a/src/py/libcamera/py_main.h b/src/py/libcamera/py_main.h
>>>> new file mode 100644
>>>> index 00000000..1b543a55
>>>> --- /dev/null
>>>> +++ b/src/py/libcamera/py_main.h
>>>> @@ -0,0 +1,12 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> + */
>>>> +
>>>> +#pragma once
>>>> +
>>>> +#include <libcamera/base/log.h>
>>>> +
>>>> +using namespace libcamera;
>>>
>>> "using namespace" directives are discouraged in header files, as they
>>> will propagate to all compilation units. Could this be avoided ? With
>>> that fixed,
>>
>> LOG_DECLARE_CATEGORY() requires either using namespace or being inside
>> libcamera namespace.
>>
>> I guess I could move all the python bindings code to be inside libcamera
>> namespace.
> 
> You can also do
> 
> namespace libcamera {
> 
> LOG_DECLARE_CATEGORY(Python)
> 
> }

That's true. I'll change this as above, and maybe later move the whole 
bindings inside namespace libcamera. Or any concerns with that change?

  Tomi

Patch
diff mbox series 

diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
index 505cc3dc..17b17f60 100644
--- a/src/py/libcamera/py_main.cpp
+++ b/src/py/libcamera/py_main.cpp
@@ -5,6 +5,8 @@ 
  * Python bindings
  */
 
+#include "py_main.h"
+
 #include <mutex>
 #include <stdexcept>
 #include <sys/eventfd.h>
@@ -23,6 +25,8 @@  namespace py = pybind11;
 
 using namespace libcamera;
 
+LOG_DEFINE_CATEGORY(Python)
+
 template<typename T>
 static py::object valueOrTuple(const ControlValue &cv)
 {
@@ -120,10 +124,10 @@  static void handleRequestCompleted(Request *req)
 	size_t s = write(gEventfd, &v, 8);
 	/*
 	 * We should never fail, and have no simple means to manage the error,
-	 * so let's use LOG(Fatal).
+	 * so let's use LOG(Python, Fatal).
 	 */
 	if (s != 8)
-		LOG(Fatal) << "Unable to write to eventfd";
+		LOG(Python, Fatal) << "Unable to write to eventfd";
 }
 
 void init_py_enums(py::module &m);
diff --git a/src/py/libcamera/py_main.h b/src/py/libcamera/py_main.h
new file mode 100644
index 00000000..1b543a55
--- /dev/null
+++ b/src/py/libcamera/py_main.h
@@ -0,0 +1,12 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
+ */
+
+#pragma once
+
+#include <libcamera/base/log.h>
+
+using namespace libcamera;
+
+LOG_DECLARE_CATEGORY(Python)