From 9aeb179edc17ba3169e6f3d2d109ca27ff4ac55d Mon Sep 17 00:00:00 2001
From: "D.R.racer" <drracer@drracer.eu>
Date: Thu, 8 Sep 2022 10:35:36 +0200
Subject: [PATCH] Fix computation of CRC for Write requests (non-zero value2)

---
 src/modules/protocol.cpp                    | 10 +++++++++-
 tests/unit/application/test_application.cpp | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/modules/protocol.cpp b/src/modules/protocol.cpp
index d051598..7c6068d 100644
--- a/src/modules/protocol.cpp
+++ b/src/modules/protocol.cpp
@@ -259,7 +259,15 @@ DecodeStatus Protocol::DecodeResponse(uint8_t c) {
 }
 
 uint8_t Protocol::EncodeResponseCmdAR(const RequestMsg &msg, ResponseMsgParamCodes ar, uint8_t *txbuff) {
-    const ResponseMsg rsp(msg, ar, 0); // this needs some cleanup @@TODO - check assembly how bad is it
+    // BEWARE:
+    // ResponseMsg rsp(RequestMsg(msg.code, msg.value), ar, 0);
+    // ... is NOT the same as:
+    // ResponseMsg rsp(msg, ar, 0);
+    // ... because of the usually unused parameter value2 (which only comes non-zero in write requests).
+    // It took me a few hours to find out why the CRC from the MMU never matched all the other sides (unit tests and the MK3S)
+    // It is because this was the only place where the original request kept its value2 non-zero.
+    // In the response, we must make sure value2 is actually zero unless being sent along with it (which is not right now)
+    const ResponseMsg rsp(RequestMsg(msg.code, msg.value), ar, 0); // this needs some cleanup @@TODO - check assembly how bad is it
     uint8_t i = BeginEncodeRequest(rsp.request, txbuff);
     txbuff[i] = (uint8_t)ar;
     ++i;
diff --git a/tests/unit/application/test_application.cpp b/tests/unit/application/test_application.cpp
index 43cc5c1..ffb8b7c 100644
--- a/tests/unit/application/test_application.cpp
+++ b/tests/unit/application/test_application.cpp
@@ -2,6 +2,7 @@
 #include "application.h"
 #include <stdint.h>
 #include "../modules/stubs/stub_serial.h"
+#include "modules/protocol.h"
 
 TEST_CASE("application::ReportVersion", "[application]") {
     modules::serial::ClearRX();
@@ -11,3 +12,22 @@ TEST_CASE("application::ReportVersion", "[application]") {
 
     REQUIRE(modules::serial::tx == "S0 A2*37\n");
 }
+
+TEST_CASE("application::WriteRegister", "[application]") {
+    modules::serial::ClearRX();
+    modules::serial::ClearTX();
+
+    application.ReportWriteRegister(mp::RequestMsg(mp::RequestMsgCodes::Write, 0x1b, 2));
+
+    REQUIRE(modules::serial::tx == "W1b A*c2\n");
+
+    {
+        modules::protocol::Protocol p;
+        // verify decoding of the message - it seems we are having issues with it on the MK3 side for unknown reason
+        // The MMU sends: W1c A*f0\n for some very strange reason, even though the unit tests compute a different CRC with the same piece of code!
+        // Found it, see explanation in Protocol::EncodeResponseCmdAR...
+        for (uint8_t i = 0; i < modules::serial::tx.size(); ++i) {
+            CHECK(p.DecodeResponse(modules::serial::tx[i]) != modules::protocol::DecodeStatus::Error);
+        }
+    }
+}