Updated target register interface with more generic register access & manipulation functions

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

@@ -43,6 +43,7 @@ using Bloom::Targets::TargetMemoryType;
 using Bloom::Targets::TargetMemoryBuffer;
 using Bloom::Targets::TargetRegister;
 using Bloom::Targets::TargetRegisterDescriptor;
+using Bloom::Targets::TargetRegisterDescriptors;
 using Bloom::Targets::TargetRegisterType;
 using Bloom::Targets::TargetRegisters;
 
@@ -714,6 +715,37 @@ void EdbgAvr8Interface::detach() {
     this->targetAttached = false;
 }
 
+TargetRegisters EdbgAvr8Interface::readGeneralPurposeRegisters(const TargetRegisterDescriptors& descriptors) {
+    auto output = TargetRegisters();
+
+    auto gpRegisterStartAddress = this->targetParameters.gpRegisterStartAddress.value_or(0x00);
+    auto registers = this->readMemory(
+        this->configVariant == Avr8ConfigVariant::XMEGA || this->configVariant == Avr8ConfigVariant::UPDI
+        ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM,
+        gpRegisterStartAddress,
+        this->targetParameters.gpRegisterSize.value_or(32)
+    );
+
+    for (const auto& descriptor : descriptors) {
+        assert(descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER);
+
+        /*
+         * All 32 single-byte AVR8 GP registers will be sorted from R0 to R31, so registers[0] == R0,
+         * registers[5] == R5, registers[31] == R31, etc.
+         *
+         * We use the register address to calculate the index.
+         */
+        auto registerIndex = static_cast<std::uint8_t>(descriptor.startAddress.value() - gpRegisterStartAddress);
+        if (registerIndex > (registers.size() - 1)) {
+            throw Exception("Invalid register index deduced from register start address");
+        }
+
+        output.emplace_back(TargetRegister(descriptor,{registers[registerIndex]}));
+    }
+
+    return output;
+}
+
 void EdbgAvr8Interface::activate() {
     if (!this->physicalInterfaceActivated) {
         this->activatePhysical();
@@ -788,7 +820,7 @@ void EdbgAvr8Interface::waitForStoppedEvent() {
     auto breakEvent = this->waitForAvrEvent<BreakEvent>();
 
     if (breakEvent == nullptr) {
-        throw Exception("Failed to receive break event for AVR8 target.");
+        throw Exception("Failed to receive break event for AVR8 target");
     }
 
     this->targetState = TargetState::STOPPED;
@@ -842,71 +874,6 @@ std::uint32_t EdbgAvr8Interface::getProgramCounter() {
     return response.extractProgramCounter();
 }
 
-TargetRegister EdbgAvr8Interface::getStackPointerRegister() {
-    auto stackPointerValue = this->readMemory(
-        Avr8MemoryType::SRAM,
-        this->targetParameters.stackPointerRegisterLowAddress.value(),
-        this->targetParameters.stackPointerRegisterSize.value()
-    );
-
-    /*
-     * The SP low byte comes before the high byte, so the SP is stored in LSB form. We use std::reverse() here to
-     * convert it to MSB.
-     */
-    std::reverse(stackPointerValue.begin(), stackPointerValue.end());
-
-    return TargetRegister(TargetRegisterDescriptor(TargetRegisterType::STACK_POINTER), stackPointerValue);
-}
-
-TargetRegister EdbgAvr8Interface::getStatusRegister() {
-    return TargetRegister(TargetRegisterDescriptor(TargetRegisterType::STATUS_REGISTER), this->readMemory(
-        Avr8MemoryType::SRAM,
-        this->targetParameters.statusRegisterStartAddress.value(),
-        this->targetParameters.statusRegisterSize.value()
-    ));
-}
-
-void EdbgAvr8Interface::setStackPointerRegister(const TargetRegister& stackPointerRegister) {
-    auto maximumStackPointerRegisterSize = this->targetParameters.stackPointerRegisterSize.value();
-    auto registerValue = stackPointerRegister.value;
-
-    if (registerValue.size() > maximumStackPointerRegisterSize) {
-        throw Exception("Provided stack pointer register value exceeds maximum size.");
-
-    } else if (registerValue.size() < maximumStackPointerRegisterSize) {
-        // Fill the missing most-significant bytes with 0x00
-        registerValue.insert(registerValue.begin(), maximumStackPointerRegisterSize - registerValue.size(), 0x00);
-    }
-
-    // Convert SP to LSB
-    std::reverse(registerValue.begin(), registerValue.end());
-
-    this->writeMemory(
-        Avr8MemoryType::SRAM,
-        this->targetParameters.stackPointerRegisterLowAddress.value(),
-        registerValue
-    );
-}
-
-void EdbgAvr8Interface::setStatusRegister(const TargetRegister& statusRegister) {
-    auto maximumStatusRegisterSize = this->targetParameters.statusRegisterSize.value();
-    auto registerValue = statusRegister.value;
-
-    if (registerValue.size() > maximumStatusRegisterSize) {
-        throw Exception("Provided status register value exceeds maximum size.");
-
-    } else if (registerValue.size() < maximumStatusRegisterSize) {
-        // Fill the missing most-significant bytes with 0x00
-        registerValue.insert(registerValue.begin(), maximumStatusRegisterSize - registerValue.size(), 0x00);
-    }
-
-    this->writeMemory(
-        Avr8MemoryType::SRAM,
-        this->targetParameters.statusRegisterStartAddress.value(),
-        registerValue
-    );
-}
-
 void EdbgAvr8Interface::setProgramCounter(std::uint32_t programCounter) {
     if (this->targetState != TargetState::STOPPED) {
         this->stop();
@@ -1182,60 +1149,87 @@ void EdbgAvr8Interface::writeMemory(Avr8MemoryType type, std::uint32_t address,
     }
 }
 
-TargetRegisters EdbgAvr8Interface::readGeneralPurposeRegisters(std::set<std::size_t> registerIds) {
+TargetRegisters EdbgAvr8Interface::readRegisters(const TargetRegisterDescriptors& descriptors) {
-    auto output = TargetRegisters();
+    auto gpRegisterDescriptors = TargetRegisterDescriptors();
-
+    std::copy_if(
-    auto registers = this->readMemory(
+        descriptors.begin(),
-        this->configVariant == Avr8ConfigVariant::XMEGA || this->configVariant == Avr8ConfigVariant::UPDI
+        descriptors.end(),
-                ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM,
+        std::back_inserter(gpRegisterDescriptors),
-        this->targetParameters.gpRegisterStartAddress.value_or(0x00),
+        [](const TargetRegisterDescriptor& descriptor) {
-        this->targetParameters.gpRegisterSize.value_or(32)
+            return descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER;
+        }
     );
 
-    for (std::size_t registerIndex = 0; registerIndex < registers.size(); registerIndex++) {
+    /*
-        if (!registerIds.empty() && registerIds.find(registerIndex) == registerIds.end()) {
+     * If there is more than one GP register to load, we load them via this->readGeneralPurposeRegisters(), in order
+     * to avoid issuing a read call for each GP register.
+     */
+    auto loadGpRegistersSeparately = gpRegisterDescriptors.size() > 1;
+    auto output = loadGpRegistersSeparately ?
+        this->readGeneralPurposeRegisters(gpRegisterDescriptors) : TargetRegisters();
+
+    for (const auto& descriptor : descriptors) {
+        if (loadGpRegistersSeparately && descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER) {
             continue;
         }
 
-        output.push_back(
+        auto registerValue = this->readMemory(
-            TargetRegister(
+            (descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER && (
-                {registerIndex, TargetRegisterType::GENERAL_PURPOSE_REGISTER},
+                    this->configVariant == Avr8ConfigVariant::XMEGA || this->configVariant == Avr8ConfigVariant::UPDI
-                {registers[registerIndex]}
                 )
+            ) ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM,
+            descriptor.startAddress.value(),
+            descriptor.size
         );
+
+        if (registerValue.size() > 1) {
+            // AVR8 registers are stored in LSB form - TargetRegister values should always be MSB
+            std::reverse(registerValue.begin(), registerValue.end());
+        }
+
+        output.emplace_back(TargetRegister(
+            descriptor,
+            registerValue
+        ));
     }
 
     return output;
 }
 
-void EdbgAvr8Interface::writeGeneralPurposeRegisters(const TargetRegisters& registers) {
+void EdbgAvr8Interface::writeRegisters(const Targets::TargetRegisters& registers) {
-    auto gpRegisterSize = this->targetParameters.gpRegisterSize.value_or(32);
+    for (const auto& reg : registers) {
-    auto gpStartAddress = this->targetParameters.gpRegisterStartAddress.value_or(0x00);
+        const auto& registerDescriptor = reg.descriptor;
+        auto registerValue = reg.value;
 
-    for (const auto& gpRegister : registers) {
+        if (registerValue.empty()) {
-        auto descriptor = gpRegister.descriptor;
+            throw Exception("Cannot write empty register value");
-        if (gpRegister.descriptor.type != TargetRegisterType::GENERAL_PURPOSE_REGISTER) {
-            // We are only to update GP registers here
-            throw Exception("Cannot write non GP register");
         }
 
-        if (!descriptor.id.has_value()) {
+        if (registerValue.size() > registerDescriptor.size) {
-            throw Exception("Missing GP register ID");
+            throw Exception("Register value exceeds size specified by register descriptor.");
 
-        } else if ((descriptor.id.value() + 1) > gpRegisterSize || (descriptor.id.value() + 1) < 0) {
+        } else if (registerValue.size() < registerDescriptor.size) {
-            throw Exception("Invalid GP register ID: " + std::to_string(descriptor.id.value()));
+            // Fill the missing most-significant bytes with 0x00
+            registerValue.insert(registerValue.begin(), registerDescriptor.size - registerValue.size(), 0x00);
         }
 
-        if (gpRegister.value.size() != 1) {
+        if (registerValue.size() > 1) {
-            throw Exception("Invalid GP register value size");
+            // AVR8 registers are stored in LSB
+            std::reverse(registerValue.begin(), registerValue.end());
+        }
+
+        auto memoryType = Avr8MemoryType::SRAM;
+        if (registerDescriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER
+            && (this->configVariant == Avr8ConfigVariant::XMEGA || this->configVariant == Avr8ConfigVariant::UPDI)
+        ) {
+            memoryType = Avr8MemoryType::REGISTER_FILE;
         }
 
         // TODO: This can be inefficient when updating many registers, maybe do something a little smarter here.
         this->writeMemory(
-            this->configVariant == Avr8ConfigVariant::XMEGA || this->configVariant == Avr8ConfigVariant::UPDI
+            memoryType,
-                    ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM,
+            registerDescriptor.startAddress.value(),
-            static_cast<std::uint32_t>(gpStartAddress + descriptor.id.value()),
+            registerValue
-            gpRegister.value
         );
     }
 }

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

@@ -433,6 +433,20 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr
          */
         void waitForStoppedEvent();
 
+        /**
+         * Reads the requested general purpose registers.
+         *
+         * When reading numerous GP registers, this function should be used as it reads all 32 registers in a single
+         * call and then returns what's needed. This is much faster than reading each individual GP register.
+         *
+         * readRegisters() will use this when there are numerous GP registers to read.
+         *
+         * @param descriptors
+         *  A collection of GP register descriptors to read. *Do not include non GP register descriptors.*
+         * @return
+         */
+        Targets::TargetRegisters readGeneralPurposeRegisters(const Targets::TargetRegisterDescriptors& descriptors);
+
     public:
         explicit EdbgAvr8Interface(EdbgInterface& edbgInterface)
         : edbgInterface(edbgInterface) {};
@@ -522,34 +536,6 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr
          */
         std::uint32_t getProgramCounter() override;
 
-        /**
-         * Reads the stack pointer register from the target.
-         *
-         * @return
-         */
-        Targets::TargetRegister getStackPointerRegister() override;
-
-        /**
-         * Reads the status register from the target.
-         *
-         * @return
-         */
-        Targets::TargetRegister getStatusRegister() override;
-
-        /**
-         * Updates the stack pointer register on ther target.
-         *
-         * @param stackPointerRegister
-         */
-        void setStackPointerRegister(const Targets::TargetRegister& stackPointerRegister) override;
-
-        /**
-         * Updates the status register on the target.
-         *
-         * @param statusRegister
-         */
-        void setStatusRegister(const Targets::TargetRegister& statusRegister) override;
-
         /**
          * Issues the "PC Write" command to the debug tool, setting the program counter on the target.
          *
@@ -592,19 +578,19 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr
         void clearAllBreakpoints() override;
 
         /**
-         * Reads gernal purpose registers from the target.
+         * Reads registers from the target.
          *
-         * @param registerIds
+         * @param descriptors
          * @return
          */
-        Targets::TargetRegisters readGeneralPurposeRegisters(std::set<std::size_t> registerIds) override;
+        Targets::TargetRegisters readRegisters(const Targets::TargetRegisterDescriptors& descriptors) override;
 
         /**
-         * Writes general purpose registers to target.
+         * Writes registers to target.
          *
          * @param registers
          */
-        void writeGeneralPurposeRegisters(const Targets::TargetRegisters& registers) override;
+        void writeRegisters(const Targets::TargetRegisters& registers) override;
 
         /**
          * This is an overloaded method.

src/DebugToolDrivers/TargetInterfaces/Microchip/AVR/AVR8/Avr8Interface.hpp

@@ -127,20 +127,6 @@ namespace Bloom::DebugToolDrivers::TargetInterfaces::Microchip::Avr::Avr8
          */
         virtual std::uint32_t getProgramCounter() = 0;
 
-        /**
-         * Should retrieve the current stack pointer register value from the target.
-         *
-         * @return
-         */
-        virtual Targets::TargetRegister getStackPointerRegister() = 0;
-
-        /**
-         * Should retrieve the current status register value from the target.
-         *
-         * @return
-         */
-        virtual Targets::TargetRegister getStatusRegister() = 0;
-
         /**
          * Should update the program counter value on the target.
          *
@@ -149,35 +135,21 @@ namespace Bloom::DebugToolDrivers::TargetInterfaces::Microchip::Avr::Avr8
         virtual void setProgramCounter(std::uint32_t programCounter) = 0;
 
         /**
-         * SHould update the stack pointer register value on the target.
+         * Should read the requested registers from the target.
          *
-         * @param stackPointerRegister
+         * @param descriptors
-         */
+         *  A collection of register descriptors, for the registers to be read.
-        virtual void setStackPointerRegister(const Targets::TargetRegister& stackPointerRegister) = 0;
-
-        /**
-         * Should update the status register value on the target.
-         *
-         * @param statusRegister
-         */
-        virtual void setStatusRegister(const Targets::TargetRegister& statusRegister) = 0;
-
-        /**
-         * Should read the requested general purpose register from the target.
-         *
-         * @param registerIds
-         *  A set of register IDs: 0 -> 31
          *
          * @return
          */
-        virtual Targets::TargetRegisters readGeneralPurposeRegisters(std::set<size_t> registerIds) = 0;
+        virtual Targets::TargetRegisters readRegisters(const Targets::TargetRegisterDescriptors& descriptors) = 0;
 
         /**
-         * Should update the value of general purpose registers.
+         * Should update the value of the given registers.
          *
          * @param registers
          */
-        virtual void writeGeneralPurposeRegisters(const Targets::TargetRegisters& registers) = 0;
+        virtual void writeRegisters(const Targets::TargetRegisters& registers) = 0;
 
         /**
          * Should read memory from the target, for the given memory type.

src/Targets/Microchip/AVR/AVR8/Avr8.cpp

@@ -267,20 +267,10 @@ void Avr8::clearAllBreakpoints() {
     this->avr8Interface->clearAllBreakpoints();
 }
 
-TargetRegisters Avr8::readGeneralPurposeRegisters(std::set<std::size_t> registerIds) {
+void Avr8::writeRegisters(TargetRegisters registers) {
-    return this->avr8Interface->readGeneralPurposeRegisters(registerIds);
+    for (auto registerIt = registers.begin(); registerIt != registers.end();) {
-}
+        if (registerIt->descriptor.type == TargetRegisterType::PROGRAM_COUNTER) {
-
+            auto programCounterBytes = registerIt->value;
-void Avr8::writeRegisters(const TargetRegisters& registers) {
-    TargetRegisters gpRegisters;
-
-    for (const auto& targetRegister : registers) {
-        if (targetRegister.descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER
-            && targetRegister.descriptor.id.has_value()) {
-            gpRegisters.push_back(targetRegister);
-
-        } else if (targetRegister.descriptor.type == TargetRegisterType::PROGRAM_COUNTER) {
-            auto programCounterBytes = targetRegister.value;
 
             if (programCounterBytes.size() < 4) {
                 // All PC register values should be at least 4 bytes in size
@@ -294,41 +284,37 @@ void Avr8::writeRegisters(const TargetRegisters& registers) {
                 | programCounterBytes[3]
             ));
 
-        } else if (targetRegister.descriptor.type == TargetRegisterType::STATUS_REGISTER) {
+            registerIt = registers.erase(registerIt);
-            this->avr8Interface->setStatusRegister(targetRegister);
 
-        } else if (targetRegister.descriptor.type == TargetRegisterType::STACK_POINTER) {
+        } else {
-            this->avr8Interface->setStackPointerRegister(targetRegister);
+            registerIt++;
         }
     }
 
-    if (!gpRegisters.empty()) {
+    if (!registers.empty()) {
-        this->avr8Interface->writeGeneralPurposeRegisters(gpRegisters);
+        this->avr8Interface->writeRegisters(registers);
     }
 }
 
-TargetRegisters Avr8::readRegisters(const TargetRegisterDescriptors& descriptors) {
+TargetRegisters Avr8::readRegisters(TargetRegisterDescriptors descriptors) {
     TargetRegisters registers;
-    std::set<std::size_t> gpRegisterIds;
 
-    for (const auto& descriptor : descriptors) {
+    for (auto registerDescriptorIt = descriptors.begin(); registerDescriptorIt != descriptors.end();) {
-        if (descriptor.type == TargetRegisterType::GENERAL_PURPOSE_REGISTER && descriptor.id.has_value()) {
+        auto& descriptor = *registerDescriptorIt;
-            gpRegisterIds.insert(descriptor.id.value());
 
-        } else if (descriptor.type == TargetRegisterType::PROGRAM_COUNTER) {
+        if (descriptor.type == TargetRegisterType::PROGRAM_COUNTER) {
             registers.push_back(this->getProgramCounterRegister());
 
-        } else if (descriptor.type == TargetRegisterType::STATUS_REGISTER) {
+            registerDescriptorIt = descriptors.erase(registerDescriptorIt);
-            registers.push_back(this->getStatusRegister());
 
-        } else if (descriptor.type == TargetRegisterType::STACK_POINTER) {
+        } else {
-            registers.push_back(this->getStackPointerRegister());
+            registerDescriptorIt++;
         }
     }
 
-    if (!gpRegisterIds.empty()) {
+    if (!descriptors.empty()) {
-        auto gpRegisters = this->readGeneralPurposeRegisters(gpRegisterIds);
+        auto otherRegisters = this->avr8Interface->readRegisters(descriptors);
-        registers.insert(registers.end(), gpRegisters.begin(), gpRegisters.end());
+        registers.insert(registers.end(), otherRegisters.begin(), otherRegisters.end());
     }
 
     return registers;
@@ -361,14 +347,6 @@ TargetRegister Avr8::getProgramCounterRegister() {
     });
 }
 
-TargetRegister Avr8::getStackPointerRegister() {
-    return this->avr8Interface->getStackPointerRegister();
-}
-
-TargetRegister Avr8::getStatusRegister() {
-    return this->avr8Interface->getStatusRegister();
-}
-
 void Avr8::setProgramCounter(std::uint32_t programCounter) {
     this->avr8Interface->setProgramCounter(programCounter);
 }

src/Targets/Microchip/AVR/AVR8/Avr8.hpp

@@ -124,9 +124,8 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit
         void removeBreakpoint(std::uint32_t address) override;
         void clearAllBreakpoints() override;
 
-        TargetRegisters readGeneralPurposeRegisters(std::set<std::size_t> registerIds) override;
+        void writeRegisters(TargetRegisters registers) override;
-        void writeRegisters(const TargetRegisters& registers) override;
+        TargetRegisters readRegisters(TargetRegisterDescriptors descriptors) override;
-        TargetRegisters readRegisters(const TargetRegisterDescriptors& descriptors) override;
 
         TargetMemoryBuffer readMemory(
             TargetMemoryType memoryType,
@@ -142,12 +141,9 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit
         TargetState getState() override;
 
         std::uint32_t getProgramCounter() override;
-        TargetRegister getProgramCounterRegister() override;
+        TargetRegister getProgramCounterRegister();
         void setProgramCounter(std::uint32_t programCounter) override;
 
-        TargetRegister getStackPointerRegister() override;
-        TargetRegister getStatusRegister() override;
-
         std::map<int, TargetPinState> getPinStates(int variantId) override;
         void setPinState(
             int variantId,

src/Targets/Target.hpp

@@ -223,21 +223,12 @@ namespace Bloom::Targets
          */
         virtual void clearAllBreakpoints() = 0;
 
-        /**
-         * Should read general purpose register values, for the given general purpose register ids.
-         *
-         * @param registerIds
-         *
-         * @return
-         */
-        virtual TargetRegisters readGeneralPurposeRegisters(std::set<std::size_t> registerIds) = 0;
-
         /**
          * Should update the value of the given registers.
          *
          * @param registers
          */
-        virtual void writeRegisters(const TargetRegisters& registers) = 0;
+        virtual void writeRegisters(TargetRegisters registers) = 0;
 
         /**
          * Should read register values of the registers described by the given descriptors.
@@ -246,7 +237,7 @@ namespace Bloom::Targets
          *
          * @return
          */
-        virtual TargetRegisters readRegisters(const TargetRegisterDescriptors& descriptors) = 0;
+        virtual TargetRegisters readRegisters(TargetRegisterDescriptors descriptors) = 0;
 
         /**
          * Should read memory from the target.
@@ -282,13 +273,6 @@ namespace Bloom::Targets
          */
         virtual std::uint32_t getProgramCounter() = 0;
 
-        /**
-         * Same as Target::getProgramCounter() but in the form of a TargetRegister.
-         *
-         * @return
-         */
-        virtual TargetRegister getProgramCounterRegister() = 0;
-
         /**
          * Should update the program counter on the target.
          *
@@ -296,20 +280,6 @@ namespace Bloom::Targets
          */
         virtual void setProgramCounter(std::uint32_t programCounter) = 0;
 
-        /**
-         * Should fetch the status register value.
-         *
-         * @return
-         */
-        virtual TargetRegister getStatusRegister() = 0;
-
-        /**
-         * Should fetch the stack pointer register value.
-         *
-         * @return
-         */
-        virtual TargetRegister getStackPointerRegister() = 0;
-
         /**
          * Should get the current pin states for each pin on the target, mapped by pin number
          *