| Message ID | 20211028111520.256612-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi (2021-10-28 12:15:12) > Add an 'enable' parameter to the EventNotifier class constructor. > > Currently an EvenNotifier is enabled as soon as it is constructed. > Add an optional parameter to the class constructor to allow post-poning > the notifier activation. > > As the 'enable' parameter has a default value of true, existing users > of the class should not be updated. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks reasonable, and I'm sure I'm about to discover why it's needed... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/base/event_notifier.h | 2 +- > src/libcamera/base/event_notifier.cpp | 12 ++++++++---- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/base/event_notifier.h b/include/libcamera/base/event_notifier.h > index f7722a32ef55..4d373a5290cc 100644 > --- a/include/libcamera/base/event_notifier.h > +++ b/include/libcamera/base/event_notifier.h > @@ -25,7 +25,7 @@ public: > Exception, > }; > > - EventNotifier(int fd, Type type, Object *parent = nullptr); > + EventNotifier(int fd, Type type, Object *parent = nullptr, bool enable = true); > virtual ~EventNotifier(); > > Type type() const { return type_; } > diff --git a/src/libcamera/base/event_notifier.cpp b/src/libcamera/base/event_notifier.cpp > index fd93c0878c6f..40428cf24a50 100644 > --- a/src/libcamera/base/event_notifier.cpp > +++ b/src/libcamera/base/event_notifier.cpp > @@ -26,8 +26,11 @@ namespace libcamera { > * > * The EventNotifier models a file descriptor event source that can be > * monitored. It is created with the file descriptor to be monitored and the > - * type of event, and is enabled by default. It will emit the \ref activated > - * signal whenever an event of the monitored type occurs on the file descriptor. > + * type of event. It will emit the \ref activated signal whenever an event of > + * the monitored type occurs on the file descriptor. > + * > + * An EventNotifier is enabled by default when created, unless otherwise > + * specified through the optional 'enable' constructor parameter. > * > * Supported type of events are EventNotifier::Read, EventNotifier::Write and > * EventNotifier::Exception. The type is specified when constructing the > @@ -62,11 +65,12 @@ namespace libcamera { > * \param[in] fd The file descriptor to monitor > * \param[in] type The event type to monitor > * \param[in] parent The parent Object > + * \param[in] enable Notifier enable flag > */ > -EventNotifier::EventNotifier(int fd, Type type, Object *parent) > +EventNotifier::EventNotifier(int fd, Type type, Object *parent, bool enable) > : Object(parent), fd_(fd), type_(type), enabled_(false) > { > - setEnabled(true); > + setEnabled(enable); > } > > EventNotifier::~EventNotifier() > -- > 2.33.1 >
Hi Jacopo, On 10/28/21 4:45 PM, Jacopo Mondi wrote: > Add an 'enable' parameter to the EventNotifier class constructor. > > Currently an EvenNotifier is enabled as soon as it is constructed. > Add an optional parameter to the class constructor to allow post-poning > the notifier activation. > > As the 'enable' parameter has a default value of true, existing users > of the class should not be updated. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/base/event_notifier.h | 2 +- > src/libcamera/base/event_notifier.cpp | 12 ++++++++---- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/base/event_notifier.h b/include/libcamera/base/event_notifier.h > index f7722a32ef55..4d373a5290cc 100644 > --- a/include/libcamera/base/event_notifier.h > +++ b/include/libcamera/base/event_notifier.h > @@ -25,7 +25,7 @@ public: > Exception, > }; > > - EventNotifier(int fd, Type type, Object *parent = nullptr); > + EventNotifier(int fd, Type type, Object *parent = nullptr, bool enable = true); > virtual ~EventNotifier(); > > Type type() const { return type_; } > diff --git a/src/libcamera/base/event_notifier.cpp b/src/libcamera/base/event_notifier.cpp > index fd93c0878c6f..40428cf24a50 100644 > --- a/src/libcamera/base/event_notifier.cpp > +++ b/src/libcamera/base/event_notifier.cpp > @@ -26,8 +26,11 @@ namespace libcamera { > * > * The EventNotifier models a file descriptor event source that can be > * monitored. It is created with the file descriptor to be monitored and the > - * type of event, and is enabled by default. It will emit the \ref activated > - * signal whenever an event of the monitored type occurs on the file descriptor. > + * type of event. It will emit the \ref activated signal whenever an event of > + * the monitored type occurs on the file descriptor. > + * > + * An EventNotifier is enabled by default when created, unless otherwise > + * specified through the optional 'enable' constructor parameter. > * > * Supported type of events are EventNotifier::Read, EventNotifier::Write and > * EventNotifier::Exception. The type is specified when constructing the > @@ -62,11 +65,12 @@ namespace libcamera { > * \param[in] fd The file descriptor to monitor > * \param[in] type The event type to monitor > * \param[in] parent The parent Object > + * \param[in] enable Notifier enable flag > */ > -EventNotifier::EventNotifier(int fd, Type type, Object *parent) > +EventNotifier::EventNotifier(int fd, Type type, Object *parent, bool enable) > : Object(parent), fd_(fd), type_(type), enabled_(false) > { > - setEnabled(true); > + setEnabled(enable); > } > > EventNotifier::~EventNotifier()
Hi Jacopo, Thank you for the patch. On Thu, Oct 28, 2021 at 01:15:12PM +0200, Jacopo Mondi wrote: > Add an 'enable' parameter to the EventNotifier class constructor. > > Currently an EvenNotifier is enabled as soon as it is constructed. s/EvenNotifier/EventNotifier/ > Add an optional parameter to the class constructor to allow post-poning s/post-poning/postponing/ > the notifier activation. s/activation/enablement/ (as activation refers to the notifier reporting an event). > As the 'enable' parameter has a default value of true, existing users > of the class should not be updated. s/should not be updated/don't need to be updated/ > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/base/event_notifier.h | 2 +- > src/libcamera/base/event_notifier.cpp | 12 ++++++++---- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/base/event_notifier.h b/include/libcamera/base/event_notifier.h > index f7722a32ef55..4d373a5290cc 100644 > --- a/include/libcamera/base/event_notifier.h > +++ b/include/libcamera/base/event_notifier.h > @@ -25,7 +25,7 @@ public: > Exception, > }; > > - EventNotifier(int fd, Type type, Object *parent = nullptr); > + EventNotifier(int fd, Type type, Object *parent = nullptr, bool enable = true); > virtual ~EventNotifier(); > > Type type() const { return type_; } > diff --git a/src/libcamera/base/event_notifier.cpp b/src/libcamera/base/event_notifier.cpp > index fd93c0878c6f..40428cf24a50 100644 > --- a/src/libcamera/base/event_notifier.cpp > +++ b/src/libcamera/base/event_notifier.cpp > @@ -26,8 +26,11 @@ namespace libcamera { > * > * The EventNotifier models a file descriptor event source that can be > * monitored. It is created with the file descriptor to be monitored and the > - * type of event, and is enabled by default. It will emit the \ref activated > - * signal whenever an event of the monitored type occurs on the file descriptor. > + * type of event. It will emit the \ref activated signal whenever an event of > + * the monitored type occurs on the file descriptor. > + * > + * An EventNotifier is enabled by default when created, unless otherwise > + * specified through the optional 'enable' constructor parameter. I'd squash this with the paragraph dealing with enablement: * The \a enable parameter controls whether the notifier is created enabled or * disabled. When the notifier is disabled it ignores events and does not emit * the \ref activated signal. The notifier can be manually enabled or disabled * with the setEnabled() function. The patch looks OK to me, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I don't know how it will be used though, so I may propose an alternative when reviewing the rest of the series. > * > * Supported type of events are EventNotifier::Read, EventNotifier::Write and > * EventNotifier::Exception. The type is specified when constructing the > @@ -62,11 +65,12 @@ namespace libcamera { > * \param[in] fd The file descriptor to monitor > * \param[in] type The event type to monitor > * \param[in] parent The parent Object > + * \param[in] enable Notifier enable flag > */ > -EventNotifier::EventNotifier(int fd, Type type, Object *parent) > +EventNotifier::EventNotifier(int fd, Type type, Object *parent, bool enable) > : Object(parent), fd_(fd), type_(type), enabled_(false) > { > - setEnabled(true); > + setEnabled(enable); > } > > EventNotifier::~EventNotifier()
diff --git a/include/libcamera/base/event_notifier.h b/include/libcamera/base/event_notifier.h index f7722a32ef55..4d373a5290cc 100644 --- a/include/libcamera/base/event_notifier.h +++ b/include/libcamera/base/event_notifier.h @@ -25,7 +25,7 @@ public: Exception, }; - EventNotifier(int fd, Type type, Object *parent = nullptr); + EventNotifier(int fd, Type type, Object *parent = nullptr, bool enable = true); virtual ~EventNotifier(); Type type() const { return type_; } diff --git a/src/libcamera/base/event_notifier.cpp b/src/libcamera/base/event_notifier.cpp index fd93c0878c6f..40428cf24a50 100644 --- a/src/libcamera/base/event_notifier.cpp +++ b/src/libcamera/base/event_notifier.cpp @@ -26,8 +26,11 @@ namespace libcamera { * * The EventNotifier models a file descriptor event source that can be * monitored. It is created with the file descriptor to be monitored and the - * type of event, and is enabled by default. It will emit the \ref activated - * signal whenever an event of the monitored type occurs on the file descriptor. + * type of event. It will emit the \ref activated signal whenever an event of + * the monitored type occurs on the file descriptor. + * + * An EventNotifier is enabled by default when created, unless otherwise + * specified through the optional 'enable' constructor parameter. * * Supported type of events are EventNotifier::Read, EventNotifier::Write and * EventNotifier::Exception. The type is specified when constructing the @@ -62,11 +65,12 @@ namespace libcamera { * \param[in] fd The file descriptor to monitor * \param[in] type The event type to monitor * \param[in] parent The parent Object + * \param[in] enable Notifier enable flag */ -EventNotifier::EventNotifier(int fd, Type type, Object *parent) +EventNotifier::EventNotifier(int fd, Type type, Object *parent, bool enable) : Object(parent), fd_(fd), type_(type), enabled_(false) { - setEnabled(true); + setEnabled(enable); } EventNotifier::~EventNotifier()
Add an 'enable' parameter to the EventNotifier class constructor. Currently an EvenNotifier is enabled as soon as it is constructed. Add an optional parameter to the class constructor to allow post-poning the notifier activation. As the 'enable' parameter has a default value of true, existing users of the class should not be updated. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/base/event_notifier.h | 2 +- src/libcamera/base/event_notifier.cpp | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-)