[libcamera-devel] [PATCH 3/3] libcamera: message: Add user message types

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jul 15, 2019 at 11:59:44AM +0300, Laurent Pinchart wrote:
> On Mon, Jul 15, 2019 at 07:59:35AM +0200, Jacopo Mondi wrote:
> > Add an operation to the Message class to create unique message type to
> > be used by derived classes to uniquely identify the messages they
> > handle.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/include/message.h |  8 ++++++++
> >  src/libcamera/message.cpp       | 20 ++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/src/libcamera/include/message.h b/src/libcamera/include/message.h
> > index db17d647c280..e7dd85a606a3 100644
> > --- a/src/libcamera/include/message.h
> > +++ b/src/libcamera/include/message.h
> > @@ -7,6 +7,8 @@
> >  #ifndef __LIBCAMERA_MESSAGE_H__
> >  #define __LIBCAMERA_MESSAGE_H__
> >  
> > +#include <mutex>
> > +
> >  namespace libcamera {
> >  
> >  class Object;
> > @@ -19,6 +21,7 @@ public:
> >  	enum Type {
> >  		None = 0,
> >  		SignalMessage = 1,
> > +		UserMessage = 1000,
> 
> This should be documented, and it may make sense to split it to a
> separate patch.

Nevermind splitting it to another patch, it's fine to combine them, but
it should still be documented.

> >  	};
> >  
> >  	Message(Type type);
> > @@ -27,11 +30,16 @@ public:
> >  	Type type() const { return type_; }
> >  	Object *receiver() const { return receiver_; }
> >  
> > +	static Type registerUserMessageType();
> 
> I think we can shorten the name to registerMessageType().
> 
> > +
> >  private:
> >  	friend class Thread;
> >  
> >  	Type type_;
> >  	Object *receiver_;
> > +
> > +	static std::mutex mutex_;
> > +	static Type nextUserType_;
> 
> I wonder if we could use an atomic variable instead and remove the
> mutex.
> 
> 	static std::atomic_uint nextUserType_;
> 
> >  };
> >  
> >  class SignalMessage : public Message
> > diff --git a/src/libcamera/message.cpp b/src/libcamera/message.cpp
> > index 0580c1051622..f82a3115595d 100644
> > --- a/src/libcamera/message.cpp
> > +++ b/src/libcamera/message.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include "message.h"
> > +#include "thread.h"
> >  
> >  #include "log.h"
> >  
> > @@ -31,6 +32,9 @@ namespace libcamera {
> >  
> >  LOG_DEFINE_CATEGORY(Message)
> >  
> > +std::mutex Message::mutex_;
> > +Message::Type Message::nextUserType_ = Message::UserMessage;
> > +
> >  /**
> >   * \class Message
> >   * \brief A message that can be posted to a Thread
> > @@ -68,6 +72,22 @@ Message::~Message()
> >   * \return The message receiver
> >   */
> >  
> > +/**
> > + * \brief Retrieve a unique identifier for user defined message types
> 
> Reserve and register a custom user-defined message type
> 
> > + *
> > + * Derived classes can define their own message types and this method provides
> > + * them a unique identifier to use for that purpose.
> 
> How about this ?
> 
>  * Custom message types use values starting at Type::UserMessage. Assigning
>  * custom types manually may lead to accidental duplicated types. To avoid this
>  * problem, this method reserves and returns the next available user-defined
>  * message type.
>  *
>  * The recommended way to use this method is to subclass Message and provide a
>  * static accessor for the custom message type.
>  *
>  * \code{.cpp}
>  * class MyCustomMessage : public Message
>  * {
>  * public:
>  *      MyCustomMessage() : Message(type()) { }
>  *
>  *      static Message::Type type()
>  *      {
>  *              static MessageType type = registerMessageType();
>  *              return type;
>  *      }
>  * };
>  * \endcode
> 
> > + *
> > + * \return A new unique message type
> > + */
> > +Message::Type Message::registerUserMessageType()
> > +{
> > +	MutexLocker locker(mutex_);
> > +
> > +	return nextUserType_ = static_cast<Type>
> > +			      (static_cast<unsigned int>(nextUserType_) + 1);
> 
> 	return static_cast<Message::Type>(nextUserType_++);
> 
> > +}
> > +
> >  /**
> >   * \class SignalMessage
> >   * \brief A message carrying a Signal across threads
> 

-- 
Regards,

Laurent Pinchart