From 698ecbcd53ab0b0cdda096f4a41ac8b679fb19c1 Mon Sep 17 00:00:00 2001
From: Thomas Basler <thomas@familie-basler.net>
Date: Mon, 31 Jul 2023 21:54:08 +0200
Subject: [PATCH] Fix: Prevent wrong values of alarm data because of non-atomic
 transaction and fix calculation of getEntryCount()

---
 .../src/commands/AlarmDataCommand.cpp         |  2 ++
 lib/Hoymiles/src/parser/AlarmLogParser.cpp    | 29 +++++++++++++++++++
 lib/Hoymiles/src/parser/AlarmLogParser.h      |  7 ++++-
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/lib/Hoymiles/src/commands/AlarmDataCommand.cpp b/lib/Hoymiles/src/commands/AlarmDataCommand.cpp
index 46b62e7c..574e0be2 100644
--- a/lib/Hoymiles/src/commands/AlarmDataCommand.cpp
+++ b/lib/Hoymiles/src/commands/AlarmDataCommand.cpp
@@ -27,11 +27,13 @@ bool AlarmDataCommand::handleResponse(InverterAbstract* inverter, fragment_t fra
 
     // Move all fragments into target buffer
     uint8_t offs = 0;
+    inverter->EventLog()->beginAppendFragment();
     inverter->EventLog()->clearBuffer();
     for (uint8_t i = 0; i < max_fragment_id; i++) {
         inverter->EventLog()->appendFragment(offs, fragment[i].fragment, fragment[i].len);
         offs += (fragment[i].len);
     }
+    inverter->EventLog()->endAppendFragment();
     inverter->EventLog()->setLastAlarmRequestSuccess(CMD_OK);
     inverter->EventLog()->setLastUpdate(millis());
     return true;
diff --git a/lib/Hoymiles/src/parser/AlarmLogParser.cpp b/lib/Hoymiles/src/parser/AlarmLogParser.cpp
index 536c412d..bad5ccfd 100644
--- a/lib/Hoymiles/src/parser/AlarmLogParser.cpp
+++ b/lib/Hoymiles/src/parser/AlarmLogParser.cpp
@@ -86,6 +86,18 @@ const std::array<const AlarmMessage_t, ALARM_MSG_COUNT> AlarmLogParser::_alarmMe
     { AlarmMessageType_t::ALL, 9000, "Microinverter is suspected of being stolen" },
 } };
 
+#define HOY_SEMAPHORE_TAKE() \
+    do {                     \
+    } while (xSemaphoreTake(_xSemaphore, portMAX_DELAY) != pdPASS)
+#define HOY_SEMAPHORE_GIVE() xSemaphoreGive(_xSemaphore)
+
+AlarmLogParser::AlarmLogParser()
+    : Parser()
+{
+    _xSemaphore = xSemaphoreCreateMutex();
+    HOY_SEMAPHORE_GIVE(); // release before first use
+}
+
 void AlarmLogParser::clearBuffer()
 {
     memset(_payloadAlarmLog, 0, ALARM_LOG_PAYLOAD_SIZE);
@@ -102,8 +114,21 @@ void AlarmLogParser::appendFragment(uint8_t offset, uint8_t* payload, uint8_t le
     _alarmLogLength += len;
 }
 
+void AlarmLogParser::beginAppendFragment()
+{
+    HOY_SEMAPHORE_TAKE();
+}
+
+void AlarmLogParser::endAppendFragment()
+{
+    HOY_SEMAPHORE_GIVE();
+}
+
 uint8_t AlarmLogParser::getEntryCount()
 {
+    if (_alarmLogLength < 2) {
+        return 0;
+    }
     return (_alarmLogLength - 2) / ALARM_LOG_ENTRY_SIZE;
 }
 
@@ -128,6 +153,8 @@ void AlarmLogParser::getLogEntry(uint8_t entryId, AlarmLogEntry_t* entry)
 
     int timezoneOffset = getTimezoneOffset();
 
+    HOY_SEMAPHORE_TAKE();
+
     uint32_t wcode = (uint16_t)_payloadAlarmLog[entryStartOffset] << 8 | _payloadAlarmLog[entryStartOffset + 1];
     uint32_t startTimeOffset = 0;
     if (((wcode >> 13) & 0x01) == 1) {
@@ -143,6 +170,8 @@ void AlarmLogParser::getLogEntry(uint8_t entryId, AlarmLogEntry_t* entry)
     entry->StartTime = (((uint16_t)_payloadAlarmLog[entryStartOffset + 4] << 8) | ((uint16_t)_payloadAlarmLog[entryStartOffset + 5])) + startTimeOffset + timezoneOffset;
     entry->EndTime = ((uint16_t)_payloadAlarmLog[entryStartOffset + 6] << 8) | ((uint16_t)_payloadAlarmLog[entryStartOffset + 7]);
 
+    HOY_SEMAPHORE_GIVE();
+
     if (entry->EndTime > 0) {
         entry->EndTime += (endTimeOffset + timezoneOffset);
     }
diff --git a/lib/Hoymiles/src/parser/AlarmLogParser.h b/lib/Hoymiles/src/parser/AlarmLogParser.h
index 5e5faf87..5c37589d 100644
--- a/lib/Hoymiles/src/parser/AlarmLogParser.h
+++ b/lib/Hoymiles/src/parser/AlarmLogParser.h
@@ -31,8 +31,11 @@ typedef struct {
 
 class AlarmLogParser : public Parser {
 public:
+    AlarmLogParser();
     void clearBuffer();
     void appendFragment(uint8_t offset, uint8_t* payload, uint8_t len);
+    void beginAppendFragment();
+    void endAppendFragment();
 
     uint8_t getEntryCount();
     void getLogEntry(uint8_t entryId, AlarmLogEntry_t* entry);
@@ -46,11 +49,13 @@ private:
     static int getTimezoneOffset();
 
     uint8_t _payloadAlarmLog[ALARM_LOG_PAYLOAD_SIZE];
-    uint8_t _alarmLogLength;
+    uint8_t _alarmLogLength = 0;
 
     LastCommandSuccess _lastAlarmRequestSuccess = CMD_NOK; // Set to NOK to fetch at startup
 
     AlarmMessageType_t _messageType = AlarmMessageType_t::ALL;
 
     static const std::array<const AlarmMessage_t, ALARM_MSG_COUNT> _alarmMessages;
+
+    SemaphoreHandle_t _xSemaphore;
 };
\ No newline at end of file