[libcamera-devel] [PATCH 0/5] Remove Raspberry Pi logging

[David Plowman]

Hi Laurent

Thanks for the review of all these patches.

I guess I was just swapping out the macros, but I think your
observations are fair enough:

* Debug that is just tracing can probably just be removed. It is
inherited from the platform where this code originally came from, and
back then it was all new and I probably liked the reassurance about
what was happening, especially as we were just processing single
images.

* Things like hierarchical categories sound nice, but I'm not sure
it's that important for us. The most likely use case for us will be
when some particular algorithm (e.g. AWB) is mis-behaviing and we want
to see what that single algorithm is doing.

* Yes, there seem to be some messages that are warnings but were not
categorised initially with RPI_WARN. Not sure why, but they probably
should be changed.

As regards making these changes, would you fold them into the existing
commits, or would you rather have them as additional changes (i.e.
keep the macro-swapping separate from anything else)? For myself, I
don't mind either way!

Let me know what you think, and I can do a v2 set accordingly.

Thanks again
David

On Sat, 23 Jan 2021 at 11:16, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David and Kieran,
>
> On Fri, Jan 22, 2021 at 11:31:58AM +0000, Kieran Bingham wrote:
> > On 22/01/2021 10:22, David Plowman wrote:
> > > This patch set removes the old Raspberry Pi logging from all our
> > > control algorithms and replaces it with libcamera logging. There is
> > > literally nothing in this patch set except for the necessary macro
> > > replacements and a few whitespace adjustments to keep the style
> > > checker happy.
> > >
> > > This is actually quite an important change because, now that we're
> > > about to publish libcamera versions of our legacy applications, it
> > > makes it much easier to get debug information from our customers.
> >
> > Aha, great, indeed it will tie into the existing logging infrastructure
> > nicely.
> >
> > > Perhaps the biggest question is whether to squish all the patches
> > > together? I've not done this yet as it's easier to squish than to
> > > un-squish, though I did roll up all the "minor" algorithms into a
> > > single commit. But I'll happily do that if it's tidier!
> >
> > I don't think that matters too much here. Indeed we could squash 1-4, or
> > split 4/5 further - but I don't think any of that effort is required here.
>
> Agreed, we can keep it as-is. It's certainly easier to review with 5
> patches than with a single one.
>
> > It's interesting that you can now enabled/disable specific algorithm
> > debug explictily. I wonder if sometime later we might want to be able to
> > 'group' multiple debug categories to enable all IPA debug without
> > enabling V4L2 debug for instance, or perhaps to have a way to 'disable'
> > some categories.
>
> We can use a wildcard * at the end of a category name, so the feature is
> already here.
>
> > But that's speculation on future features - for this whole series
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > Thanks and best regards
> > > David
> > >
> > > David Plowman (5):
> > >   ipa: raspberrypi: controller: Replace Raspberry Pi debug with
> > >     libcamera debug
> > >   ipa: raspberrypi: alsc: Replace Raspberry Pi debug with libcamera
> > >     debug
> > >   ipa: raspberrypi: awb: Replace Raspberry Pi debug with libcamera debug
> > >   ipa: raspberrypi: Replace Raspberry Pi debug with libcamera debug
> > >   ipa: raspberrypi: Remove legacy Rasberry Pi logging
> > >
> > >  src/ipa/raspberrypi/controller/algorithm.hpp  |  1 -
> > >  src/ipa/raspberrypi/controller/controller.cpp | 29 +++---
> > >  src/ipa/raspberrypi/controller/logging.hpp    | 30 ------
> > >  src/ipa/raspberrypi/controller/rpi/alsc.cpp   | 59 ++++++------
> > >  src/ipa/raspberrypi/controller/rpi/awb.cpp    | 92 ++++++++++---------
> > >  .../controller/rpi/black_level.cpp            |  8 +-
> > >  src/ipa/raspberrypi/controller/rpi/ccm.cpp    | 24 +++--
> > >  .../raspberrypi/controller/rpi/contrast.cpp   | 15 ++-
> > >  src/ipa/raspberrypi/controller/rpi/dpc.cpp    |  8 +-
> > >  src/ipa/raspberrypi/controller/rpi/geq.cpp    | 18 ++--
> > >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 12 ++-
> > >  src/ipa/raspberrypi/controller/rpi/noise.cpp  | 14 ++-
> > >  src/ipa/raspberrypi/controller/rpi/sdn.cpp    | 21 +++--
> > >  .../raspberrypi/controller/rpi/sharpen.cpp    |  8 +-
> > >  14 files changed, 179 insertions(+), 160 deletions(-)
> > >  delete mode 100644 src/ipa/raspberrypi/controller/logging.hpp
>
> --
> Regards,
>
> Laurent Pinchart