| Message ID | 20190117202043.21420-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Jan 17, 2019 at 08:20:41PM +0000, Kieran Bingham wrote: > The msec parameter was multiplied as a 32 bit value when converting to > nanosecond resolution. This wraps at 4.2949 seconds, and causes timers > longer than this to fail. > > Fix the multiplication to upcast to 64 bit using an unsigned long long > specifier on the multiplier. > > While we're here, initialise the two integer class members in the > constructor initialiser list. > A Fixes: tag would be nice. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/timer.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp > index b5212ba26869..306825fde3c0 100644 > --- a/src/libcamera/timer.cpp > +++ b/src/libcamera/timer.cpp > @@ -36,6 +36,7 @@ namespace libcamera { > * \brief Construct a timer > */ > Timer::Timer() > + : interval_(0), deadline_(0) > { > } > > @@ -51,7 +52,7 @@ void Timer::start(unsigned int msec) > clock_gettime(CLOCK_MONOTONIC, &tp); > > interval_ = msec; > - deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000; > + deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000ULL; > > LOG(Debug) << "Starting timer " << this << " with interval " << msec > << ": deadline " << deadline_;
On 17/01/2019 20:37, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Jan 17, 2019 at 08:20:41PM +0000, Kieran Bingham wrote: >> The msec parameter was multiplied as a 32 bit value when converting to >> nanosecond resolution. This wraps at 4.2949 seconds, and causes timers >> longer than this to fail. >> >> Fix the multiplication to upcast to 64 bit using an unsigned long long >> specifier on the multiplier. >> >> While we're here, initialise the two integer class members in the >> constructor initialiser list. >> > > A Fixes: tag would be nice. Apart from that, Wait, what? It was here? How did I lose that! I'll add it back. Fixes: 1a57bcb8d1a7 ("libcamera: Add event notification infrastructure") > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/timer.cpp | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp >> index b5212ba26869..306825fde3c0 100644 >> --- a/src/libcamera/timer.cpp >> +++ b/src/libcamera/timer.cpp >> @@ -36,6 +36,7 @@ namespace libcamera { >> * \brief Construct a timer >> */ >> Timer::Timer() >> + : interval_(0), deadline_(0) >> { >> } >> >> @@ -51,7 +52,7 @@ void Timer::start(unsigned int msec) >> clock_gettime(CLOCK_MONOTONIC, &tp); >> >> interval_ = msec; >> - deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000; >> + deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000ULL; >> >> LOG(Debug) << "Starting timer " << this << " with interval " << msec >> << ": deadline " << deadline_; >
Hi Kieran, Thanks for your effort. On 2019-01-17 20:20:41 +0000, Kieran Bingham wrote: > The msec parameter was multiplied as a 32 bit value when converting to > nanosecond resolution. This wraps at 4.2949 seconds, and causes timers > longer than this to fail. > > Fix the multiplication to upcast to 64 bit using an unsigned long long > specifier on the multiplier. > > While we're here, initialise the two integer class members in the > constructor initialiser list. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/timer.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp > index b5212ba26869..306825fde3c0 100644 > --- a/src/libcamera/timer.cpp > +++ b/src/libcamera/timer.cpp > @@ -36,6 +36,7 @@ namespace libcamera { > * \brief Construct a timer > */ > Timer::Timer() > + : interval_(0), deadline_(0) > { > } > > @@ -51,7 +52,7 @@ void Timer::start(unsigned int msec) > clock_gettime(CLOCK_MONOTONIC, &tp); > > interval_ = msec; > - deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000; > + deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000ULL; > > LOG(Debug) << "Starting timer " << this << " with interval " << msec > << ": deadline " << deadline_; > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp index b5212ba26869..306825fde3c0 100644 --- a/src/libcamera/timer.cpp +++ b/src/libcamera/timer.cpp @@ -36,6 +36,7 @@ namespace libcamera { * \brief Construct a timer */ Timer::Timer() + : interval_(0), deadline_(0) { } @@ -51,7 +52,7 @@ void Timer::start(unsigned int msec) clock_gettime(CLOCK_MONOTONIC, &tp); interval_ = msec; - deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000; + deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000ULL; LOG(Debug) << "Starting timer " << this << " with interval " << msec << ": deadline " << deadline_;
The msec parameter was multiplied as a 32 bit value when converting to nanosecond resolution. This wraps at 4.2949 seconds, and causes timers longer than this to fail. Fix the multiplication to upcast to 64 bit using an unsigned long long specifier on the multiplier. While we're here, initialise the two integer class members in the constructor initialiser list. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/timer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)