fix(atik_efw): append reported fix proposals. (#1339)

TallFurryMan · web-flow · commit 5741bf586cd6 · 2026-06-21T12:04:50.000+03:00
* fix(atik-efw): handle EFW2 status responses

Parse compact position/slot frames observed on hardware, skip FTDI status-only packets, and fall back safely when status remains unavailable.

* refactor(atik-efw): address static analysis findings

Avoid virtual dispatch from the destructor and simplify status frame parsing so bounds are explicit.
diff --git a/indi-atik-efw/CMakeLists.txt b/indi-atik-efw/CMakeLists.txt
@@ -10,7 +10,7 @@ find_package(USB1 REQUIRED)
 find_package(Threads REQUIRED)
 
 set(ATIK_EFW_VERSION_MAJOR 1)
-set(ATIK_EFW_VERSION_MINOR 0)
+set(ATIK_EFW_VERSION_MINOR 1)
 
 configure_file(${CMAKE_CURRENT_SOURCE_DIR}/config.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config.h)
 configure_file(${CMAKE_CURRENT_SOURCE_DIR}/indi_atik_efw.xml.cmake ${CMAKE_CURRENT_BINARY_DIR}/indi_atik_efw.xml)
diff --git a/indi-atik-efw/atik_efw.cpp b/indi-atik-efw/atik_efw.cpp
@@ -25,6 +25,7 @@
 #include "config.h"
 
 #include <algorithm>
+#include <chrono>
 #include <deque>
 #include <iomanip>
 #include <memory>
@@ -53,9 +54,11 @@ constexpr uint16_t kFtdiFlowValue = 0x0303;
 constexpr unsigned int kControlTimeoutMs = 1000;
 constexpr unsigned int kReadTimeoutMs = 1000;
 constexpr unsigned int kWriteTimeoutMs = 1000;
+constexpr int kMaxStatusReadAttempts = 4;
 constexpr useconds_t kUsbResetDelayUs = 1000000;
 constexpr useconds_t kFtdiDelayUs = 200000;
 constexpr useconds_t kStatusDelayUs = 100000;
+constexpr std::chrono::milliseconds kMoveFallbackDelay {1500};
 constexpr int kDefaultSlots = 5;
 constexpr int kMaxSlots = 16;
 constexpr int kMaxResponseBytes = 64;
@@ -75,6 +78,9 @@ std::string bytesToHex(const std::vector<uint8_t> &data)
 
 std::vector<uint8_t> sanitizeResponse(const std::vector<uint8_t> &raw)
 {
+    if (raw.size() == 2)
+        return {};
+
     if (raw.size() >= 3 && raw[0] != kFrameByte && raw[2] == kFrameByte)
         return {raw.begin() + 2, raw.end()};
 
@@ -90,37 +96,59 @@ int decodeByte(uint8_t value)
 
 bool parseStatusResponse(const std::vector<uint8_t> &data, int *position, int *slots)
 {
-    auto start = data.begin();
-    while (start != data.end())
+    std::vector<int> compactValues;
+    size_t start = 0;
+    while (start < data.size())
     {
-        start = std::find(start, data.end(), kFrameByte);
-        if (start == data.end())
-            return false;
+        while (start < data.size() && data[start] != kFrameByte)
+            start++;
+        if (start == data.size())
+            break;
+
+        size_t end = start + 1;
+        while (end < data.size() && data[end] != kFrameByte)
+            end++;
+        if (end == data.size())
+            break;
 
-        auto end = std::find(start + 1, data.end(), kFrameByte);
-        if (end == data.end())
-            return false;
+        size_t payloadSize = end - start - 1;
 
-        if (end - start >= 3 && *(start + 1) == kCmdStatus)
+        // Some wheel firmware returns one frame containing the status command and values.
+        if (payloadSize >= 2 && data[start + 1] == kCmdStatus)
         {
-            int decodedPosition = decodeByte(*(start + 2));
+            int decodedPosition = decodeByte(data[start + 2]);
             if (decodedPosition > 0 && position)
                 *position = decodedPosition;
 
-            if (end - start >= 4)
+            if (payloadSize >= 3)
             {
-                int decodedSlots = decodeByte(*(start + 3));
+                int decodedSlots = decodeByte(data[start + 3]);
                 if (decodedSlots > 0 && slots)
                     *slots = decodedSlots;
             }
 
             return (decodedPosition > 0);
         }
 
+        // EFW2 hardware also returns two compact frames: #position##slot-count#.
+        if (payloadSize == 1)
+        {
+            int value = decodeByte(data[start + 1]);
+            if (value > 0 && value <= kMaxSlots)
+                compactValues.push_back(value);
+        }
+
         start = end + 1;
     }
 
-    return false;
+    if (compactValues.empty())
+        return false;
+
+    if (position)
+        *position = compactValues[0];
+    if (slots && compactValues.size() > 1)
+        *slots = compactValues[1];
+    return true;
 }
 
 bool writeCommand(AtikEfwUsb::DeviceHandle &handle, const std::vector<uint8_t> &command)
@@ -129,18 +157,55 @@ bool writeCommand(AtikEfwUsb::DeviceHandle &handle, const std::vector<uint8_t> &
     return (rc == static_cast<int>(command.size()));
 }
 
-bool readResponse(AtikEfwUsb::DeviceHandle &handle, std::vector<uint8_t> *response)
+enum class StatusReadResult
 {
-    if (!response)
-        return false;
+    NoResponse,
+    Unparsed,
+    Parsed
+};
 
-    unsigned char buffer[kMaxResponseBytes] = {0};
-    int rc = handle.read(kEndpointIn, buffer, sizeof(buffer), kReadTimeoutMs);
-    if (rc <= 0)
-        return false;
+StatusReadResult readStatusResponse(AtikEfwUsb::DeviceHandle &handle, std::vector<uint8_t> *rawResponse,
+                                    int *position, int *slots)
+{
+    std::vector<uint8_t> raw;
+    std::vector<uint8_t> cleaned;
+    auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(kReadTimeoutMs);
 
-    response->assign(buffer, buffer + rc);
-    return true;
+    for (int attempt = 0; attempt < kMaxStatusReadAttempts; attempt++)
+    {
+        auto remaining = std::chrono::duration_cast<std::chrono::milliseconds>(
+                             deadline - std::chrono::steady_clock::now()).count();
+        if (remaining <= 0)
+            break;
+
+        unsigned char buffer[kMaxResponseBytes] = {0};
+        int rc = handle.read(kEndpointIn, buffer, sizeof(buffer), static_cast<unsigned int>(remaining));
+        if (rc <= 0)
+            break;
+
+        std::vector<uint8_t> packet(buffer, buffer + rc);
+        raw.insert(raw.end(), packet.begin(), packet.end());
+
+        auto payload = sanitizeResponse(packet);
+        cleaned.insert(cleaned.end(), payload.begin(), payload.end());
+
+        int parsedPosition = 0;
+        int parsedSlots = 0;
+        if (parseStatusResponse(cleaned, &parsedPosition, &parsedSlots))
+        {
+            if (rawResponse)
+                *rawResponse = std::move(raw);
+            if (position)
+                *position = parsedPosition;
+            if (slots)
+                *slots = parsedSlots;
+            return StatusReadResult::Parsed;
+        }
+    }
+
+    if (rawResponse)
+        *rawResponse = raw;
+    return raw.empty() ? StatusReadResult::NoResponse : StatusReadResult::Unparsed;
 }
 
 bool initializeWheel(AtikEfwUsb::DeviceHandle &handle, std::string *error)
@@ -199,27 +264,18 @@ bool probeStatus(AtikEfwUsb::DeviceHandle &handle, bool requireParse, int *slotC
 
     usleep(kStatusDelayUs);
 
-    std::vector<uint8_t> response;
-    if (!readResponse(handle, &response))
-        return false;
-
-    if (raw)
-        *raw = response;
-
-    auto cleaned = sanitizeResponse(response);
     int slots = 0;
     int position = 0;
-    bool parsed = parseStatusResponse(cleaned, &position, &slots);
-
-    if (parsed)
+    auto result = readStatusResponse(handle, raw, &position, &slots);
+    if (result == StatusReadResult::Parsed)
     {
         if (slotCount)
             *slotCount = slots;
         if (currentSlot)
             *currentSlot = position;
     }
 
-    return parsed || !requireParse;
+    return result == StatusReadResult::Parsed || (!requireParse && result == StatusReadResult::Unparsed);
 }
 
 std::string buildDeviceName(const AtikEfwUsb::DeviceInfo &info, size_t index, size_t count)
@@ -275,10 +331,7 @@ AtikEFW::AtikEFW(const DeviceDescriptor &desc, AtikEfwUsb::Backend &backend)
     setDeviceName(desc.name.c_str());
 }
 
-AtikEFW::~AtikEFW()
-{
-    Disconnect();
-}
+AtikEFW::~AtikEFW() = default;
 
 const char *AtikEFW::getDefaultName()
 {
@@ -402,24 +455,6 @@ bool AtikEFW::sendCommand(const std::vector<uint8_t> &command)
     return true;
 }
 
-bool AtikEFW::readResponse(std::vector<uint8_t> *response)
-{
-    if (!handle_ || !response)
-        return false;
-
-    unsigned char buffer[kMaxResponseBytes] = {0};
-    int rc = handle_->read(kEndpointIn, buffer, sizeof(buffer), kReadTimeoutMs);
-    if (rc <= 0)
-    {
-        LOGF_WARN("%s: no response (%d)", getDeviceName(), rc);
-        return false;
-    }
-
-    response->assign(buffer, buffer + rc);
-    LOGF_DEBUG("%s: raw response %s", getDeviceName(), bytesToHex(*response).c_str());
-    return true;
-}
-
 bool AtikEFW::sendStatus(bool requireParse, int *slotCount, int *currentSlot)
 {
     const std::vector<uint8_t> statusCommand {kFrameByte, kCmdStatus, 0x00, kFrameByte};
@@ -430,17 +465,19 @@ bool AtikEFW::sendStatus(bool requireParse, int *slotCount, int *currentSlot)
     usleep(kStatusDelayUs);
 
     std::vector<uint8_t> response;
-    if (!readResponse(&response))
-        return false;
-
-    auto cleaned = sanitizeResponse(response);
     int slots = 0;
     int position = 0;
-    bool parsed = parseStatusResponse(cleaned, &position, &slots);
+    auto result = readStatusResponse(*handle_, &response, &position, &slots);
+
+    if (!response.empty())
+        LOGF_DEBUG("%s: raw response %s", getDeviceName(), bytesToHex(response).c_str());
 
-    if (!parsed)
+    if (result != StatusReadResult::Parsed)
     {
-        LOGF_WARN("%s: unparsed status response %s", getDeviceName(), bytesToHex(response).c_str());
+        if (result == StatusReadResult::NoResponse)
+            LOGF_WARN("%s: no status response", getDeviceName());
+        else
+            LOGF_WARN("%s: unparsed status response %s", getDeviceName(), bytesToHex(response).c_str());
         return !requireParse;
     }
 
@@ -486,6 +523,8 @@ void AtikEFW::applySlotCount(int slots, bool updateProperty)
 
 bool AtikEFW::Connect()
 {
+    movementPending_ = false;
+
     if (isSimulation())
     {
         int slots = slotCountHint_ > 0 ? slotCountHint_ : kDefaultSlots;
@@ -516,7 +555,7 @@ bool AtikEFW::Connect()
     int detectedPosition = 0;
     if (!sendStatus(false, &detectedSlots, &detectedPosition))
     {
-        LOGF_ERROR("%s: no status response, aborting connection", getDeviceName());
+        LOGF_ERROR("%s: failed to send status command", getDeviceName());
         handle_.reset();
         return false;
     }
@@ -530,6 +569,7 @@ bool AtikEFW::Connect()
     CurrentFilter = (detectedPosition > 0) ? detectedPosition : currentSlotHint_;
     if (CurrentFilter <= 0)
         CurrentFilter = 1;
+    currentSlotHint_ = CurrentFilter;
 
     applySlotCount(slots, true);
 
@@ -546,6 +586,7 @@ bool AtikEFW::Connect()
 
 bool AtikEFW::Disconnect()
 {
+    movementPending_ = false;
     handle_.reset();
     return true;
 }
@@ -585,6 +626,8 @@ bool AtikEFW::SelectFilter(int targetFilter)
         return false;
     }
 
+    movementPending_ = true;
+    movementStartedAt_ = std::chrono::steady_clock::now();
     return true;
 }
 
@@ -601,6 +644,21 @@ int AtikEFW::QueryFilter()
     int position = 0;
     if (!sendStatus(true, &slots, &position))
     {
+        if (movementPending_ && std::chrono::steady_clock::now() - movementStartedAt_ >= kMoveFallbackDelay)
+        {
+            LOGF_WARN("%s: status unavailable after filter change; assuming target slot %d",
+                      getDeviceName(), TargetFilter);
+            CurrentFilter = TargetFilter;
+            currentSlotHint_ = CurrentFilter;
+            movementPending_ = false;
+            FilterSlotNP[0].setValue(CurrentFilter);
+            FilterSlotNP.apply();
+            return CurrentFilter;
+        }
+
+        if (movementPending_)
+            return -1;
+
         FilterSlotNP.setState(IPS_ALERT);
         FilterSlotNP.apply();
         return -1;
@@ -612,6 +670,9 @@ int AtikEFW::QueryFilter()
     if (position > 0)
     {
         CurrentFilter = position;
+        currentSlotHint_ = CurrentFilter;
+        if (CurrentFilter == TargetFilter)
+            movementPending_ = false;
         FilterSlotNP[0].setValue(CurrentFilter);
         FilterSlotNP.apply();
         return CurrentFilter;
diff --git a/indi-atik-efw/atik_efw.h b/indi-atik-efw/atik_efw.h
@@ -26,6 +26,7 @@
 
 #include <indifilterwheel.h>
 
+#include <chrono>
 #include <memory>
 #include <string>
 #include <vector>
@@ -69,14 +70,15 @@ class AtikEFW : public INDI::FilterWheel
         bool configureDevice();
         bool sendStatus(bool requireParse, int *slotCount, int *currentSlot);
         bool sendCommand(const std::vector<uint8_t> &command);
-        bool readResponse(std::vector<uint8_t> *response);
         void applySlotCount(int slots, bool updateProperty);
 
         AtikEfwUsb::Backend &backend_;
         AtikEfwUsb::DeviceInfo deviceInfo_;
         std::unique_ptr<AtikEfwUsb::DeviceHandle> handle_;
         int slotCountHint_ {0};
         int currentSlotHint_ {0};
+        bool movementPending_ {false};
+        std::chrono::steady_clock::time_point movementStartedAt_;
 
         INDI::PropertyNumber SlotCountNP {1};
 };
diff --git a/indi-atik-efw/unit_tests/test_atik_efw.cpp b/indi-atik-efw/unit_tests/test_atik_efw.cpp
