Best practice when modifying data of a QObject from another thread

Can someone suggest to me the best practice for the following use case:

I have a QObject class with signals that live in the main thread, for example

class Motor : public QObject
{
   Q_OBJECT
   Q_PROPERTY(int speed READ speed NOTIFY speedChanged FINAL)

   friend class Plc;

public:
   explicit Motor(QObject *parent = nullptr) :
      QObject{ parent }
   {}

   int speed() const { return m_speed; }

signals:
   void speedChanged();

protected:
   void setSpeed(int speed) {
      if(m_speed == speed)
          return;
      m_speed = speed:
      emit speedChanged();
   }

private:
   int m_speed = 0;
};

The setter is defined as protected because I want that the main program can only read the value. Only friend class Plc can write it.

Plc class is running on a different thread. It reads data coming from a PLC and update the Motor class.

Below an example of the main thread event loop

void MainWindow::MainWindow()
{
   m_plc = new Plc();
   connect(this, &MainWindow::triggerRead, m_plc, &Plc::readPlcData);
   connect(m_plc , &Plc::readCompleted, this, &MainWindow::processData);
   m_plc->setResource(&m_motor);

   m_plcThread = new QThread(this);
   m_plc->moveToThread(m_plcThread);
   connect(m_plcThread, &QThread::started, m_plc, &Plc::readPlcData);

   thread->start();
}

void processData()
{
   // Limiting the motor speed
   if(m_motor.speed() > 20)
      plc->writeMotorSpeed(20); // Write data into PLC only. Next read will update motor.speed value

   // Do other stuff

   emit triggerRead();
}

Below an exaple of Plc class

class Plc : public QObject
{
   Plc() : QObject{ nullptr }
   {
      connect(this, &Plc::readPlcData, this, &Plc::doReadData);
   }

   void setResource(Motor *motorPtr) { m_motor = motorPtr; }

signals:
   void readPlcData();
   void readCompleted(QPrivateSingal);

private:
   void doReadData()
   {
      // Reding data from PLC

      if(m_motor) {
         m_motor->setSpeed(data[0]);
      }

      emit readCompleted(QPrivateSingal());
   }
   
private:
   Motor *m_motor = nullptr;
};

In this scenario, is "impossible" that the Plc class is updating a Motor value meanwhile the main thread is reading it. The same if I've exposed the Motor class to QML for display some Q_PROPERTY.

The UI has some buttons that if clicked, the main thread checks some Motor's values. In this case, I suppose that there is the risk that Plc class is updating values while the main thread is reading it. It's right?

If the latter is true, is better to protect the Motor class with a mutex that protects the reading and writing of the class, right?

Generally speaking, yes, you need to synchronize access to the shared mutable state for the following reason.

Modern CPUs use multi-layered memory caches to improve performance and some/all of them are per-core. The data doesn't sync between caches unless the CPU encounters a so called memory barrier in the executed code. Memory barrier is either a dedicated CPU instruction or a side-effect of another instruction that causes memory caches to sync.

Keeping this in mind, consider the following case. The GUI thread and Plc thread are scheduled to run on different cores. The Plc thread writes the speed value into the corresponding Motor class variable. There is no memory barrier and it's written only to the corresponding core cache. The GUI thread tries to read the variable before the Plc thread hits a memory barrier and gets only the stale value from the main memory. This is, of course, undesirable.

C++ exposes memory barrier functionality via std::atomic standard library class. Qt also has QAtomic with comparable functionality. It's a dedicated class for the case when you read/write a single variable. In principle, you could declare your speed variable atomic and any access to it would insert a memory barrier into the code.

However, a class variable normally doesn't stand alone and you have certain dependencies with the other class variables and want to see consistent state in all threads. Therefore, you normally would use a mutex since mutices do that and also insert a memory barrier during their operation (their implementation involves an atomic flag).

So, generally speaking, a mutex is necessary.

I say generally because in your particular case

1. the GUI thread doesn't interact with the shared state during the Plc thread execution until the Plc thread finishes working with it

2. the Qt GUI thread event loop needs a memory barrier in order to fetch the next signal activation

So by the time GUI thread examines the motor speed not only the Plc thread stopped working with it, there was also a memory barrier when the GUI thread event loop fetched the Plc::readCompleted signal handler from the event queue. Therefore the GUI thread can "see" the most recent state of the motor.

It means that for this particular case mutex is not necessary. I'd recommend to document that the GUI thread shouldn't interact with the motor until Plc thread finishes since it's too easy for an unwary developer to break this assumption.