| Message ID | 20210518100706.578526-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thank you for this patch. On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > A new RPiController::Duration typedef is defined to represent a > std::chrono::duration type with double precision nanosecond timebase > that will be used throughout. This minimises the loss of precision when > converting timebases. A function for returning the duration count in any > timebase is also provided. > > An operator << overload is define to help with displaying > RPiController::Duration value in stream objects. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ > 1 file changed, 33 insertions(+) > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp b/src/ipa/raspberrypi/controller/duration.hpp > new file mode 100644 > index 000000000000..98aa3d78f52f > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/duration.hpp > @@ -0,0 +1,33 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > + * > + * duration.hpp - Helper macros for std::chrono::duration handling. > + */ > +#pragma once > + > +#include <chrono> > +#include <iomanip> > +#include <iostream> > + > +namespace RPiController { > + > +using Duration = std::chrono::duration<double, std::nano>; > + > +// Helper to convert and return the count of any std::chrono::duration type to > +// another timebase. Use a double rep type to try and preserve precision. > +template <typename P, typename T> > +static constexpr double DurationValue(T const &d) > +{ > + return std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); > +}; > + > +static inline std::ostream &operator<<(std::ostream &os, const Duration &duration) > +{ > + std::streamsize ss = os.precision(); > + os << std::fixed << std::setprecision(2) << DurationValue<std::micro>(duration) << " us"; I suppose I wonder slightly why we change the precision, I assume the output is just a bit tidier like that? And a space before the "us"? I guess, why not! Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > + os << std::setprecision(ss) << std::defaultfloat; > + return os; > +} > + > +} // namespace RPiController > -- > 2.25.1 >
Hi David, Thank you for your review feedback. On Wed, 19 May 2021 at 11:38, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thank you for this patch. > > On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > A new RPiController::Duration typedef is defined to represent a > > std::chrono::duration type with double precision nanosecond timebase > > that will be used throughout. This minimises the loss of precision when > > converting timebases. A function for returning the duration count in any > > timebase is also provided. > > > > An operator << overload is define to help with displaying > > RPiController::Duration value in stream objects. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp > b/src/ipa/raspberrypi/controller/duration.hpp > > new file mode 100644 > > index 000000000000..98aa3d78f52f > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/duration.hpp > > @@ -0,0 +1,33 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > > + * > > + * duration.hpp - Helper macros for std::chrono::duration handling. > > + */ > > +#pragma once > > + > > +#include <chrono> > > +#include <iomanip> > > +#include <iostream> > > + > > +namespace RPiController { > > + > > +using Duration = std::chrono::duration<double, std::nano>; > > + > > +// Helper to convert and return the count of any std::chrono::duration > type to > > +// another timebase. Use a double rep type to try and preserve > precision. > > +template <typename P, typename T> > > +static constexpr double DurationValue(T const &d) > > +{ > > + return std::chrono::duration_cast<std::chrono::duration<double, > P>>(d).count(); > > +}; > > + > > +static inline std::ostream &operator<<(std::ostream &os, const Duration > &duration) > > +{ > > + std::streamsize ss = os.precision(); > > + os << std::fixed << std::setprecision(2) << > DurationValue<std::micro>(duration) << " us"; > > I suppose I wonder slightly why we change the precision, I assume the > output is just a bit tidier like that? And a space before the "us"? I > guess, why not! > Yes, I did not want more than 2 decimal places of precision in the output. I can remove the space before the "us" in the next revision. Regards, Naush > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks > David > > > + os << std::setprecision(ss) << std::defaultfloat; > > + return os; > > +} > > + > > +} // namespace RPiController > > -- > > 2.25.1 > > >
Hi Naush On Wed, 19 May 2021 at 11:50, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi David, > > Thank you for your review feedback. > > On Wed, 19 May 2021 at 11:38, David Plowman <david.plowman@raspberrypi.com> wrote: >> >> Hi Naush >> >> Thank you for this patch. >> >> On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> wrote: >> > >> > A new RPiController::Duration typedef is defined to represent a >> > std::chrono::duration type with double precision nanosecond timebase >> > that will be used throughout. This minimises the loss of precision when >> > converting timebases. A function for returning the duration count in any >> > timebase is also provided. >> > >> > An operator << overload is define to help with displaying >> > RPiController::Duration value in stream objects. >> > >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> > --- >> > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ >> > 1 file changed, 33 insertions(+) >> > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp >> > >> > diff --git a/src/ipa/raspberrypi/controller/duration.hpp b/src/ipa/raspberrypi/controller/duration.hpp >> > new file mode 100644 >> > index 000000000000..98aa3d78f52f >> > --- /dev/null >> > +++ b/src/ipa/raspberrypi/controller/duration.hpp >> > @@ -0,0 +1,33 @@ >> > +/* SPDX-License-Identifier: BSD-2-Clause */ >> > +/* >> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited >> > + * >> > + * duration.hpp - Helper macros for std::chrono::duration handling. >> > + */ >> > +#pragma once >> > + >> > +#include <chrono> >> > +#include <iomanip> >> > +#include <iostream> >> > + >> > +namespace RPiController { >> > + >> > +using Duration = std::chrono::duration<double, std::nano>; >> > + >> > +// Helper to convert and return the count of any std::chrono::duration type to >> > +// another timebase. Use a double rep type to try and preserve precision. >> > +template <typename P, typename T> >> > +static constexpr double DurationValue(T const &d) >> > +{ >> > + return std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); >> > +}; >> > + >> > +static inline std::ostream &operator<<(std::ostream &os, const Duration &duration) >> > +{ >> > + std::streamsize ss = os.precision(); >> > + os << std::fixed << std::setprecision(2) << DurationValue<std::micro>(duration) << " us"; >> >> I suppose I wonder slightly why we change the precision, I assume the >> output is just a bit tidier like that? And a space before the "us"? I >> guess, why not! > > > Yes, I did not want more than 2 decimal places of precision in the output. I can remove the space > before the "us" in the next revision. It's OK... I don't mind the space! David > > Regards, > Naush > >> >> >> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> >> >> Thanks >> David >> >> > + os << std::setprecision(ss) << std::defaultfloat; >> > + return os; >> > +} >> > + >> > +} // namespace RPiController >> > -- >> > 2.25.1 >> >
Hi Naush, Thank you for the patch. On Tue, May 18, 2021 at 11:07:03AM +0100, Naushir Patuck wrote: > A new RPiController::Duration typedef is defined to represent a > std::chrono::duration type with double precision nanosecond timebase > that will be used throughout. This minimises the loss of precision when > converting timebases. A function for returning the duration count in any > timebase is also provided. > > An operator << overload is define to help with displaying > RPiController::Duration value in stream objects. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ > 1 file changed, 33 insertions(+) > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp b/src/ipa/raspberrypi/controller/duration.hpp > new file mode 100644 > index 000000000000..98aa3d78f52f > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/duration.hpp > @@ -0,0 +1,33 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > + * > + * duration.hpp - Helper macros for std::chrono::duration handling. > + */ > +#pragma once > + > +#include <chrono> > +#include <iomanip> > +#include <iostream> > + > +namespace RPiController { > + > +using Duration = std::chrono::duration<double, std::nano>; > + > +// Helper to convert and return the count of any std::chrono::duration type to > +// another timebase. Use a double rep type to try and preserve precision. > +template <typename P, typename T> > +static constexpr double DurationValue(T const &d) > +{ > + return std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); > +}; > + > +static inline std::ostream &operator<<(std::ostream &os, const Duration &duration) > +{ > + std::streamsize ss = os.precision(); > + os << std::fixed << std::setprecision(2) << DurationValue<std::micro>(duration) << " us"; > + os << std::setprecision(ss) << std::defaultfloat; This will reset to std::defaultfloat, while the stream may already be in std::fixed. > + return os; > +} Would it be too much yak-shaving to ask for the implementation to be closer to https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt, and moved to utils.h ? It could be useful more widely in libcamera than just in the RPi IPA. > + > +} // namespace RPiController
Hi Laurent, On Wed, 19 May 2021 at 15:24, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Tue, May 18, 2021 at 11:07:03AM +0100, Naushir Patuck wrote: > > A new RPiController::Duration typedef is defined to represent a > > std::chrono::duration type with double precision nanosecond timebase > > that will be used throughout. This minimises the loss of precision when > > converting timebases. A function for returning the duration count in any > > timebase is also provided. > > > > An operator << overload is define to help with displaying > > RPiController::Duration value in stream objects. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp > b/src/ipa/raspberrypi/controller/duration.hpp > > new file mode 100644 > > index 000000000000..98aa3d78f52f > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/duration.hpp > > @@ -0,0 +1,33 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > > + * > > + * duration.hpp - Helper macros for std::chrono::duration handling. > > + */ > > +#pragma once > > + > > +#include <chrono> > > +#include <iomanip> > > +#include <iostream> > > + > > +namespace RPiController { > > + > > +using Duration = std::chrono::duration<double, std::nano>; > > + > > +// Helper to convert and return the count of any std::chrono::duration > type to > > +// another timebase. Use a double rep type to try and preserve > precision. > > +template <typename P, typename T> > > +static constexpr double DurationValue(T const &d) > > +{ > > + return std::chrono::duration_cast<std::chrono::duration<double, > P>>(d).count(); > > +}; > > + > > +static inline std::ostream &operator<<(std::ostream &os, const Duration > &duration) > > +{ > > + std::streamsize ss = os.precision(); > > + os << std::fixed << std::setprecision(2) << > DurationValue<std::micro>(duration) << " us"; > > + os << std::setprecision(ss) << std::defaultfloat; > > This will reset to std::defaultfloat, while the stream may already be in > std::fixed. > Good point, I should save and reapply like I do with precision. > > > + return os; > > +} > > Would it be too much yak-shaving to ask for the implementation to be > closer to > https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt, and > moved to utils.h ? It could be useful more widely in libcamera than just > in the RPi IPA. > Yes, I could look into that. Did you mean move the whole implementation into utils.h, or only the operator <<() bit? Actually, I would like to make an impactful change in the next revision of the series, and replace Duration with a class derived from a std::chrono::duration object. This is a bit neater in terms of encapsulation, and allows me to implement an operator bool() method that cleans up the code a bit more. Functionally, everything else remains identical. Did you want me to put that into utils.h? Sorry David, your review for patch 1/4 may need redoing with :-( Regards, Naush > > + > > +} // namespace RPiController > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, May 19, 2021 at 03:34:05PM +0100, Naushir Patuck wrote: > On Wed, 19 May 2021 at 15:24, Laurent Pinchart wrote: > > On Tue, May 18, 2021 at 11:07:03AM +0100, Naushir Patuck wrote: > > > A new RPiController::Duration typedef is defined to represent a > > > std::chrono::duration type with double precision nanosecond timebase > > > that will be used throughout. This minimises the loss of precision when > > > converting timebases. A function for returning the duration count in any > > > timebase is also provided. > > > > > > An operator << overload is define to help with displaying > > > RPiController::Duration value in stream objects. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ > > > 1 file changed, 33 insertions(+) > > > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > > > > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp b/src/ipa/raspberrypi/controller/duration.hpp > > > new file mode 100644 > > > index 000000000000..98aa3d78f52f > > > --- /dev/null > > > +++ b/src/ipa/raspberrypi/controller/duration.hpp > > > @@ -0,0 +1,33 @@ > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > +/* > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > > > + * > > > + * duration.hpp - Helper macros for std::chrono::duration handling. > > > + */ > > > +#pragma once > > > + > > > +#include <chrono> > > > +#include <iomanip> > > > +#include <iostream> > > > + > > > +namespace RPiController { > > > + > > > +using Duration = std::chrono::duration<double, std::nano>; > > > + > > > +// Helper to convert and return the count of any std::chrono::duration type to > > > +// another timebase. Use a double rep type to try and preserve precision. > > > +template <typename P, typename T> > > > +static constexpr double DurationValue(T const &d) > > > +{ > > > + return std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); > > > +}; > > > + > > > +static inline std::ostream &operator<<(std::ostream &os, const Duration &duration) > > > +{ > > > + std::streamsize ss = os.precision(); > > > + os << std::fixed << std::setprecision(2) << DurationValue<std::micro>(duration) << " us"; > > > + os << std::setprecision(ss) << std::defaultfloat; > > > > This will reset to std::defaultfloat, while the stream may already be in > > std::fixed. > > Good point, I should save and reapply like I do with precision. > > > > + return os; > > > +} > > > > Would it be too much yak-shaving to ask for the implementation to be > > closer to > > https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt, and > > moved to utils.h ? It could be useful more widely in libcamera than just > > in the RPi IPA. > > Yes, I could look into that. Did you mean move the whole implementation into utils.h, or > only the operator <<() bit? I meant operator<<(). DurationValue would likely need to go there too if it's used internally, but the implementation of operator<<() could also hardcode it as os << std::fixed << std::setprecision(2) << duration.count() / 1000.0 << " us"; if needed. > Actually, I would like to make an impactful change in the next > revision of the series, and replace Duration with a class derived from a std::chrono::duration > object. This is a bit neater in terms of encapsulation, and allows me to implement an > operator bool() method that cleans up the code a bit more. Functionally, everything else > remains identical. Did you want me to put that into utils.h? I think it should actually go to a public header if we want to standardize all timestamps and durations to std::chrono, but that's too much yak-shaving :-) We can handle it later. We already have the following types in utils.h: using clock = std::chrono::steady_clock; using duration = std::chrono::steady_clock::duration; using time_point = std::chrono::steady_clock::time_point; There are also a few helpers to perform string conversion and to convert to a timespec. Ideally all of this should be consolidated, but again it's propably a bit too much work. If we can make operator<<() conform to the C++20 definition to be able to later switch to it, I'll be happy already. > Sorry David, your review for patch 1/4 may need redoing with :-( > > > > + > > > +} // namespace RPiController
Hi Laurent, On Wed, 19 May 2021 at 15:48, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Wed, May 19, 2021 at 03:34:05PM +0100, Naushir Patuck wrote: > > On Wed, 19 May 2021 at 15:24, Laurent Pinchart wrote: > > > On Tue, May 18, 2021 at 11:07:03AM +0100, Naushir Patuck wrote: > > > > A new RPiController::Duration typedef is defined to represent a > > > > std::chrono::duration type with double precision nanosecond timebase > > > > that will be used throughout. This minimises the loss of precision > when > > > > converting timebases. A function for returning the duration count in > any > > > > timebase is also provided. > > > > > > > > An operator << overload is define to help with displaying > > > > RPiController::Duration value in stream objects. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/ipa/raspberrypi/controller/duration.hpp | 33 > +++++++++++++++++++++ > > > > 1 file changed, 33 insertions(+) > > > > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp > b/src/ipa/raspberrypi/controller/duration.hpp > > > > new file mode 100644 > > > > index 000000000000..98aa3d78f52f > > > > --- /dev/null > > > > +++ b/src/ipa/raspberrypi/controller/duration.hpp > > > > @@ -0,0 +1,33 @@ > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > +/* > > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > > > > + * > > > > + * duration.hpp - Helper macros for std::chrono::duration handling. > > > > + */ > > > > +#pragma once > > > > + > > > > +#include <chrono> > > > > +#include <iomanip> > > > > +#include <iostream> > > > > + > > > > +namespace RPiController { > > > > + > > > > +using Duration = std::chrono::duration<double, std::nano>; > > > > + > > > > +// Helper to convert and return the count of any > std::chrono::duration type to > > > > +// another timebase. Use a double rep type to try and preserve > precision. > > > > +template <typename P, typename T> > > > > +static constexpr double DurationValue(T const &d) > > > > +{ > > > > + return > std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); > > > > +}; > > > > + > > > > +static inline std::ostream &operator<<(std::ostream &os, const > Duration &duration) > > > > +{ > > > > + std::streamsize ss = os.precision(); > > > > + os << std::fixed << std::setprecision(2) << > DurationValue<std::micro>(duration) << " us"; > > > > + os << std::setprecision(ss) << std::defaultfloat; > > > > > > This will reset to std::defaultfloat, while the stream may already be > in > > > std::fixed. > > > > Good point, I should save and reapply like I do with precision. > > > > > > + return os; > > > > +} > > > > > > Would it be too much yak-shaving to ask for the implementation to be > > > closer to > > > https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt, and > > > moved to utils.h ? It could be useful more widely in libcamera than > just > > > in the RPi IPA. > > > > Yes, I could look into that. Did you mean move the whole implementation > into utils.h, or > > only the operator <<() bit? > > I meant operator<<(). DurationValue would likely need to go there too if > it's used internally, but the implementation of operator<<() could also > hardcode it as > > os << std::fixed << std::setprecision(2) << duration.count() / > 1000.0 << " us"; > > if needed. > > > Actually, I would like to make an impactful change in the next > > revision of the series, and replace Duration with a class derived from a > std::chrono::duration > > object. This is a bit neater in terms of encapsulation, and allows me > to implement an > > operator bool() method that cleans up the code a bit more. > Functionally, everything else > > remains identical. Did you want me to put that into utils.h? > > I think it should actually go to a public header if we want to > standardize all timestamps and durations to std::chrono, but that's too > much yak-shaving :-) We can handle it later. > > We already have the following types in utils.h: > > using clock = std::chrono::steady_clock; > using duration = std::chrono::steady_clock::duration; > using time_point = std::chrono::steady_clock::time_point; > > There are also a few helpers to perform string conversion and to convert > to a timespec. Ideally all of this should be consolidated, but again > it's propably a bit too much work. If we can make operator<<() conform > to the C++20 definition to be able to later switch to it, I'll be happy > already. > Here's what I think I will do: I will update patch 1/4 with a new Duration class, now defined in utils.h. This will include operator <<() based on the C++20 signature from: https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt The only thing I would change from the C++20 functionally is to hardcode it to output in microseconds rather than taking the base unit of the Duration class (which is nanoseconds). For my current usage, us makes more sense than ns, but I do accept that for timestamps, etc. ns may be more appropriate. Perhaps some further specialization is needed? All the existing std::chrono definitions will remain as-is and we can convert to Duration in due course. Let me know if you agree/disagree with the above. Regards, Naush > > > Sorry David, your review for patch 1/4 may need redoing with :-( > > > > > > + > > > > +} // namespace RPiController > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, May 19, 2021 at 04:21:04PM +0100, Naushir Patuck wrote: > On Wed, 19 May 2021 at 15:48, Laurent Pinchart wrote: > > On Wed, May 19, 2021 at 03:34:05PM +0100, Naushir Patuck wrote: > > > On Wed, 19 May 2021 at 15:24, Laurent Pinchart wrote: > > > > On Tue, May 18, 2021 at 11:07:03AM +0100, Naushir Patuck wrote: > > > > > A new RPiController::Duration typedef is defined to represent a > > > > > std::chrono::duration type with double precision nanosecond timebase > > > > > that will be used throughout. This minimises the loss of precision when > > > > > converting timebases. A function for returning the duration count in any > > > > > timebase is also provided. > > > > > > > > > > An operator << overload is define to help with displaying > > > > > RPiController::Duration value in stream objects. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ > > > > > 1 file changed, 33 insertions(+) > > > > > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp b/src/ipa/raspberrypi/controller/duration.hpp > > > > > new file mode 100644 > > > > > index 000000000000..98aa3d78f52f > > > > > --- /dev/null > > > > > +++ b/src/ipa/raspberrypi/controller/duration.hpp > > > > > @@ -0,0 +1,33 @@ > > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > > +/* > > > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > > > > > + * > > > > > + * duration.hpp - Helper macros for std::chrono::duration handling. > > > > > + */ > > > > > +#pragma once > > > > > + > > > > > +#include <chrono> > > > > > +#include <iomanip> > > > > > +#include <iostream> > > > > > + > > > > > +namespace RPiController { > > > > > + > > > > > +using Duration = std::chrono::duration<double, std::nano>; > > > > > + > > > > > +// Helper to convert and return the count of any std::chrono::duration type to > > > > > +// another timebase. Use a double rep type to try and preserve precision. > > > > > +template <typename P, typename T> > > > > > +static constexpr double DurationValue(T const &d) > > > > > +{ > > > > > + return std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); > > > > > +}; > > > > > + > > > > > +static inline std::ostream &operator<<(std::ostream &os, const Duration &duration) > > > > > +{ > > > > > + std::streamsize ss = os.precision(); > > > > > + os << std::fixed << std::setprecision(2) << DurationValue<std::micro>(duration) << " us"; > > > > > + os << std::setprecision(ss) << std::defaultfloat; > > > > > > > > This will reset to std::defaultfloat, while the stream may already be in > > > > std::fixed. > > > > > > Good point, I should save and reapply like I do with precision. > > > > > > > > + return os; > > > > > +} > > > > > > > > Would it be too much yak-shaving to ask for the implementation to be > > > > closer to > > > > https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt, and > > > > moved to utils.h ? It could be useful more widely in libcamera than just > > > > in the RPi IPA. > > > > > > Yes, I could look into that. Did you mean move the whole implementation into utils.h, or > > > only the operator <<() bit? > > > > I meant operator<<(). DurationValue would likely need to go there too if > > it's used internally, but the implementation of operator<<() could also > > hardcode it as > > > > os << std::fixed << std::setprecision(2) << duration.count() / 1000.0 << " us"; > > > > if needed. > > > > > Actually, I would like to make an impactful change in the next > > > revision of the series, and replace Duration with a class derived from a std::chrono::duration > > > object. This is a bit neater in terms of encapsulation, and allows me to implement an > > > operator bool() method that cleans up the code a bit more. Functionally, everything else > > > remains identical. Did you want me to put that into utils.h? > > > > I think it should actually go to a public header if we want to > > standardize all timestamps and durations to std::chrono, but that's too > > much yak-shaving :-) We can handle it later. > > > > We already have the following types in utils.h: > > > > using clock = std::chrono::steady_clock; > > using duration = std::chrono::steady_clock::duration; > > using time_point = std::chrono::steady_clock::time_point; > > > > There are also a few helpers to perform string conversion and to convert > > to a timespec. Ideally all of this should be consolidated, but again > > it's propably a bit too much work. If we can make operator<<() conform > > to the C++20 definition to be able to later switch to it, I'll be happy > > already. > > Here's what I think I will do: > > I will update patch 1/4 with a new Duration class, now defined in utils.h. > This will include > operator <<() based on the C++20 signature from: > https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt Sounds good. > The only thing I would change from the C++20 functionally is to hardcode it to output in > microseconds rather than taking the base unit of the Duration class (which is nanoseconds). > For my current usage, us makes more sense than ns, but I do accept that for timestamps, etc. > ns may be more appropriate. Perhaps some further specialization is needed? Given that you print the value as a floating point number, would it make sense to print it in seconds ? > All the existing std::chrono definitions will remain as-is and we can > convert to Duration in due > course. > > Let me know if you agree/disagree with the above. > > > > Sorry David, your review for patch 1/4 may need redoing with :-( > > > > > > > > + > > > > > +} // namespace RPiController
On Wed, 19 May 2021 at 16:38, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Wed, May 19, 2021 at 04:21:04PM +0100, Naushir Patuck wrote: > > On Wed, 19 May 2021 at 15:48, Laurent Pinchart wrote: > > > On Wed, May 19, 2021 at 03:34:05PM +0100, Naushir Patuck wrote: > > > > On Wed, 19 May 2021 at 15:24, Laurent Pinchart wrote: > > > > > On Tue, May 18, 2021 at 11:07:03AM +0100, Naushir Patuck wrote: > > > > > > A new RPiController::Duration typedef is defined to represent a > > > > > > std::chrono::duration type with double precision nanosecond > timebase > > > > > > that will be used throughout. This minimises the loss of > precision when > > > > > > converting timebases. A function for returning the duration > count in any > > > > > > timebase is also provided. > > > > > > > > > > > > An operator << overload is define to help with displaying > > > > > > RPiController::Duration value in stream objects. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > src/ipa/raspberrypi/controller/duration.hpp | 33 > +++++++++++++++++++++ > > > > > > 1 file changed, 33 insertions(+) > > > > > > create mode 100644 src/ipa/raspberrypi/controller/duration.hpp > > > > > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/duration.hpp > b/src/ipa/raspberrypi/controller/duration.hpp > > > > > > new file mode 100644 > > > > > > index 000000000000..98aa3d78f52f > > > > > > --- /dev/null > > > > > > +++ b/src/ipa/raspberrypi/controller/duration.hpp > > > > > > @@ -0,0 +1,33 @@ > > > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > > > +/* > > > > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited > > > > > > + * > > > > > > + * duration.hpp - Helper macros for std::chrono::duration > handling. > > > > > > + */ > > > > > > +#pragma once > > > > > > + > > > > > > +#include <chrono> > > > > > > +#include <iomanip> > > > > > > +#include <iostream> > > > > > > + > > > > > > +namespace RPiController { > > > > > > + > > > > > > +using Duration = std::chrono::duration<double, std::nano>; > > > > > > + > > > > > > +// Helper to convert and return the count of any > std::chrono::duration type to > > > > > > +// another timebase. Use a double rep type to try and preserve > precision. > > > > > > +template <typename P, typename T> > > > > > > +static constexpr double DurationValue(T const &d) > > > > > > +{ > > > > > > + return > std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); > > > > > > +}; > > > > > > + > > > > > > +static inline std::ostream &operator<<(std::ostream &os, const > Duration &duration) > > > > > > +{ > > > > > > + std::streamsize ss = os.precision(); > > > > > > + os << std::fixed << std::setprecision(2) << > DurationValue<std::micro>(duration) << " us"; > > > > > > + os << std::setprecision(ss) << std::defaultfloat; > > > > > > > > > > This will reset to std::defaultfloat, while the stream may already > be in > > > > > std::fixed. > > > > > > > > Good point, I should save and reapply like I do with precision. > > > > > > > > > > + return os; > > > > > > +} > > > > > > > > > > Would it be too much yak-shaving to ask for the implementation to > be > > > > > closer to > > > > > https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt, > and > > > > > moved to utils.h ? It could be useful more widely in libcamera > than just > > > > > in the RPi IPA. > > > > > > > > Yes, I could look into that. Did you mean move the whole > implementation into utils.h, or > > > > only the operator <<() bit? > > > > > > I meant operator<<(). DurationValue would likely need to go there too > if > > > it's used internally, but the implementation of operator<<() could also > > > hardcode it as > > > > > > os << std::fixed << std::setprecision(2) << duration.count() / > 1000.0 << " us"; > > > > > > if needed. > > > > > > > Actually, I would like to make an impactful change in the next > > > > revision of the series, and replace Duration with a class derived > from a std::chrono::duration > > > > object. This is a bit neater in terms of encapsulation, and allows > me to implement an > > > > operator bool() method that cleans up the code a bit more. > Functionally, everything else > > > > remains identical. Did you want me to put that into utils.h? > > > > > > I think it should actually go to a public header if we want to > > > standardize all timestamps and durations to std::chrono, but that's too > > > much yak-shaving :-) We can handle it later. > > > > > > We already have the following types in utils.h: > > > > > > using clock = std::chrono::steady_clock; > > > using duration = std::chrono::steady_clock::duration; > > > using time_point = std::chrono::steady_clock::time_point; > > > > > > There are also a few helpers to perform string conversion and to > convert > > > to a timespec. Ideally all of this should be consolidated, but again > > > it's propably a bit too much work. If we can make operator<<() conform > > > to the C++20 definition to be able to later switch to it, I'll be happy > > > already. > > > > Here's what I think I will do: > > > > I will update patch 1/4 with a new Duration class, now defined in > utils.h. > > This will include > > operator <<() based on the C++20 signature from: > > https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt > > Sounds good. > > > The only thing I would change from the C++20 functionally is to hardcode > it to output in > > microseconds rather than taking the base unit of the Duration class > (which is nanoseconds). > > For my current usage, us makes more sense than ns, but I do accept that > for timestamps, etc. > > ns may be more appropriate. Perhaps some further specialization is > needed? > > Given that you print the value as a floating point number, would it make > sense to print it in seconds ? > My personal preference would be to keep it in microseconds, as I find it easier to read the values that relate to exposure, frame durations, etc. However, I am not strongly opposed to changing it if the consensus goes the other way. > > > All the existing std::chrono definitions will remain as-is and we can > > convert to Duration in due > > course. > > > > Let me know if you agree/disagree with the above. > > > > > > Sorry David, your review for patch 1/4 may need redoing with :-( > > > > > > > > > > + > > > > > > +} // namespace RPiController > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/ipa/raspberrypi/controller/duration.hpp b/src/ipa/raspberrypi/controller/duration.hpp new file mode 100644 index 000000000000..98aa3d78f52f --- /dev/null +++ b/src/ipa/raspberrypi/controller/duration.hpp @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2021, Raspberry Pi (Trading) Limited + * + * duration.hpp - Helper macros for std::chrono::duration handling. + */ +#pragma once + +#include <chrono> +#include <iomanip> +#include <iostream> + +namespace RPiController { + +using Duration = std::chrono::duration<double, std::nano>; + +// Helper to convert and return the count of any std::chrono::duration type to +// another timebase. Use a double rep type to try and preserve precision. +template <typename P, typename T> +static constexpr double DurationValue(T const &d) +{ + return std::chrono::duration_cast<std::chrono::duration<double, P>>(d).count(); +}; + +static inline std::ostream &operator<<(std::ostream &os, const Duration &duration) +{ + std::streamsize ss = os.precision(); + os << std::fixed << std::setprecision(2) << DurationValue<std::micro>(duration) << " us"; + os << std::setprecision(ss) << std::defaultfloat; + return os; +} + +} // namespace RPiController
A new RPiController::Duration typedef is defined to represent a std::chrono::duration type with double precision nanosecond timebase that will be used throughout. This minimises the loss of precision when converting timebases. A function for returning the duration count in any timebase is also provided. An operator << overload is define to help with displaying RPiController::Duration value in stream objects. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/controller/duration.hpp | 33 +++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 src/ipa/raspberrypi/controller/duration.hpp