Dramatically improved speed of register reading in the AVR8 EDBG driver

This will accommodate Insight's eager loading of target registers via the new TargetRegistersSidePaneWidget

CMakeLists.txt

@@ -88,6 +88,7 @@ add_executable(Bloom
     src/DebugToolDrivers/Protocols/CMSIS-DAP/Response.cpp
     src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/AvrCommand.cpp
     src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/CommandFrames/AvrCommandFrame.cpp
+    src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/CommandFrames/AVR8Generic/ReadMemory.cpp
     src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/AvrResponse.cpp
     src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/ResponseFrames/AvrResponseFrame.cpp
     src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/AvrEvent.cpp

src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/Avr8Generic.hpp

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <map>
+
 #include "src/Targets/Microchip/AVR/AVR8/PhysicalInterface.hpp"
 
 namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr

src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/CommandFrames/AVR8Generic/ReadMemory.cpp

@@ -0,0 +1,55 @@
+#include "ReadMemory.hpp"
+
+#include <bitset>
+
+using namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr::CommandFrames::Avr8Generic;
+
+std::vector<unsigned char> ReadMemory::getPayload() const {
+    /*
+     * The read memory command consists of 11/11 + (this->bytes / 8) bytes:
+     * 1. Command ID (0x21 or 0x22, for reading with a mask)
+     * 2. Version (0x00)
+     * 3. Memory type (Avr8MemoryType)
+     * 4. Start address (4 bytes)
+     * 5. Number of bytes to read (4 bytes)
+     * 6. Mask to apply (this->bytes / 8)
+     */
+    auto output = std::vector<unsigned char>(11, 0x00);
+    output[0] = this->excludedAddresses.empty() ? 0x21 : 0x22;
+    output[1] = 0x00;
+    output[2] = static_cast<unsigned char>(this->type);
+    output[3] = static_cast<unsigned char>(this->address);
+    output[4] = static_cast<unsigned char>(this->address >> 8);
+    output[5] = static_cast<unsigned char>(this->address >> 16);
+    output[6] = static_cast<unsigned char>(this->address >> 24);
+    output[7] = static_cast<unsigned char>(this->bytes);
+    output[8] = static_cast<unsigned char>(this->bytes >> 8);
+    output[9] = static_cast<unsigned char>(this->bytes >> 16);
+    output[10] = static_cast<unsigned char>(this->bytes >> 24);
+
+    if (!this->excludedAddresses.empty()) {
+        auto const endAddress = this->address + (this->bytes - 1);
+
+        constexpr auto byteBitSize = std::numeric_limits<unsigned char>::digits;
+        auto byteBitset = std::bitset<byteBitSize>();
+        byteBitset.reset();
+
+        for (std::uint32_t address = this->address; address <= endAddress; address++) {
+            auto addressIndex = address - this->address;
+            auto bitIndex = static_cast<std::size_t>(
+                addressIndex - (std::floor(addressIndex / byteBitSize) * byteBitSize)
+            );
+
+            if (!this->excludedAddresses.contains(address)) {
+                byteBitset[bitIndex] = 1;
+            }
+
+            if (address > 0 && (bitIndex == 7 || address == endAddress)) {
+                output.emplace_back(byteBitset.to_ulong());
+                byteBitset.reset();
+            }
+        }
+    }
+
+    return output;
+}

src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/CommandFrames/AVR8Generic/ReadMemory.hpp

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <cstdint>
+#include <set>
 
 #include "Avr8GenericCommandFrame.hpp"
 #include "../../ResponseFrames/AVR8Generic/ReadMemory.hpp"
@@ -13,6 +14,7 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr::CommandFrames
         Avr8MemoryType type = Avr8MemoryType::SRAM;
         std::uint32_t address = 0;
         std::uint32_t bytes = 0;
+        std::set<std::uint32_t> excludedAddresses;
 
     public:
         using ResponseFrameType = ResponseFrames::Avr8Generic::ReadMemory;
@@ -31,29 +33,10 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr::CommandFrames
             this->bytes = bytes;
         }
 
-        [[nodiscard]] std::vector<unsigned char> getPayload() const override {
+        void setExcludedAddresses(const std::set<std::uint32_t>& excludedAddresses) {
-            /*
+            this->excludedAddresses = excludedAddresses;
-             * The read memory command consists of 11 bytes:
-             * 1. Command ID (0x21)
-             * 2. Version (0x00)
-             * 3. Memory type (Avr8MemoryType)
-             * 4. Start address (4 bytes)
-             * 5. Number of bytes to read (4 bytes)
-             */
-            auto output = std::vector<unsigned char>(11, 0x00);
-            output[0] = 0x21;
-            output[1] = 0x00;
-            output[2] = static_cast<unsigned char>(this->type);
-            output[3] = static_cast<unsigned char>(this->address);
-            output[4] = static_cast<unsigned char>(this->address >> 8);
-            output[5] = static_cast<unsigned char>(this->address >> 16);
-            output[6] = static_cast<unsigned char>(this->address >> 24);
-            output[7] = static_cast<unsigned char>(this->bytes);
-            output[8] = static_cast<unsigned char>(this->bytes >> 8);
-            output[9] = static_cast<unsigned char>(this->bytes >> 16);
-            output[10] = static_cast<unsigned char>(this->bytes >> 24);
-
-            return output;
         }
+
+        [[nodiscard]] std::vector<unsigned char> getPayload() const override;
     };
 }

src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp

@@ -126,23 +126,6 @@ void EdbgAvr8Interface::setDebugWireAndJtagParameters() {
         );
     }
 
-    /*
-     * All addresses for registers that reside in the mapped IO memory segment include the mapped IO segment offset
-     * (start address). But the EDBG protocol requires *some* of these addresses to be stripped of this offset before
-     * sending them as target parameters.
-     *
-     * This applies to the following addresses:
-     *
-     *  - OSCALL Address
-     *  - EEARL Address
-     *  - EEARH Address
-     *  - EECR Address
-     *  - EEDR Address
-     *
-     * It *doesn't* seem to apply to the SPMCR address.
-     */
-    auto mappedIoStartAddress = this->targetParameters.mappedIoSegmentStartAddress.value_or(0);
-
     if (this->targetParameters.ocdDataRegister.has_value()) {
         Logger::debug("Setting DEVICE_OCD_DATA_REGISTER AVR8 parameter");
         this->setParameter(
@@ -159,6 +142,31 @@ void EdbgAvr8Interface::setDebugWireAndJtagParameters() {
         );
     }
 
+    if (this->targetParameters.bootSectionStartAddress.has_value()) {
+        Logger::debug("Setting DEVICE_BOOT_START_ADDR AVR8 parameter");
+        this->setParameter(
+            Avr8EdbgParameters::DEVICE_BOOT_START_ADDR,
+            this->targetParameters.bootSectionStartAddress.value()
+        );
+    }
+
+    /*
+     * All addresses for registers that reside in the mapped IO memory segment include the mapped IO segment offset
+     * (start address). But the EDBG protocol requires *some* of these addresses to be stripped of this offset before
+     * sending them as target parameters.
+     *
+     * This applies to the following addresses:
+     *
+     *  - OSCALL Address
+     *  - EEARL Address
+     *  - EEARH Address
+     *  - EECR Address
+     *  - EEDR Address
+     *
+     * It *doesn't* seem to apply to the SPMCR or OCDDR address.
+     */
+    auto mappedIoStartAddress = this->targetParameters.mappedIoSegmentStartAddress.value_or(0);
+
     if (this->targetParameters.osccalAddress.has_value()) {
         Logger::debug("Setting DEVICE_OSCCAL_ADDR AVR8 parameter");
         this->setParameter(
@@ -208,14 +216,6 @@ void EdbgAvr8Interface::setDebugWireAndJtagParameters() {
                 )
         );
     }
-
-    if (this->targetParameters.bootSectionStartAddress.has_value()) {
-        Logger::debug("Setting DEVICE_BOOT_START_ADDR AVR8 parameter");
-        this->setParameter(
-            Avr8EdbgParameters::DEVICE_BOOT_START_ADDR,
-            this->targetParameters.bootSectionStartAddress.value()
-        );
-    }
 }
 
 void EdbgAvr8Interface::setPdiParameters() {
@@ -988,7 +988,12 @@ TargetState EdbgAvr8Interface::getTargetState() {
     return this->targetState;
 }
 
-TargetMemoryBuffer EdbgAvr8Interface::readMemory(Avr8MemoryType type, std::uint32_t address, std::uint32_t bytes) {
+TargetMemoryBuffer EdbgAvr8Interface::readMemory(
+    Avr8MemoryType type,
+    std::uint32_t address,
+    std::uint32_t bytes,
+    std::set<std::uint32_t> excludedAddresses
+) {
     if (type == Avr8MemoryType::FLASH_PAGE) {
         if (this->targetParameters.flashPageSize.value_or(0) < 1) {
             throw Exception("Missing/invalid flash page size parameter");
@@ -1064,7 +1069,7 @@ TargetMemoryBuffer EdbgAvr8Interface::readMemory(Avr8MemoryType type, std::uint3
              * Now that the start address and bytes to read have been aligned, we can simply invoke this function
              * for a second time, with the aligned values. Then, return the requested data and discard the rest.
              */
-            auto memoryBuffer = this->readMemory(type, alignedAddress, alignedBytesToRead);
+            auto memoryBuffer = this->readMemory(type, alignedAddress, alignedBytesToRead, excludedAddresses);
             return TargetMemoryBuffer(
                 memoryBuffer.begin() + (address - alignedAddress),
                 memoryBuffer.begin() + (address - alignedAddress) + bytes
@@ -1113,8 +1118,13 @@ TargetMemoryBuffer EdbgAvr8Interface::readMemory(Avr8MemoryType type, std::uint3
 
             for (float i = 1; i <= totalReadsRequired; i++) {
                 auto bytesToRead = static_cast<std::uint32_t>((bytes - output.size()) > (singlePacketSize * 2) ?
-                                                              (singlePacketSize * 2) : bytes - output.size());
+                    (singlePacketSize * 2) : bytes - output.size());
-                auto data = this->readMemory(type, static_cast<std::uint32_t>(address + output.size()), bytesToRead);
+                auto data = this->readMemory(
+                    type,
+                    static_cast<std::uint32_t>(address + output.size()),
+                    bytesToRead,
+                    excludedAddresses
+                );
                 output.insert(output.end(), data.begin(), data.end());
             }
 
@@ -1126,6 +1136,7 @@ TargetMemoryBuffer EdbgAvr8Interface::readMemory(Avr8MemoryType type, std::uint3
     commandFrame.setType(type);
     commandFrame.setAddress(address);
     commandFrame.setBytes(bytes);
+    commandFrame.setExcludedAddresses(excludedAddresses);
 
     auto response = this->edbgInterface.sendAvrCommandFrameAndWaitForResponseFrame(commandFrame);
     if (response.getResponseId() == Avr8ResponseId::FAILED) {
@@ -1153,47 +1164,116 @@ void EdbgAvr8Interface::writeMemory(Avr8MemoryType type, std::uint32_t address,
 }
 
 TargetRegisters EdbgAvr8Interface::readRegisters(const TargetRegisterDescriptors& descriptors) {
-    auto gpRegisterDescriptors = TargetRegisterDescriptors();
+    auto output = TargetRegisters();
-    std::copy_if(
+
-        descriptors.begin(),
+    // Group descriptors by type and resolve the address range for each type
-        descriptors.end(),
+    auto descriptorsByType = std::map<TargetRegisterType, std::set<const TargetRegisterDescriptor*>>();
-        std::back_inserter(gpRegisterDescriptors),
-        [](const TargetRegisterDescriptor& descriptor) {
-            return descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER;
-        }
-    );
 
     /*
-     * If there is more than one GP register to load, we load them via this->readGeneralPurposeRegisters(), in order
+     * An address range is just an std::pair of integers - the first being the start address, the second being the
-     * to avoid issuing a read call for each GP register.
+     * end address.
      */
-    auto loadGpRegistersSeparately = gpRegisterDescriptors.size() > 1;
+    using AddressRange = std::pair<std::uint32_t, std::uint32_t>;
-    auto output = loadGpRegistersSeparately ?
+    auto addressRangeByType = std::map<TargetRegisterType, AddressRange>();
-        this->readGeneralPurposeRegisters(gpRegisterDescriptors) : TargetRegisters();
 
     for (const auto& descriptor : descriptors) {
-        if (loadGpRegistersSeparately && descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER) {
+        if (!descriptor.startAddress.has_value()) {
+            Logger::debug(
+                "Attempted to read register in the absence of a start address - register name: "
+                    + descriptor.name.value_or("unknown")
+            );
             continue;
         }
 
-        auto registerValue = this->readMemory(
+        descriptorsByType[descriptor.type].insert(&descriptor);
-            (descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER && (
+
-                    this->configVariant == Avr8ConfigVariant::XMEGA || this->configVariant == Avr8ConfigVariant::UPDI
+        const auto startAddress = descriptor.startAddress.value();
-                )
+        const auto endAddress = startAddress + (descriptor.size - 1);
-            ) ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM,
+
-            descriptor.startAddress.value(),
+        if (!addressRangeByType.contains(descriptor.type)) {
-            descriptor.size
+            auto addressRange = AddressRange();
+            addressRange.first = startAddress;
+            addressRange.second = endAddress;
+            addressRangeByType[descriptor.type] = addressRange;
+
+        } else {
+            auto& addressRange = addressRangeByType[descriptor.type];
+
+            if (startAddress < addressRange.first) {
+                addressRange.first = startAddress;
+            }
+
+            if (endAddress > addressRange.second) {
+                addressRange.second = endAddress;
+            }
+        }
+    }
+
+    /*
+     * Now that we have our address ranges and grouped descriptors, we can perform a single read call
+     * for each register type.
+     */
+    for (const auto& [registerType, descriptors] : descriptorsByType) {
+        const auto& addressRange = addressRangeByType[registerType];
+        const auto startAddress = addressRange.first;
+        const auto endAddress = addressRange.second;
+        const auto bufferSize = (endAddress - startAddress) + 1;
+
+        const auto memoryType = (registerType != TargetRegisterType::GENERAL_PURPOSE_REGISTER) ? Avr8MemoryType::SRAM : (
+            this->configVariant == Avr8ConfigVariant::XMEGA || this->configVariant == Avr8ConfigVariant::UPDI
+                ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM
         );
 
-        if (registerValue.size() > 1) {
+        /*
-            // AVR8 registers are stored in LSB form - TargetRegister values should always be MSB
+         * When reading the entire range, we must avoid any attempts to access the OCD data register (OCDDR), as the
-            std::reverse(registerValue.begin(), registerValue.end());
+         * debug tool will reject the command and respond with a 0x36 error code (invalid address error).
+         *
+         * For this reason, we specify the OCDDR address as an excluded address. This will mean
+         * the EdbgAvr8Interface::readMemory() function will employ the masked read memory command, as opposed to the
+         * general read memory command. The masked read memory command allows us to specify which addresses to
+         * read and which ones to ignore. For ignored addresses, the debug tool will just return a 0x00 byte.
+         * For more info, see section 7.1.22 titled 'Memory Read Masked', in the EDBG protocol document.
+         *
+         * Interestingly, the masked read memory command doesn't seem to require us to explicitly specify the OCDDR
+         * address as an excluded address. It seems to exclude the OCDDR automatically, even if we've not
+         * instructed it to do so. This is plausible, as we send the OCDDR address to the debug tool during target
+         * initialisation (see EdbgAvr8Interface::setDebugWireAndJtagParameters()). So this means we don't have to
+         * specify the OCDDR address as an excluded address, but the EdbgAvr8Interface::readMemory() function will
+         * only employ the masked read memory command when we supply at least one excluded address. For this reason,
+         * we still pass the OCDDR address to EdbgAvr8Interface::readMemory(), as an excluded address (provided we
+         * have it).
+         *
+         * See CommandFrames::Avr8Generic::ReadMemory(); and the Microchip EDBG documentation for more.
+         */
+        auto excludedAddresses = std::set<std::uint32_t>();
+        if (this->targetParameters.ocdDataRegister.has_value()) {
+            excludedAddresses.insert(this->targetParameters.ocdDataRegister.value());
         }
 
-        output.emplace_back(TargetRegister(
+        auto flatMemoryBuffer = this->readMemory(
-            descriptor,
+            memoryType,
-            registerValue
+            startAddress,
-        ));
+            bufferSize,
+            excludedAddresses
+        );
+
+        if (flatMemoryBuffer.size() != bufferSize) {
+            throw Exception(
+                "Failed to read memory within register type address range (" + std::to_string(startAddress)
+                    + " - " + std::to_string(endAddress) +"). Expected " + std::to_string(bufferSize)
+                    + " bytes, got " + std::to_string(flatMemoryBuffer.size())
+            );
+        }
+
+        // Construct our TargetRegister objects directly from the flat memory buffer
+        for (const auto& descriptor : descriptors) {
+            const auto bufferStartIt = flatMemoryBuffer.begin() + (descriptor->startAddress.value() - startAddress);
+
+            output.emplace_back(
+                TargetRegister(*descriptor,
+                TargetMemoryBuffer(bufferStartIt, bufferStartIt + descriptor->size))
+            );
+        }
     }
 
     return output;

src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.hpp

@@ -356,9 +356,18 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr
          * @param bytes
          *  Number of bytes to access.
          *
+         * @param excludedAddresses
+         *  A set of addresses to exclude from the read operation. This is used to read memory ranges that could
+         *  involve accessing an illegal address, like the OCDDR address.
+         *
          * @return
          */
-        Targets::TargetMemoryBuffer readMemory(Avr8MemoryType type, std::uint32_t address, std::uint32_t bytes);
+        Targets::TargetMemoryBuffer readMemory(
+            Avr8MemoryType type,
+            std::uint32_t address,
+            std::uint32_t bytes,
+            std::set<std::uint32_t> excludedAddresses = {}
+        );
 
         /**
          * Writes memory to the target.