AVR8 EDBG driver changes:

- Concealing pending software breakpoint operations
- Injecting active software breakpoints for memory types that filter them out
- Some tidying

src/DebugToolDrivers/Microchip/Protocols/Edbg/Avr/EdbgAvr8Interface.cpp

@@ -4,6 +4,7 @@
 #include <cassert>
 #include <cmath>
 #include <algorithm>
+#include <array>
 
 #include "src/Services/PathService.hpp"
 #include "src/Services/StringService.hpp"
@@ -80,15 +81,18 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
     using CommandFrames::Avr8Generic::DisableDebugWire;
 
     using Targets::TargetAddressSpaceDescriptor;
+    using Targets::TargetMemorySegmentDescriptor;
     using Targets::TargetMemorySegmentType;
     using Targets::TargetExecutionState;
     using Targets::TargetPhysicalInterface;
     using Targets::TargetMemoryBuffer;
     using Targets::TargetMemoryAddress;
+    using Targets::TargetMemoryAddressRange;
     using Targets::TargetMemorySize;
     using Targets::TargetRegisterDescriptor;
     using Targets::TargetRegisterDescriptors;
     using Targets::TargetRegisterDescriptorAndValuePairs;
+    using Targets::TargetProgramBreakpoint;
 
     EdbgAvr8Interface::EdbgAvr8Interface(
         EdbgInterface* edbgInterface,
@@ -169,6 +173,7 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         }
 
         this->cachedExecutionState = TargetExecutionState::RUNNING;
+        this->commitPendingBreakpointOperations();
     }
 
     void EdbgAvr8Interface::runTo(TargetMemoryAddress address) {
@@ -180,6 +185,7 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         }
 
         this->cachedExecutionState = TargetExecutionState::RUNNING;
+        this->commitPendingBreakpointOperations();
     }
 
     void EdbgAvr8Interface::step() {
@@ -189,6 +195,7 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         }
 
         this->cachedExecutionState = TargetExecutionState::STEPPING;
+        this->commitPendingBreakpointOperations();
     }
 
     void EdbgAvr8Interface::reset() {
@@ -360,80 +367,34 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         return responseFrame.extractSignature(this->session.targetConfig.physicalInterface);
     }
 
-    void EdbgAvr8Interface::setSoftwareBreakpoint(TargetMemoryAddress address) {
-        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
-            SetSoftwareBreakpoints{{address}}
-        );
+    void EdbgAvr8Interface::setProgramBreakpoint(const TargetProgramBreakpoint& breakpoint) {
+        if (breakpoint.type == TargetProgramBreakpoint::Type::HARDWARE) {
+            this->setHardwareBreakpoint(breakpoint.address);
 
-        if (responseFrame.id == Avr8ResponseId::FAILED) {
-            throw Avr8CommandFailure{"AVR8 Set software breakpoint command failed", responseFrame};
-        }
-    }
-
-    void EdbgAvr8Interface::clearSoftwareBreakpoint(TargetMemoryAddress address) {
-        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
-            ClearSoftwareBreakpoints{{address}}
-        );
-
-        if (responseFrame.id == Avr8ResponseId::FAILED) {
-            throw Avr8CommandFailure{"AVR8 Clear software breakpoint command failed", responseFrame};
-        }
-    }
-
-    void EdbgAvr8Interface::setHardwareBreakpoint(TargetMemoryAddress address) {
-        static const auto getAvailableBreakpointNumbers = [this] () {
-            auto breakpointNumbers = std::set<std::uint8_t>{1, 2, 3};
-
-            for (const auto& [address, allocatedNumber] : this->hardwareBreakpointNumbersByAddress) {
-                breakpointNumbers.erase(allocatedNumber);
+        } else {
+            if (this->pendingSoftwareBreakpointInsertions.contains(breakpoint)) {
+                return;
             }
 
-            return breakpointNumbers;
-        };
-
-        if (this->hardwareBreakpointNumbersByAddress.contains(address)) {
-            Logger::debug(
-                "Hardware breakpoint already installed for byte address 0x" + Services::StringService::toHex(address)
-                    + " - ignoring request"
-            );
-            return;
+            this->setSoftwareBreakpoint(breakpoint.address);
+            this->pendingSoftwareBreakpointInsertions.insert(breakpoint);
+            this->pendingSoftwareBreakpointDeletions.remove(breakpoint);
         }
-
-        const auto availableBreakpointNumbers = getAvailableBreakpointNumbers();
-
-        if (availableBreakpointNumbers.empty()) {
-            throw Exception{"Maximum hardware breakpoints have been allocated"};
-        }
-
-        const auto breakpointNumber = *(availableBreakpointNumbers.begin());
-
-        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
-            SetHardwareBreakpoint{address, breakpointNumber}
-        );
-
-        if (responseFrame.id == Avr8ResponseId::FAILED) {
-            throw Avr8CommandFailure{"AVR8 Set hardware breakpoint command failed", responseFrame};
-        }
-
-        this->hardwareBreakpointNumbersByAddress.emplace(address, breakpointNumber);
     }
 
-    void EdbgAvr8Interface::clearHardwareBreakpoint(TargetMemoryAddress address) {
-        const auto breakpointNumberIt = this->hardwareBreakpointNumbersByAddress.find(address);
-        if (breakpointNumberIt == this->hardwareBreakpointNumbersByAddress.end()) {
-            Logger::error("No hardware breakpoint at byte address 0x" + Services::StringService::toHex(address));
-            return;
+    void EdbgAvr8Interface::removeProgramBreakpoint(const TargetProgramBreakpoint& breakpoint) {
+        if (breakpoint.type == TargetProgramBreakpoint::Type::HARDWARE) {
+            this->clearHardwareBreakpoint(breakpoint.address);
+
+        } else {
+            if (this->pendingSoftwareBreakpointDeletions.contains(breakpoint)) {
+                return;
+            }
+
+            this->clearSoftwareBreakpoint(breakpoint.address);
+            this->pendingSoftwareBreakpointDeletions.insert(breakpoint);
+            this->pendingSoftwareBreakpointInsertions.remove(breakpoint);
         }
-
-        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
-            ClearHardwareBreakpoint{breakpointNumberIt->second}
-        );
-
-        if (responseFrame.id == Avr8ResponseId::FAILED) {
-            throw Avr8CommandFailure{"AVR8 Clear hardware breakpoint command failed", responseFrame};
-        }
-
-        this->hardwareBreakpointNumbersByAddress.erase(address);
     }
 
     TargetRegisterDescriptorAndValuePairs EdbgAvr8Interface::readRegisters(
@@ -628,8 +589,8 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
     }
 
     TargetMemoryBuffer EdbgAvr8Interface::readMemory(
-        const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
-        const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+        const TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+        const TargetMemorySegmentDescriptor& memorySegmentDescriptor,
         TargetMemoryAddress startAddress,
         TargetMemorySize bytes,
         const std::set<Targets::TargetMemoryAddressRange>& excludedAddressRanges
@@ -659,12 +620,30 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         }
 
         if (memorySegmentDescriptor.type == TargetMemorySegmentType::FLASH) {
+            const auto injectAndConcealBreakpoints = [&] (TargetMemoryBuffer&& data) -> TargetMemoryBuffer&& {
+                this->injectActiveBreakpoints(
+                    addressSpaceDescriptor,
+                    memorySegmentDescriptor,
+                    data,
+                    startAddress
+                );
+                this->concealPendingBreakpointOperations(
+                    addressSpaceDescriptor,
+                    memorySegmentDescriptor,
+                    data,
+                    startAddress
+                );
+                return std::move(data);
+            };
+
             if (this->session.configVariant == Avr8ConfigVariant::MEGAJTAG) {
-                return this->readMemory(
-                    this->programmingModeEnabled ? Avr8MemoryType::FLASH_PAGE : Avr8MemoryType::SPM,
-                    startAddress,
-                    bytes,
-                    excludedAddresses
+                return injectAndConcealBreakpoints(
+                    this->readMemory(
+                        this->programmingModeEnabled ? Avr8MemoryType::FLASH_PAGE : Avr8MemoryType::SPM,
+                        startAddress,
+                        bytes,
+                        excludedAddresses
+                    )
                 );
             }
 
@@ -675,11 +654,13 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
                      * When using the BOOT_FLASH memory type, the address should be relative to the start of the
                      * boot section.
                      */
-                    return this->readMemory(
-                        Avr8MemoryType::BOOT_FLASH,
-                        startAddress - bootSectionStartAddress,
-                        bytes,
-                        excludedAddresses
+                    return injectAndConcealBreakpoints(
+                        this->readMemory(
+                            Avr8MemoryType::BOOT_FLASH,
+                            startAddress - bootSectionStartAddress,
+                            bytes,
+                            excludedAddresses
+                        )
                     );
                 }
 
@@ -687,15 +668,19 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
                  * When using the APPL_FLASH memory type, the address should be relative to the start of the
                  * application section.
                  */
-                return this->readMemory(
-                    Avr8MemoryType::APPL_FLASH,
-                    startAddress - this->session.programAppSection.value().get().startAddress,
-                    bytes,
-                    excludedAddresses
+                return injectAndConcealBreakpoints(
+                    this->readMemory(
+                        Avr8MemoryType::APPL_FLASH,
+                        startAddress - this->session.programAppSection.value().get().startAddress,
+                        bytes,
+                        excludedAddresses
+                    )
                 );
             }
 
-            return this->readMemory(Avr8MemoryType::FLASH_PAGE, startAddress, bytes, excludedAddresses);
+            return injectAndConcealBreakpoints(
+                this->readMemory(Avr8MemoryType::FLASH_PAGE, startAddress, bytes, excludedAddresses)
+            );
         }
 
         if (memorySegmentDescriptor.type == TargetMemorySegmentType::EEPROM) {
@@ -742,8 +727,8 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
     }
 
     void EdbgAvr8Interface::writeMemory(
-        const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
-        const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+        const TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+        const TargetMemorySegmentDescriptor& memorySegmentDescriptor,
         TargetMemoryAddress startAddress,
         Targets::TargetMemoryBufferSpan buffer
     ) {
@@ -772,8 +757,6 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
                         buffer
                     );
                 }
-
-                return this->writeMemory(Avr8MemoryType::FLASH_PAGE, startAddress, buffer);
             }
 
             return this->writeMemory(Avr8MemoryType::FLASH_PAGE, startAddress, buffer);
@@ -895,6 +878,9 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
          * To avoid this issue, we send the 'clear all software breakpoints' command to the tool, just before
          * entering programming mode. That command will clear all breakpoints immediately, preventing program
          * memory corruption at the next flow control command.
+         *
+         * The TargetController will reinsert all breakpoints at the end of a programming session, so the breakpoints
+         * that we clear here will be restored.
          */
         Logger::debug("Clearing all software breakpoints in preparation for programming mode");
         this->clearAllSoftwareBreakpoints();
@@ -1232,6 +1218,82 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         this->targetAttached = false;
     }
 
+    void EdbgAvr8Interface::setSoftwareBreakpoint(TargetMemoryAddress address) {
+        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
+            SetSoftwareBreakpoints{{address}}
+        );
+
+        if (responseFrame.id == Avr8ResponseId::FAILED) {
+            throw Avr8CommandFailure{"AVR8 Set software breakpoint command failed", responseFrame};
+        }
+    }
+
+    void EdbgAvr8Interface::clearSoftwareBreakpoint(TargetMemoryAddress address) {
+        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
+            ClearSoftwareBreakpoints{{address}}
+        );
+
+        if (responseFrame.id == Avr8ResponseId::FAILED) {
+            throw Avr8CommandFailure{"AVR8 Clear software breakpoint command failed", responseFrame};
+        }
+    }
+
+    void EdbgAvr8Interface::setHardwareBreakpoint(TargetMemoryAddress address) {
+        static const auto getAvailableBreakpointNumbers = [this] () {
+            auto breakpointNumbers = std::set<std::uint8_t>{1, 2, 3};
+
+            for (const auto& [address, allocatedNumber] : this->hardwareBreakpointNumbersByAddress) {
+                breakpointNumbers.erase(allocatedNumber);
+            }
+
+            return breakpointNumbers;
+        };
+
+        if (this->hardwareBreakpointNumbersByAddress.contains(address)) {
+            Logger::debug(
+                "Hardware breakpoint already installed for byte address 0x" + Services::StringService::toHex(address)
+                    + " - ignoring request"
+            );
+            return;
+        }
+
+        const auto availableBreakpointNumbers = getAvailableBreakpointNumbers();
+
+        if (availableBreakpointNumbers.empty()) {
+            throw Exception{"Maximum hardware breakpoints have been allocated"};
+        }
+
+        const auto breakpointNumber = *(availableBreakpointNumbers.begin());
+
+        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
+            SetHardwareBreakpoint{address, breakpointNumber}
+        );
+
+        if (responseFrame.id == Avr8ResponseId::FAILED) {
+            throw Avr8CommandFailure{"AVR8 Set hardware breakpoint command failed", responseFrame};
+        }
+
+        this->hardwareBreakpointNumbersByAddress.emplace(address, breakpointNumber);
+    }
+
+    void EdbgAvr8Interface::clearHardwareBreakpoint(TargetMemoryAddress address) {
+        const auto breakpointNumberIt = this->hardwareBreakpointNumbersByAddress.find(address);
+        if (breakpointNumberIt == this->hardwareBreakpointNumbersByAddress.end()) {
+            Logger::error("No hardware breakpoint at byte address 0x" + Services::StringService::toHex(address));
+            return;
+        }
+
+        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
+            ClearHardwareBreakpoint{breakpointNumberIt->second}
+        );
+
+        if (responseFrame.id == Avr8ResponseId::FAILED) {
+            throw Avr8CommandFailure{"AVR8 Clear hardware breakpoint command failed", responseFrame};
+        }
+
+        this->hardwareBreakpointNumbersByAddress.erase(address);
+    }
+
     void EdbgAvr8Interface::clearAllSoftwareBreakpoints() {
         const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
             ClearAllSoftwareBreakpoints{}
@@ -1240,6 +1302,10 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         if (responseFrame.id == Avr8ResponseId::FAILED) {
             throw Avr8CommandFailure{"AVR8 Clear all software breakpoints command failed", responseFrame};
         }
+
+        this->activeSoftwareBreakpoints.clear();
+        this->pendingSoftwareBreakpointInsertions.clear();
+        this->pendingSoftwareBreakpointDeletions.clear();
     }
 
     void EdbgAvr8Interface::clearAllBreakpoints() {
@@ -1251,6 +1317,164 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         }
     }
 
+    void EdbgAvr8Interface::injectActiveBreakpoints(
+        const TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+        const TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+        TargetMemoryBuffer& data,
+        TargetMemoryAddress startAddress
+    ) {
+        if (data.empty()) {
+            return;
+        }
+
+        for (const auto& [addressSpaceId, breakpointsByAddress] : this->activeSoftwareBreakpoints) {
+            if (addressSpaceId != addressSpaceDescriptor.id) {
+                continue;
+            }
+
+            for (const auto& [address, breakpoint] : breakpointsByAddress) {
+                this->injectBreakOpcodeAtBreakpoint(
+                    breakpoint,
+                    addressSpaceDescriptor,
+                    memorySegmentDescriptor,
+                    data,
+                    startAddress
+                );
+            }
+        }
+    }
+
+    void EdbgAvr8Interface::concealPendingBreakpointOperations(
+        const TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+        const TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+        TargetMemoryBuffer& data,
+        TargetMemoryAddress startAddress
+    ) {
+        if (data.empty()) {
+            return;
+        }
+
+        for (const auto& [addressSpaceId, breakpointsByAddress] : this->pendingSoftwareBreakpointInsertions) {
+            if (addressSpaceId != addressSpaceDescriptor.id) {
+                continue;
+            }
+
+            for (const auto& [address, breakpoint] : breakpointsByAddress) {
+                this->injectBreakOpcodeAtBreakpoint(
+                    breakpoint,
+                    addressSpaceDescriptor,
+                    memorySegmentDescriptor,
+                    data,
+                    startAddress
+                );
+            }
+        }
+
+        for (const auto& [addressSpaceId, breakpointsByAddress] : this->pendingSoftwareBreakpointDeletions) {
+            if (addressSpaceId != addressSpaceDescriptor.id) {
+                continue;
+            }
+
+            for (const auto& [address, breakpoint] : breakpointsByAddress) {
+                this->injectOriginalOpcodeAtBreakpoint(
+                    breakpoint,
+                    addressSpaceDescriptor,
+                    memorySegmentDescriptor,
+                    data,
+                    startAddress
+                );
+            }
+        }
+    }
+
+    void EdbgAvr8Interface::commitPendingBreakpointOperations() {
+        for (const auto& [addressSpaceId, breakpointsByAddress] : this->pendingSoftwareBreakpointInsertions) {
+            for (const auto& [address, breakpoint] : breakpointsByAddress) {
+                this->activeSoftwareBreakpoints.insert(breakpoint);
+            }
+        }
+
+        for (const auto& [addressSpaceId, breakpointsByAddress] : this->pendingSoftwareBreakpointDeletions) {
+            for (const auto& [address, breakpoint] : breakpointsByAddress) {
+                this->activeSoftwareBreakpoints.remove(breakpoint);
+            }
+        }
+
+        this->pendingSoftwareBreakpointInsertions.clear();
+        this->pendingSoftwareBreakpointDeletions.clear();
+    }
+
+    void EdbgAvr8Interface::injectBreakOpcodeAtBreakpoint(
+        const Targets::TargetProgramBreakpoint& breakpoint,
+        const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+        const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+        TargetMemoryBuffer& data,
+        Targets::TargetMemoryAddress startAddress
+    ) {
+        static constexpr auto breakOpcode = std::to_array<unsigned char>({0x98, 0x95});
+        assert(breakpoint.size == breakOpcode.size());
+
+        const auto addressRange = TargetMemoryAddressRange{
+            startAddress,
+            static_cast<TargetMemoryAddress>(startAddress + (data.size() / addressSpaceDescriptor.unitSize) - 1)
+        };
+
+        const auto breakpointRange = TargetMemoryAddressRange{
+            breakpoint.address,
+            breakpoint.address + breakpoint.size - 1
+        };
+
+        const auto intersectingSize = addressRange.intersectingSize(breakpointRange);
+        if (intersectingSize == 0) {
+            return;
+        }
+
+        const auto addressOffset = breakpointRange.startAddress < addressRange.startAddress
+            ? addressRange.startAddress - breakpointRange.startAddress
+            : 0;
+        const auto breakOpcodeOffset = breakOpcode.begin() + addressOffset;
+
+        std::copy(
+            breakOpcodeOffset,
+            breakOpcodeOffset + intersectingSize,
+            data.begin() + (breakpointRange.startAddress - addressRange.startAddress + addressOffset)
+        );
+    }
+
+    void EdbgAvr8Interface::injectOriginalOpcodeAtBreakpoint(
+        const Targets::TargetProgramBreakpoint& breakpoint,
+        const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+        const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+        TargetMemoryBuffer& data,
+        Targets::TargetMemoryAddress startAddress
+    ) {
+        const auto addressRange = TargetMemoryAddressRange{
+            startAddress,
+            static_cast<TargetMemoryAddress>(startAddress + (data.size() / addressSpaceDescriptor.unitSize) - 1)
+        };
+
+        const auto breakpointRange = TargetMemoryAddressRange{
+            breakpoint.address,
+            breakpoint.address + breakpoint.size - 1
+        };
+
+        const auto intersectingSize = addressRange.intersectingSize(breakpointRange);
+        if (intersectingSize == 0) {
+            return;
+        }
+
+        const auto addressOffset = breakpointRange.startAddress < addressRange.startAddress
+            ? addressRange.startAddress - breakpointRange.startAddress
+            : 0;
+        const auto originalDataOffset = breakpoint.originalData.begin() + addressOffset;
+
+        std::copy(
+            originalDataOffset,
+            originalDataOffset + intersectingSize,
+            data.begin() + (breakpointRange.startAddress - addressRange.startAddress + addressOffset)
+        );
+    }
+
     std::unique_ptr<AvrEvent> EdbgAvr8Interface::getAvrEvent() {
         auto event = this->edbgInterface->requestAvrEvent();
 
@@ -1510,7 +1734,6 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         }
 
         auto data = responseFrame.getMemoryData();
-
         if (data.size() != bytes) {
             throw Avr8CommandFailure{"Unexpected number of bytes returned from EDBG debug tool"};
         }

src/DebugToolDrivers/Microchip/Protocols/Edbg/Avr/EdbgAvr8Interface.hpp

@@ -14,6 +14,7 @@
 
 #include "src/Targets/TargetPhysicalInterface.hpp"
 #include "src/Targets/TargetMemory.hpp"
+#include "src/Targets/ProgramBreakpointRegistry.hpp"
 #include "src/Targets/TargetRegisterDescriptor.hpp"
 #include "src/Targets/Microchip/Avr8/TargetDescriptionFile.hpp"
 #include "src/Targets/Microchip/Avr8/Family.hpp"
@@ -65,8 +66,8 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
          * memory command that exceeds 256 bytes in the size of the read request, despite the fact that the HID report
          * size is 512 bytes. The debug tool doesn't report any error, it just returns incorrect data.
          *
-         * To address this, debug tool drivers can set a soft limit on the number of bytes this EdbgAvr8Interface instance
-         * will attempt to access in a single request, using the EdbgAvr8Interface::setMaximumMemoryAccessSizePerRequest()
+         * To address this, debug tool drivers can set a soft limit on the number of bytes this EdbgAvr8Interface
+         * instance will attempt to access in a single request, using the EdbgAvr8Interface::setMaximumMemoryAccessSizePerRequest()
          * member function.
          *
          * This limit will be enforced in all forms of memory access on the AVR8 target, including register access.
@@ -91,51 +92,15 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
          * See the comments in that class for more info on the expected behaviour of each method.
          */
 
-        /**
-         * Initialises the AVR8 Generic protocol interface by setting the appropriate parameters on the debug tool.
-         */
         void init() override;
 
-        /**
-         * Issues the "stop" command to the debug tool, halting target execution.
-         */
         void stop() override;
-
-        /**
-         * Issues the "run" command to the debug tool, resuming execution on the target.
-         */
         void run() override;
-
-        /**
-         * Issues the "run to" command to the debug tool, resuming execution on the target, up to a specific byte
-         * address. The target will dispatch an AVR BREAK event once it reaches the specified address.
-         *
-         * @param address
-         *  The (byte) address to run to.
-         */
         void runTo(Targets::TargetMemoryAddress address) override;
-
-        /**
-         * Issues the "step" command to the debug tool, stepping the execution on the target. The stepping can be
-         * configured to step in, out or over. But currently we only support stepping in. The target will dispatch
-         * an AVR BREAK event once it reaches the next instruction.
-         */
         void step() override;
-
-        /**
-         * Issues the "reset" command to the debug tool, resetting target execution.
-         */
         void reset() override;
 
-        /**
-         * Activates the physical interface and starts a debug session on the target (via attach()).
-         */
         void activate() override;
-
-        /**
-         * Terminates any active debug session on the target and severs the connection between the debug tool and
-         * the target (by deactivating the physical interface).
-         */
         void deactivate() override;
 
         void applyAccessRestrictions(
@@ -143,92 +108,19 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
             const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor
         ) override;
 
-        /**
-         * Issues the "PC Read" command to the debug tool, to extract the current program counter.
-         *
-         * @return
-         */
         Targets::TargetMemoryAddress getProgramCounter() override;
-
-        /**
-         * Issues the "PC Write" command to the debug tool, setting the program counter on the target.
-         *
-         * @param programCounter
-         *  The byte address to set as the program counter.
-         */
         void setProgramCounter(Targets::TargetMemoryAddress programCounter) override;
 
-        /**
-         * Issues the "Get ID" command to the debug tool, to extract the signature from the target.
-         *
-         * @return
-         */
         Targets::Microchip::Avr8::TargetSignature getDeviceId() override;
 
-        /**
-         * Issues the "Software Breakpoint Set" command to the debug tool, setting a software breakpoint at the given
-         * byte address.
-         *
-         * @param address
-         *  The byte address to place the breakpoint.
-         */
-        void setSoftwareBreakpoint(Targets::TargetMemoryAddress address) override;
+        virtual void setProgramBreakpoint(const Targets::TargetProgramBreakpoint& breakpoint) override;
+        virtual void removeProgramBreakpoint(const Targets::TargetProgramBreakpoint& breakpoint) override;
 
-        /**
-         * Issues the "Software Breakpoint Clear" command to the debug tool, clearing any software breakpoint at the
-         * given byte address.
-         *
-         * @param address
-         *  The byte address of the breakpoint to clear.
-         */
-        void clearSoftwareBreakpoint(Targets::TargetMemoryAddress address) override;
-
-        /**
-         * Issues the "Hardware Breakpoint Set" command to the debug tool, setting a hardware breakpoint at the given
-         * byte address.
-         *
-         * @param address
-         *  The byte address to place the breakpoint.
-         */
-        void setHardwareBreakpoint(Targets::TargetMemoryAddress address) override;
-
-        /**
-         * Issues the "Hardware Breakpoint Clear" command to the debug tool, clearing any hardware breakpoint at the
-         * given byte address.
-         *
-         * @param address
-         *  The byte address of the breakpoint to clear.
-         */
-        void clearHardwareBreakpoint(Targets::TargetMemoryAddress address) override;
-
-        /**
-         * Reads registers from the target.
-         *
-         * @param descriptorIds
-         * @return
-         */
         Targets::TargetRegisterDescriptorAndValuePairs readRegisters(
             const Targets::TargetRegisterDescriptors& descriptors
         ) override;
-
-        /**
-         * Writes registers to target.
-         *
-         * @param registers
-         */
         void writeRegisters(const Targets::TargetRegisterDescriptorAndValuePairs& registers) override;
 
-        /**
-         * This is an overloaded method.
-         *
-         * Resolves the correct Avr8MemoryType from the given TargetMemoryType and calls readMemory().
-         *
-         * @param addressSpaceDescriptor
-         * @param memorySegmentDescriptor
-         * @param startAddress
-         * @param bytes
-         * @return
-         */
         Targets::TargetMemoryBuffer readMemory(
             const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
             const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
@@ -236,17 +128,6 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
             Targets::TargetMemorySize bytes,
             const std::set<Targets::TargetMemoryAddressRange>& excludedAddressRanges = {}
         ) override;
-
-        /**
-         * This is an overloaded method.
-         *
-         * Resolves the correct Avr8MemoryType from the given TargetMemoryType and calls writeMemory().
-         *
-         * @param addressSpaceDescriptor
-         * @param memorySegmentDescriptor
-         * @param startAddress
-         * @param buffer
-         */
         void writeMemory(
             const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
             const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
@@ -254,86 +135,57 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
             Targets::TargetMemoryBufferSpan buffer
         ) override;
 
-        /**
-         * Issues the "Erase" command to erase a particular section of program memory.
-         *
-         * @param section
-         */
         void eraseProgramMemory(
             std::optional<Targets::Microchip::Avr8::ProgramMemorySection> section = std::nullopt
         ) override;
-
-        /**
-         * Issues the "Erase" command to erase the entire chip.
-         */
         void eraseChip() override;
 
-        /**
-         * Returns the current state of the target.
-         *
-         * @return
-         */
         Targets::TargetExecutionState getExecutionState() override;
 
-        /**
-         * Enters programming mode on the EDBG debug tool.
-         */
         void enableProgrammingMode() override;
-
-        /**
-         * Leaves programming mode on the EDBG debug tool.
-         */
         void disableProgrammingMode() override;
 
     private:
-        /**
-         * The AVR8 Generic protocol is a sub-protocol of the EDBG AVR protocol, which is served via CMSIS-DAP vendor
-         * commands.
-         *
-         * Every EDBG based debug tool that utilises this implementation must provide access to its EDBG interface.
-         */
         EdbgInterface* edbgInterface = nullptr;
-
-        /**
-         * The active EDBG AVR8 session.
-         */
         EdbgAvr8Session session;
 
-        /**
-         * See the comment for EdbgAvr8Interface::setAvoidMaskedMemoryRead().
-         */
         bool avoidMaskedMemoryRead = true;
-
-        /**
-         * See the comment for EdbgAvr8Interface::setMaximumMemoryAccessSizePerRequest().
-         */
         std::optional<Targets::TargetMemorySize> maximumMemoryAccessSizePerRequest;
-
         bool reactivateJtagTargetPostProgrammingMode = false;
-
-        /**
-         * We keep record of the current target execution state for caching purposes.
-         *
-         * We'll only refresh the target state if the target is running. If it has already stopped, then we assume it
-         * cannot transition to a running state without an instruction from us.
-         *
-         * @TODO: Review this. Is the above assumption correct? Always? Explore the option of polling the target state.
-         */
         Targets::TargetExecutionState cachedExecutionState = Targets::TargetExecutionState::UNKNOWN;
-
-        /**
-         * Upon configuration, the physical interface must be activated on the debug tool. We keep record of this to
-         * assist in our decision to deactivate the physical interface, when deactivate() is called.
-         */
         bool physicalInterfaceActivated = false;
+        bool targetAttached = false;
+        bool programmingModeEnabled = false;
 
         /**
-         * As with activating the physical interface, we are required to issue the "attach" command to the debug
-         * tool, in order to start a debug session in the target.
+         * The TargetController refreshes the relevant program memory cache after inserting/removing software
+         * breakpoints. So it expects the insertion/removal operation to take place immediately. But, EDBG tools do
+         * not appear to support this. They queue the insertion/removal of software breakpoints until the next
+         * program flow control command.
+         *
+         * To get around this, we keep track of the pending operations and intercept program memory access operations
+         * to inject the relevant instruction opcodes, so that it appears as if the breakpoints have been
+         * inserted/removed. In other words: we fake it - we make it seem as if the breakpoints were inserted/removed
+         * immediately, when in actuality, the operation is still pending.
+         *
+         * It's horrible, but I can't think of a better way. Yes, I have considered loosening the restriction on the
+         * target driver, but I dislike that approach even more, because it would mean making the rest of Bloom
+         * accommodate the "I'll do it when I feel like it" nature of the EDBG tool, which is something I really
+         * don't want to do.
+         *
+         * This also addresses another potential issue, where EDBG tools will filter out software breakpoints when
+         * accessing program memory via some memory types. This is undesired behaviour. We negate it by injecting
+         * BREAK opcodes at the addresses of all active software breakpoints, when accessing program memory via any
+         * memory type.
+         *
+         * For more, see the following member functions:
+         *   - EdbgAvr8Interface::injectActiveBreakpoints()
+         *   - EdbgAvr8Interface::concealPendingBreakpointOperations()
+         *   - EdbgAvr8Interface::commitPendingBreakpointOperations()
          */
-        bool targetAttached = false;
-
-        bool programmingModeEnabled = false;
+        Targets::ProgramBreakpointRegistry pendingSoftwareBreakpointInsertions;
+        Targets::ProgramBreakpointRegistry pendingSoftwareBreakpointDeletions;
+        Targets::ProgramBreakpointRegistry activeSoftwareBreakpoints;
 
         /**
          * Every hardware breakpoint is assigned a "breakpoint number", which we need to keep track of in order to
@@ -341,38 +193,11 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
          */
         std::map<Targets::TargetMemoryAddress, std::uint8_t> hardwareBreakpointNumbersByAddress;
 
-        /**
-         * Sends the necessary target parameters to the debug tool.
-         *
-         * @param config
-         */
         void setTargetParameters();
-
-        /**
-         * Sets an AVR8 parameter on the debug tool. See the Avr8EdbgParameters class and protocol documentation
-         * for more on available parameters.
-         *
-         * @param parameter
-         * @param value
-         */
         void setParameter(const Avr8EdbgParameter& parameter, const std::vector<unsigned char>& value);
-
-        /**
-         * Overload for setting parameters with single byte values.
-         *
-         * @param parameter
-         * @param value
-         */
         void setParameter(const Avr8EdbgParameter& parameter, std::uint8_t value) {
             this->setParameter(parameter, std::vector<unsigned char>(1, value));
         }
-
-        /**
-         * Overload for setting parameters with four byte integer values.
-         *
-         * @param parameter
-         * @param value
-         */
         void setParameter(const Avr8EdbgParameter& parameter, std::uint32_t value) {
             auto paramValue = std::vector<unsigned char>(4);
             paramValue[0] = static_cast<unsigned char>(value);
@@ -382,13 +207,6 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
 
             this->setParameter(parameter, paramValue);
         }
-
-        /**
-         * Overload for setting parameters with two byte integer values.
-         *
-         * @param parameter
-         * @param value
-         */
         void setParameter(const Avr8EdbgParameter& parameter, std::uint16_t value) {
             auto paramValue = std::vector<unsigned char>(2);
             paramValue[0] = static_cast<unsigned char>(value);
@@ -397,13 +215,6 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
             this->setParameter(parameter, paramValue);
         }
 
-        /**
-         * Fetches an AV8 parameter from the debug tool.
-         *
-         * @param parameter
-         * @param size
-         * @return
-         */
         std::vector<unsigned char> getParameter(const Avr8EdbgParameter& parameter, std::uint8_t size);
 
         /**
@@ -425,91 +236,55 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         void setPdiParameters();
         void setUpdiParameters();
 
-        /**
-         * Sends the "Activate Physical" command to the debug tool, activating the physical interface and thus enabling
-         * communication between the debug tool and the target.
-         *
-         * @param applyExternalReset
-         */
         void activatePhysical(bool applyExternalReset = false);
-
-        /**
-         * Sends the "Deactivate Physical" command to the debug tool, which will result in the connection between the
-         * debug tool and the target being severed.
-         */
         void deactivatePhysical();
 
-        /**
-         * Sends the "Attach" command to the debug tool, which starts a debug session on the target.
-         */
         void attach();
-
-        /**
-         * Sends the "Detach" command to the debug tool, which terminates any active debug session on the target.
-         */
         void detach();
 
-        /**
-         * Issues the "Software Breakpoint Clear All" command to the debug tool, clearing all software breakpoints
-         * immediately.
-         */
+        void setSoftwareBreakpoint(Targets::TargetMemoryAddress address);
+        void clearSoftwareBreakpoint(Targets::TargetMemoryAddress address);
+        void setHardwareBreakpoint(Targets::TargetMemoryAddress address);
+        void clearHardwareBreakpoint(Targets::TargetMemoryAddress address);
         void clearAllSoftwareBreakpoints();
-
-        /**
-         * Clears all software and hardware breakpoints on the target.
-         *
-         * This function will not clear any untracked hardware breakpoints (breakpoints that were installed in a
-         * previous session).
-         */
         void clearAllBreakpoints();
+        void injectActiveBreakpoints(
+            const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+            const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+            Targets::TargetMemoryBuffer& data,
+            Targets::TargetMemoryAddress startAddress
+        );
+        void concealPendingBreakpointOperations(
+            const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+            const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+            Targets::TargetMemoryBuffer& data,
+            Targets::TargetMemoryAddress startAddress
+        );
+        void commitPendingBreakpointOperations();
+        void injectBreakOpcodeAtBreakpoint(
+            const Targets::TargetProgramBreakpoint& breakpoint,
+            const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+            const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+            Targets::TargetMemoryBuffer& data,
+            Targets::TargetMemoryAddress startAddress
+        );
+        void injectOriginalOpcodeAtBreakpoint(
+            const Targets::TargetProgramBreakpoint& breakpoint,
+            const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor,
+            const Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor,
+            Targets::TargetMemoryBuffer& data,
+            Targets::TargetMemoryAddress startAddress
+        );
 
-        /**
-         * Fetches any queued events belonging to the AVR8 Generic protocol (such as target break events).
-         *
-         * @return
-         */
         std::unique_ptr<AvrEvent> getAvrEvent();
-
-        /**
-         * Clears (discards) any queued AVR8 Generic protocol events on the debug tool.
-         */
         void clearEvents();
 
         Avr8MemoryType getRegisterMemoryType(const Targets::TargetRegisterDescriptor& descriptor);
 
-        /**
-         * Checks if alignment is required for memory access via a given Avr8MemoryType.
-         *
-         * @param memoryType
-         * @return
-         */
         bool alignmentRequired(Avr8MemoryType memoryType);
-
-        /**
-         * Aligns a memory address for a given memory type's page size.
-         *
-         * @param memoryType
-         * @param address
-         * @return
-         */
         Targets::TargetMemoryAddress alignMemoryAddress(Avr8MemoryType memoryType, Targets::TargetMemoryAddress address);
-
-        /**
-         * Aligns a number of bytes used to address memory, for a given memory type's page size.
-         *
-         * @param memoryType
-         * @param bytes
-         * @return
-         */
         Targets::TargetMemorySize alignMemoryBytes(Avr8MemoryType memoryType, Targets::TargetMemorySize bytes);
 
-        /**
-         * Determines the maximum memory access size imposed on the given Avr8MemoryType.
-         *
-         * @param memoryType
-         *
-         * @return
-         */
         Targets::TargetMemorySize maximumMemoryAccessSize(Avr8MemoryType memoryType);
 
         /**
@@ -558,18 +333,8 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
             Targets::TargetMemoryBufferSpan buffer
         );
 
-        /**
-         * Fetches the current target state.
-         *
-         * This currently uses AVR BREAK events to determine if a target has stopped. The lack of any
-         * queued BREAK events leads to the assumption that the target is still running.
-         */
         void refreshTargetState();
 
-        /**
-         * Temporarily disables the debugWIRE module on the target. This does not affect the DWEN fuse. The module
-         * will be reactivated upon the cycling of the target power.
-         */
         void disableDebugWire();
 
         /**
@@ -586,7 +351,7 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
          *  will be returned.
          */
         template <class AvrEventType>
-        std::unique_ptr<AvrEventType> waitForAvrEvent(int maximumAttempts = 20) {
+        std::unique_ptr<AvrEventType> waitForAvrEvent(int maximumAttempts = 60) {
             int attemptCount = 0;
 
             while (attemptCount <= maximumAttempts) {
@@ -601,21 +366,12 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
                     }
                 }
 
-                std::this_thread::sleep_for(std::chrono::milliseconds{50});
+                std::this_thread::sleep_for(std::chrono::milliseconds{5});
                 attemptCount++;
             }
 
             return nullptr;
         }
-
-        /**
-         * Waits for an AVR BREAK event.
-         *
-         * This simply wraps a call to waitForAvrEvent<BreakEvent>(). An exception will be thrown if the call doesn't
-         * return a BreakEvent.
-         *
-         * This should only be used when a BreakEvent is always expected.
-         */
         void waitForStoppedEvent();
     };
 }

src/DebugToolDrivers/TargetInterfaces/Microchip/Avr8/Avr8DebugInterface.hpp

@@ -15,6 +15,7 @@
 #include "src/Targets/TargetState.hpp"
 #include "src/Targets/TargetRegisterDescriptor.hpp"
 #include "src/Targets/TargetMemory.hpp"
+#include "src/Targets/TargetBreakpoint.hpp"
 
 namespace DebugToolDrivers::TargetInterfaces::Microchip::Avr8
 {
@@ -95,10 +96,8 @@ namespace DebugToolDrivers::TargetInterfaces::Microchip::Avr8
          */
         virtual Targets::Microchip::Avr8::TargetSignature getDeviceId() = 0;
 
-        virtual void setSoftwareBreakpoint(Targets::TargetMemoryAddress address) = 0;
-        virtual void clearSoftwareBreakpoint(Targets::TargetMemoryAddress address) = 0;
-        virtual void setHardwareBreakpoint(Targets::TargetMemoryAddress address) = 0;
-        virtual void clearHardwareBreakpoint(Targets::TargetMemoryAddress address) = 0;
+        virtual void setProgramBreakpoint(const Targets::TargetProgramBreakpoint& breakpoint) = 0;
+        virtual void removeProgramBreakpoint(const Targets::TargetProgramBreakpoint& breakpoint) = 0;
 
         virtual Targets::TargetMemoryAddress getProgramCounter() = 0;
         virtual void setProgramCounter(Targets::TargetMemoryAddress programCounter) = 0;

src/Targets/Microchip/Avr8/Avr8.cpp

@@ -367,28 +367,18 @@ namespace Targets::Microchip::Avr8
 
     void Avr8::setProgramBreakpoint(const TargetProgramBreakpoint& breakpoint) {
         if (breakpoint.addressSpaceDescriptor != this->programAddressSpaceDescriptor) {
-            throw Exception{"Unexpected address space descriptor"};
+            throw Exception{"Unexpected address space"};
         }
 
-        if (breakpoint.type == TargetProgramBreakpoint::Type::HARDWARE) {
-            this->avr8DebugInterface->setHardwareBreakpoint(breakpoint.address);
-
-        } else {
-            this->avr8DebugInterface->setSoftwareBreakpoint(breakpoint.address);
-        }
+        this->avr8DebugInterface->setProgramBreakpoint(breakpoint);
     }
 
     void Avr8::removeProgramBreakpoint(const TargetProgramBreakpoint& breakpoint) {
         if (breakpoint.addressSpaceDescriptor != this->programAddressSpaceDescriptor) {
-            throw Exception{"Unexpected address space descriptor"};
+            throw Exception{"Unexpected address space"};
         }
 
-        if (breakpoint.type == TargetProgramBreakpoint::Type::HARDWARE) {
-            this->avr8DebugInterface->clearHardwareBreakpoint(breakpoint.address);
-
-        } else {
-            this->avr8DebugInterface->clearSoftwareBreakpoint(breakpoint.address);
-        }
+        this->avr8DebugInterface->removeProgramBreakpoint(breakpoint);
     }
 
     TargetRegisterDescriptorAndValuePairs Avr8::readRegisters(const Targets::TargetRegisterDescriptors& descriptors) {