From 3f1247ce740c3ee4de0df2366d69766c5c117352 Mon Sep 17 00:00:00 2001 From: Nav Date: Sun, 6 Jun 2021 17:44:13 +0100 Subject: [PATCH] Fixed issue with automatic config variant selection, where we were not properly handling XMEGA targets with the JTAG physical interface. Also introduced new AVR8 families, for D series targets. Also moved AVR family param outside of TargetParameters struct --- .../AVR8/Avr8TargetDescriptionFile.php | 16 +++ .../VendorSpecific/EDBG/AVR/Avr8Generic.hpp | 5 +- .../EDBG/AVR/EdbgAvr8Interface.cpp | 27 +++- .../EDBG/AVR/EdbgAvr8Interface.hpp | 123 ++++++++++++++++-- .../Microchip/AVR/AVR8/Avr8Interface.hpp | 10 +- src/Targets/Microchip/AVR/AVR8/Avr8.cpp | 12 +- src/Targets/Microchip/AVR/AVR8/Avr8.hpp | 1 + src/Targets/Microchip/AVR/AVR8/Family.hpp | 3 + .../TargetDescriptionFile.cpp | 4 +- .../TargetDescriptionFile.hpp | 3 + .../Microchip/AVR/AVR8/TargetParameters.hpp | 2 - 11 files changed, 178 insertions(+), 28 deletions(-) diff --git a/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php b/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php index 8f0ef13d..88886113 100644 --- a/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php +++ b/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php @@ -10,6 +10,9 @@ class Avr8TargetDescriptionFile extends TargetDescriptionFile const AVR8_FAMILY_MEGA = 'MEGA'; const AVR8_FAMILY_TINY = 'TINY'; const AVR8_FAMILY_XMEGA = 'XMEGA'; + const AVR8_FAMILY_DB = 'DB'; + const AVR8_FAMILY_DA = 'DA'; + const AVR8_FAMILY_DD = 'DD'; const AVR8_FAMILY_OTHER = 'OTHER'; const AVR8_PHYSICAL_INTERFACE_JTAG = 'JTAG'; @@ -77,6 +80,15 @@ class Avr8TargetDescriptionFile extends TargetDescriptionFile } else if (stristr($deviceAttributes['family'], 'mega') !== false) { $this->family = self::AVR8_FAMILY_MEGA; + } else if (strtolower(trim($deviceAttributes['family'])) == 'avr da') { + $this->family = self::AVR8_FAMILY_DA; + + } else if (strtolower(trim($deviceAttributes['family'])) == 'avr db') { + $this->family = self::AVR8_FAMILY_DB; + + } else if (strtolower(trim($deviceAttributes['family'])) == 'avr dd') { + $this->family = self::AVR8_FAMILY_DD; + } else { $this->family = self::AVR8_FAMILY_OTHER; } @@ -338,6 +350,10 @@ class Avr8TargetDescriptionFile extends TargetDescriptionFile return $failures; } + if (is_null($this->family) || $this->family == self::AVR8_FAMILY_OTHER) { + $failures[] = 'Unknown AVR8 family'; + } + if (is_null($this->stackPointerRegisterStartAddress)) { $failures[] = 'Missing stack pointer register start address.'; } diff --git a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/Avr8Generic.hpp b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/Avr8Generic.hpp index 7a124660..88a77a9d 100644 --- a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/Avr8Generic.hpp +++ b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/Avr8Generic.hpp @@ -88,7 +88,7 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr enum class Avr8MemoryType: unsigned char { /** - * The SRAM memory type can be used to read &write to internal memory on the target. + * The SRAM memory type can be used to read & write to internal memory on the target. * * It's available with all of the config variants in debugging mode. */ @@ -144,5 +144,4 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr DATA = 0x84, FAILED = 0xA0, }; - -} \ No newline at end of file +} diff --git a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp index 4a350a8f..9e7f45d2 100644 --- a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp +++ b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp @@ -69,7 +69,6 @@ void EdbgAvr8Interface::configure(const TargetConfig& targetConfig) { auto physicalInterface = targetConfig.jsonObject.find("physicalInterface")->toString().toLower().toStdString(); auto availablePhysicalInterfaces = EdbgAvr8Interface::physicalInterfacesByName; - auto availableConfigVariants = EdbgAvr8Interface::configVariantsByPhysicalInterface; if (physicalInterface.empty() || availablePhysicalInterfaces.find(physicalInterface) == availablePhysicalInterfaces.end() @@ -79,16 +78,18 @@ void EdbgAvr8Interface::configure(const TargetConfig& targetConfig) { auto selectedPhysicalInterface = availablePhysicalInterfaces.find(physicalInterface)->second; - if (availableConfigVariants.find(selectedPhysicalInterface) == availableConfigVariants.end()) { - throw InvalidConfig("Invalid config variant deduced from physical interface."); - } - if (selectedPhysicalInterface == Avr8PhysicalInterface::DEBUG_WIRE) { Logger::warning("AVR8 debugWire interface selected - the DWEN fuse will need to be enabled"); } this->physicalInterface = selectedPhysicalInterface; - this->configVariant = availableConfigVariants.find(selectedPhysicalInterface)->second; + + if (this->physicalInterface == Avr8PhysicalInterface::JTAG && !this->family.has_value()) { + throw InvalidConfig("Cannot use JTAG physical interface with ambiguous target name - please specify the" + " exact name of the target in your configuration file. See https://bloom.oscillate.io/docs/supported-targets"); + } + + this->configVariant = this->resolveConfigVariant().value_or(Avr8ConfigVariant::NONE); } void EdbgAvr8Interface::setTargetParameters(const Avr8Bit::TargetParameters& config) { @@ -110,6 +111,18 @@ void EdbgAvr8Interface::setTargetParameters(const Avr8Bit::TargetParameters& con throw Exception("Failed to find status register size"); } + if (this->configVariant == Avr8ConfigVariant::NONE) { + auto configVariant = this->resolveConfigVariant(); + + if (!configVariant.has_value()) { + throw Exception("Failed to resolve config variant for the selected " + "physical interface and AVR8 family. The selected physical interface is not known to be supported " + "by the AVR8 family."); + } + + this->configVariant = configVariant.value(); + } + if (this->configVariant == Avr8ConfigVariant::XMEGA) { if (!config.appSectionPdiOffset.has_value()) { throw Exception("Missing required parameter: APPL_BASE_ADDR"); @@ -850,7 +863,7 @@ TargetRegisters EdbgAvr8Interface::readGeneralPurposeRegisters(std::setreadMemory( - this->targetParameters.family == Family::XMEGA ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM, + this->family == Family::XMEGA ? Avr8MemoryType::REGISTER_FILE : Avr8MemoryType::SRAM, this->targetParameters.gpRegisterStartAddress.value_or(0x00), this->targetParameters.gpRegisterSize.value_or(32) ); diff --git a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.hpp b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.hpp index e5e8f633..00f296f3 100644 --- a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.hpp +++ b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.hpp @@ -41,6 +41,14 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr */ EdbgInterface& edbgInterface; + /** + * The target family is taken into account when configuring the AVR8 Generic protocol on the EDBG device. + * + * We use this to determine which config variant to select. + * See EdbgAvr8Interface::resolveConfigVariant() for more. + */ + std::optional family; + /** * The AVR8 Generic protocol provides two functions: Debugging and programming. The desired function must be * configured via the setting of the "AVR8_CONFIG_FUNCTION" parameter. @@ -52,13 +60,13 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr * The "AVR8_CONFIG_VARIANT" parameter allows us to determine which target parameters are required by the * debug tool. */ - Avr8ConfigVariant configVariant; + Avr8ConfigVariant configVariant = Avr8ConfigVariant::NONE; /** * Currently, the AVR8 Generic protocol supports 4 physical interfaces: debugWire, JTAG, PDI and UPDI. * The desired physical interface must be selected by setting the "AVR8_PHY_PHYSICAL" parameter. */ - Avr8PhysicalInterface physicalInterface; + Avr8PhysicalInterface physicalInterface = Avr8PhysicalInterface::NONE; /** * EDBG-based debug tools require target specific parameters such as memory locations, page sizes and @@ -123,17 +131,100 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr }; /** - * Although users can supply the desired config variant via their Bloom configuration, this is not required. - * This mapping allows us to determine which config variant to select, based on the selected physical - * interface. + * This mapping allows us to determine which config variant to select, based on the target family and the + * selected physical interface. */ - static inline std::map configVariantsByPhysicalInterface = { - {Avr8PhysicalInterface::DEBUG_WIRE, Avr8ConfigVariant::DEBUG_WIRE}, - {Avr8PhysicalInterface::PDI, Avr8ConfigVariant::XMEGA}, - {Avr8PhysicalInterface::JTAG, Avr8ConfigVariant::MEGAJTAG}, - {Avr8PhysicalInterface::PDI_1W, Avr8ConfigVariant::UPDI}, + static inline auto getConfigVariantsByFamilyAndPhysicalInterface() { + using Targets::Microchip::Avr::Avr8Bit::Family; + return std::map>({ + { + Family::MEGA, + { + {Avr8PhysicalInterface::JTAG, Avr8ConfigVariant::MEGAJTAG}, + {Avr8PhysicalInterface::DEBUG_WIRE, Avr8ConfigVariant::DEBUG_WIRE}, + } + }, + { + Family::TINY, + { + {Avr8PhysicalInterface::JTAG, Avr8ConfigVariant::MEGAJTAG}, + {Avr8PhysicalInterface::DEBUG_WIRE, Avr8ConfigVariant::DEBUG_WIRE}, + } + }, + { + Family::XMEGA, + { + {Avr8PhysicalInterface::JTAG, Avr8ConfigVariant::XMEGA}, + {Avr8PhysicalInterface::PDI, Avr8ConfigVariant::XMEGA}, + } + }, + { + Family::DA, + { + {Avr8PhysicalInterface::PDI_1W, Avr8ConfigVariant::UPDI}, + } + }, + { + Family::DB, + { + {Avr8PhysicalInterface::PDI_1W, Avr8ConfigVariant::UPDI}, + } + }, + { + Family::DD, + { + {Avr8PhysicalInterface::PDI_1W, Avr8ConfigVariant::UPDI}, + } + }, + }); }; + /** + * Will attempt to resolve the config variant with the information currently held. + * + * @return + */ + std::optional resolveConfigVariant() { + if (this->family.has_value()) { + auto configVariantsByFamily = EdbgAvr8Interface::getConfigVariantsByFamilyAndPhysicalInterface(); + + if (configVariantsByFamily.contains(this->family.value())) { + auto configVariantsByPhysicalInterface = configVariantsByFamily + .at(this->family.value()); + + if (configVariantsByPhysicalInterface.contains(this->physicalInterface)) { + return configVariantsByPhysicalInterface.at(this->physicalInterface); + } + } + + } else { + /* + * If there is no family set, we may be able to resort to a simpler mapping of physical interfaces + * to config variants. But this will only work if the selected physical interface is *NOT* JTAG. + * + * This is because JTAG is the only physical interface that could map to two different config + * variants (MEGAJTAG and XMEGA). The only way we can figure out which config variant to use is if we + * know the target family. + * + * This is why we don't allow users to use ambiguous target names (such as the generic "avr8" target + * name), when using the JTAG physical interface. We won't be able to resolve the correct target + * variant. Users are required to specify the exact target name in their config, when using the JTAG + * physical interface. That way, this->family will be set by the time resolveConfigVariant() is called. + */ + static std::map physicalInterfacesToConfigVariants = { + {Avr8PhysicalInterface::DEBUG_WIRE, Avr8ConfigVariant::DEBUG_WIRE}, + {Avr8PhysicalInterface::PDI, Avr8ConfigVariant::XMEGA}, + {Avr8PhysicalInterface::PDI_1W, Avr8ConfigVariant::UPDI}, + }; + + if (physicalInterfacesToConfigVariants.contains(this->physicalInterface)) { + return physicalInterfacesToConfigVariants.at(this->physicalInterface); + } + } + + return std::nullopt; + } + /** * Sets an AVR8 parameter on the debug tool. See the Avr8EdbgParameters class and protocol documentation * for more on available parameters. @@ -338,6 +429,16 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr */ virtual void configure(const TargetConfig& targetConfig) override; + /** + * Configures the target family. For some physical interfaces, the target family is required in order + * properly configure the EDBG tool. See EdbgAvr8Interface::resolveConfigVariant() for more. + * + * @param family + */ + void setFamily(Targets::Microchip::Avr::Avr8Bit::Family family) override { + this->family = family; + } + /** * Accepts target parameters from the AVR8 target instance and sends the necessary target parameters to the * debug tool. @@ -522,4 +623,4 @@ namespace Bloom::DebugToolDrivers::Protocols::CmsisDap::Edbg::Avr */ virtual Targets::TargetState getTargetState() override; }; -} \ No newline at end of file +} diff --git a/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR/AVR8/Avr8Interface.hpp b/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR/AVR8/Avr8Interface.hpp index 9f1cdc23..8a7f662c 100644 --- a/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR/AVR8/Avr8Interface.hpp +++ b/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR/AVR8/Avr8Interface.hpp @@ -5,6 +5,7 @@ #include "src/Targets/Microchip/AVR/TargetSignature.hpp" #include "src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp" +#include "src/Targets/Microchip/AVR/AVR8/Family.hpp" #include "src/Targets/TargetState.hpp" #include "src/Targets/TargetRegister.hpp" #include "src/Targets/TargetMemory.hpp" @@ -36,6 +37,13 @@ namespace Bloom::DebugToolDrivers::TargetInterfaces::Microchip::Avr::Avr8 */ virtual void configure(const TargetConfig& targetConfig) = 0; + /** + * Sets the target family, independent of other configuration. + * + * @param family + */ + virtual void setFamily(Targets::Microchip::Avr::Avr8Bit::Family family) = 0; + /** * Should accept Avr8 target parameters for configuration of the interface. * @@ -205,4 +213,4 @@ namespace Bloom::DebugToolDrivers::TargetInterfaces::Microchip::Avr::Avr8 */ virtual Targets::TargetState getTargetState() = 0; }; -} \ No newline at end of file +} diff --git a/src/Targets/Microchip/AVR/AVR8/Avr8.cpp b/src/Targets/Microchip/AVR/AVR8/Avr8.cpp index 535b12c9..b0c227d4 100644 --- a/src/Targets/Microchip/AVR/AVR8/Avr8.cpp +++ b/src/Targets/Microchip/AVR/AVR8/Avr8.cpp @@ -33,6 +33,10 @@ using namespace Exceptions; void Avr8::preActivationConfigure(const TargetConfig& targetConfig) { Target::preActivationConfigure(targetConfig); + if (this->family.has_value()) { + this->avr8Interface->setFamily(this->family.value()); + } + this->avr8Interface->configure(targetConfig); } @@ -53,7 +57,7 @@ void Avr8::postActivationConfigure() { if (targetSignature != tdSignature) { throw Exception("Failed to validate connected target - target signature mismatch.\nThe target signature" - "(\"" + targetSignature.toHex() + "\") does not match the AVR8 target description signature (\"" + " (\"" + targetSignature.toHex() + "\") does not match the AVR8 target description signature (\"" + tdSignature.toHex() + "\"). This will likely be due to an incorrect target name in the configuration file" + " (bloom.json)." ); @@ -61,6 +65,11 @@ void Avr8::postActivationConfigure() { } void Avr8::postPromotionConfigure() { + if (!this->family.has_value()) { + throw Exception("Failed to resolve AVR8 family"); + } + + this->avr8Interface->setFamily(this->family.value()); this->avr8Interface->setTargetParameters(this->getTargetParameters()); } @@ -276,7 +285,6 @@ TargetParameters& Avr8::getTargetParameters() { if (!this->targetParameters.has_value()) { assert(this->targetDescriptionFile.has_value()); this->targetParameters = TargetParameters(); - this->targetParameters->family = this->family; auto& propertyGroups = this->targetDescriptionFile->getPropertyGroupsMappedByName(); auto flashMemorySegment = this->targetDescriptionFile->getFlashMemorySegment(); diff --git a/src/Targets/Microchip/AVR/AVR8/Avr8.hpp b/src/Targets/Microchip/AVR/AVR8/Avr8.hpp index 0316fbab..052a157d 100644 --- a/src/Targets/Microchip/AVR/AVR8/Avr8.hpp +++ b/src/Targets/Microchip/AVR/AVR8/Avr8.hpp @@ -62,6 +62,7 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit explicit Avr8() = default; Avr8(const std::string& name, const TargetSignature& signature): name(name) { this->id = signature; + this->loadTargetDescriptionFile(); }; /* diff --git a/src/Targets/Microchip/AVR/AVR8/Family.hpp b/src/Targets/Microchip/AVR/AVR8/Family.hpp index 048dca95..666e6684 100644 --- a/src/Targets/Microchip/AVR/AVR8/Family.hpp +++ b/src/Targets/Microchip/AVR/AVR8/Family.hpp @@ -7,5 +7,8 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit MEGA, XMEGA, TINY, + DB, + DA, + DD, }; } diff --git a/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp b/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp index 22eecc4c..3034faf2 100644 --- a/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp +++ b/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp @@ -141,9 +141,9 @@ Family TargetDescriptionFile::getFamily() const { throw Exception("Could not find target family name in target description file."); } - if (familyNameToEnums.find(familyName) == familyNameToEnums.end()) { + if (!familyNameToEnums.contains(familyName)) { throw Exception("Unknown family name in target description file."); } - return familyNameToEnums.find(familyName)->second; + return familyNameToEnums.at(familyName); } diff --git a/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.hpp b/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.hpp index 61ed6116..f2dc6148 100644 --- a/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.hpp +++ b/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.hpp @@ -36,6 +36,9 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit::TargetDescription {"avr tiny", Family::TINY}, {"tinyavr", Family::TINY}, {"tinyavr 2", Family::TINY}, + {"avr da", Family::DA}, + {"avr db", Family::DB}, + {"avr dd", Family::DD}, }; }; diff --git a/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp b/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp index f2686066..7ddaefee 100644 --- a/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp +++ b/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp @@ -10,8 +10,6 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit { struct TargetParameters { - std::optional family; - std::optional bootSectionStartAddress; std::optional gpRegisterStartAddress; std::optional gpRegisterSize;