Fix LED-device updates queue up and let Hyperion crash (#1887)

Author: Lord-Grey

CHANGELOG.md

@@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Qt-Grabber (Windows) does not apply pixel ratio (#1882) - _Thanks to @SolberLight_
   - LED-devices are not retrying to establish connectivity, if supported by the device
   - LED-devices are resolving IP-addresses for API and UDP two times in sequence
+  - LED-device updates queue up and let Hyperion crash (#1887)
 
 ---
 

include/leddevice/LedDevice.h

@@ -209,10 +209,10 @@ public slots:
 	///
 	/// @brief Update the color values of the device's LEDs.
 	///
-	/// Handles refreshing of LEDs.
+	/// Updates are skipped while another update is in progress to avoid queueing.
 	///
 	/// @param[in] ledValues The color per LED
-	/// @return Zero on success else negative (i.e. device is not ready)
+	/// @return Zero on success else negative
 	///
 	virtual int updateLeds(std::vector<ColorRgb> ledValues);
 
@@ -426,7 +426,7 @@ public slots:
 	/// @param data ByteArray
 	/// @param number Number of array items to be converted.
 	/// @return array as string of hex values
-	static QString toHex(const QByteArray& data, int number = -1) ;
+	static QString toHex(const QByteArray& data, qsizetype number = -1) ;
 
 	/// Current device's type
 	QString _activeDeviceType;
@@ -508,13 +508,30 @@ protected slots:
 
 private slots:
 
+	///
+	/// @brief Process LED updates requested.
+	///
+	/// @return Zero on success else negative
+	///
+	void processLedUpdate();
+
 	///
 	/// @brief Retry to enable the LED-device
 	///
 	void retryEnable();
 
 private:
 
+	///
+	/// @brief Update the color values of the device's LEDs.
+	///
+	/// Handles refreshing of LEDs.
+	///
+	/// @param[in] ledValues The color per LED
+	/// @return Zero on success else negative (i.e. device is not ready)
+	///
+	int writeLedUpdate(const std::vector<ColorRgb>& ledValues);
+
 	/// @brief Start a new refresh cycle
 	void startRefreshTimer();
 
@@ -542,6 +559,14 @@ private slots:
 
 	/// Last LED values written
 	std::vector<ColorRgb> _lastLedValues;
+
+	// Member variables for the new logic
+	// Use std::atomic for the flag. No mutex needed for it!
+	std::atomic<bool> _isUpdatePending{ false };
+
+	// The mutex now ONLY protects the data buffer.
+	QMutex _ledBufferMutex;
+	std::vector<ColorRgb> _ledUpdateBuffer;
 };
 
 #endif // LEDEVICE_H

libsrc/leddevice/LedDevice.cpp

@@ -309,6 +309,36 @@ void LedDevice::stopEnableAttemptsTimer()
 }
 
 int LedDevice::updateLeds(std::vector<ColorRgb> ledValues)
+{
+	// Take the LED update into a shared buffer and return quickly
+	{
+		QMutexLocker locker(&_ledBufferMutex);
+		_ledUpdateBuffer = std::move(ledValues);
+	}
+
+	// If a frame processing is NOT already scheduled, schedule one.
+	if (!_isUpdatePending.exchange(true))
+	{
+		QTimer::singleShot(0, this, &LedDevice::processLedUpdate);
+	}
+
+	return 0; // Return immediately
+}
+
+void LedDevice::processLedUpdate()
+{
+	std::vector<ColorRgb> valuesToProcess;
+	{
+		QMutexLocker locker(&_ledBufferMutex);
+		valuesToProcess = _ledUpdateBuffer;
+	}
+
+	writeLedUpdate(valuesToProcess);
+
+	_isUpdatePending.store(false);
+}
+
+int LedDevice::writeLedUpdate(const std::vector<ColorRgb>& ledValues)
 {
 	int retval = 0;
 	if (!_isEnabled || !_isOn || !_isDeviceReady || _isDeviceInError)
@@ -318,7 +348,7 @@ int LedDevice::updateLeds(std::vector<ColorRgb> ledValues)
 	}
 	else
 	{
-		qint64 elapsedTimeMs = _lastWriteTime.msecsTo(QDateTime::currentDateTime());
+		qint64 const elapsedTimeMs = _lastWriteTime.msecsTo(QDateTime::currentDateTime());
 		if (_latchTime_ms == 0 || elapsedTimeMs >= _latchTime_ms)
 		{
 			retval = write(ledValues);
@@ -346,6 +376,14 @@ int LedDevice::updateLeds(std::vector<ColorRgb> ledValues)
 
 int LedDevice::rewriteLEDs()
 {
+	bool expected = false;
+	if (!_isUpdatePending.compare_exchange_strong(expected, true))
+	{
+		// The flag was already true, meaning a write from updateLeds/processFrame is currently in progress or scheduled.
+		// We can safely skip this refresh, as the other write will restart the timer.
+		return 0;
+	}
+
 	int retval = -1;
 
 	if (_isEnabled && _isOn && _isDeviceReady && !_isDeviceInError)
@@ -526,7 +564,7 @@ QJsonObject LedDevice::discover(const QJsonObject& /*params*/)
 
 	devicesDiscovered.insert("ledDeviceType", _activeDeviceType);
 
-	QJsonArray deviceList;
+	QJsonArray const deviceList;
 	devicesDiscovered.insert("devices", deviceList);
 
 	Debug(_log, "devicesDiscovered: [%s]", QString(QJsonDocument(devicesDiscovered).toJson(QJsonDocument::Compact)).toUtf8().constData());
@@ -548,7 +586,7 @@ QJsonObject LedDevice::getProperties(const QJsonObject& params)
 
 	QJsonObject properties;
 
-	QJsonObject deviceProperties;
+	QJsonObject const deviceProperties;
 	properties.insert("properties", deviceProperties);
 
 	Debug(_log, "properties: [%s]", QString(QJsonDocument(properties).toJson(QJsonDocument::Compact)).toUtf8().constData());
@@ -633,7 +671,7 @@ void LedDevice::printLedValues(const std::vector<ColorRgb>& ledValues)
 	{
 		std::cout << color;
 	}
-	std::cout << "]" << std::endl;
+	std::cout << "]\n";
 }
 
 QString LedDevice::uint8_t_to_hex_string(const uint8_t* data, const int size, int number)
@@ -643,15 +681,15 @@ QString LedDevice::uint8_t_to_hex_string(const uint8_t* data, const int size, in
 		number = size;
 	}
 
-	QByteArray bytes(reinterpret_cast<const char*>(data), number);
+	QByteArray const bytes(reinterpret_cast<const char*>(data), number);
 #if (QT_VERSION >= QT_VERSION_CHECK(5, 9, 0))
 	return bytes.toHex(':');
 #else
 	return bytes.toHex();
 #endif
 }
 
-QString LedDevice::toHex(const QByteArray& data, int number)
+QString LedDevice::toHex(const QByteArray& data, qsizetype number)
 {
 	if (number <= 0 || number > data.size())
 	{