From af16b4bdf8250eb8ad188e3c4acb50e5372690d5 Mon Sep 17 00:00:00 2001 From: Nav Date: Wed, 22 Jun 2022 22:23:00 +0100 Subject: [PATCH] Refactored AVR8 target pin state manipulation - removed unnecessary register accesses when setting pin states --- src/Targets/Microchip/AVR/AVR8/Avr8.cpp | 135 +++++------------- .../Microchip/AVR/AVR8/PadDescriptor.hpp | 18 +-- .../TargetDescriptionFile.cpp | 63 +++----- 3 files changed, 61 insertions(+), 155 deletions(-) diff --git a/src/Targets/Microchip/AVR/AVR8/Avr8.cpp b/src/Targets/Microchip/AVR/AVR8/Avr8.cpp index 10f09d9e..8f9064a9 100644 --- a/src/Targets/Microchip/AVR/AVR8/Avr8.cpp +++ b/src/Targets/Microchip/AVR/AVR8/Avr8.cpp @@ -452,23 +452,23 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit auto pinState = TargetPinState(); - if (pad.ddrSetAddress.has_value()) { - auto dataDirectionRegisterValue = readMemoryBitset(pad.ddrSetAddress.value()); - pinState.ioDirection = dataDirectionRegisterValue.test(pad.gpioPinNumber.value()) ? + if (pad.gpioDdrAddress.has_value()) { + const auto ddrValue = readMemoryBitset(pad.gpioDdrAddress.value()); + + pinState.ioDirection = ddrValue.test(pad.gpioPinNumber.value()) ? TargetPinState::IoDirection::OUTPUT : TargetPinState::IoDirection::INPUT; if (pinState.ioDirection == TargetPinState::IoDirection::OUTPUT - && pad.gpioPortSetAddress.has_value() + && pad.gpioPortAddress.has_value() ) { - auto portRegisterValue = readMemoryBitset(pad.gpioPortSetAddress.value()); - pinState.ioState = portRegisterValue.test(pad.gpioPinNumber.value()) ? + const auto portRegisterValueBitset = readMemoryBitset(pad.gpioPortAddress.value()); + pinState.ioState = portRegisterValueBitset.test(pad.gpioPinNumber.value()) ? TargetPinState::IoState::HIGH : TargetPinState::IoState::LOW; } else if (pinState.ioDirection == TargetPinState::IoDirection::INPUT && pad.gpioPortInputAddress.has_value() ) { - auto portInputRegisterValue = readMemoryBitset(pad.gpioPortInputAddress.value()); - auto h = portInputRegisterValue.to_string(); + const auto portInputRegisterValue = readMemoryBitset(pad.gpioPortInputAddress.value()); pinState.ioState = portInputRegisterValue.test(pad.gpioPinNumber.value()) ? TargetPinState::IoState::HIGH : TargetPinState::IoState::LOW; } @@ -495,126 +495,63 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit throw Exception("Missing IO direction state"); } - auto& variant = this->targetVariantsById.at(variantId); - auto& padDescriptor = this->padDescriptorsByName.at(pinDescriptor.padName); + const auto& variant = this->targetVariantsById.at(variantId); + const auto& padDescriptor = this->padDescriptorsByName.at(pinDescriptor.padName); auto ioState = state.ioState; if (state.ioDirection == TargetPinState::IoDirection::INPUT) { - // When setting the direction to INPUT, we must always set the IO pinstate to LOW + // When setting the direction to INPUT, we must always set the IO pin state to LOW ioState = TargetPinState::IoState::LOW; } if ( - !padDescriptor.ddrSetAddress.has_value() - || !padDescriptor.gpioPortSetAddress.has_value() + !padDescriptor.gpioDdrAddress.has_value() + || !padDescriptor.gpioPortAddress.has_value() || !padDescriptor.gpioPinNumber.has_value() ) { throw Exception("Inadequate pad descriptor"); } - auto pinNumber = padDescriptor.gpioPinNumber.value(); - auto ddrSetAddress = padDescriptor.ddrSetAddress.value(); - auto ddrSetValue = this->readMemory(TargetMemoryType::RAM, ddrSetAddress, 1); + const auto pinNumber = padDescriptor.gpioPinNumber.value(); + const auto ddrAddress = padDescriptor.gpioDdrAddress.value(); + const auto ddrValue = this->readMemory(TargetMemoryType::RAM, ddrAddress, 1); - if (ddrSetValue.empty()) { - throw Exception("Failed to read DDSR value"); + if (ddrValue.empty()) { + throw Exception("Failed to read DDR value"); } - auto ddrSetBitset = std::bitset::digits>(ddrSetValue.front()); - if (ddrSetBitset.test(pinNumber) != (state.ioDirection == TargetPinState::IoDirection::OUTPUT)) { + auto ddrValueBitset = std::bitset::digits>(ddrValue.front()); + if (ddrValueBitset.test(pinNumber) != (state.ioDirection == TargetPinState::IoDirection::OUTPUT)) { // DDR needs updating - ddrSetBitset.set(pinNumber, (state.ioDirection == TargetPinState::IoDirection::OUTPUT)); + ddrValueBitset.set(pinNumber, (state.ioDirection == TargetPinState::IoDirection::OUTPUT)); this->writeMemory( TargetMemoryType::RAM, - ddrSetAddress, - {static_cast(ddrSetBitset.to_ulong())} + ddrAddress, + {static_cast(ddrValueBitset.to_ulong())} ); } - if (padDescriptor.ddrClearAddress.has_value() && padDescriptor.ddrClearAddress != ddrSetAddress) { - // We also need to ensure the data direction clear register value is correct - auto ddrClearAddress = padDescriptor.ddrClearAddress.value(); - auto ddrClearValue = this->readMemory(TargetMemoryType::RAM, ddrClearAddress, 1); - - if (ddrClearValue.empty()) { - throw Exception("Failed to read DDCR value"); - } - - auto ddrClearBitset = std::bitset::digits>(ddrClearValue.front()); - if (ddrClearBitset.test(pinNumber) == (state.ioDirection == TargetPinState::IoDirection::INPUT)) { - ddrClearBitset.set(pinNumber, (state.ioDirection == TargetPinState::IoDirection::INPUT)); - - this->writeMemory( - TargetMemoryType::RAM, - ddrClearAddress, - {static_cast(ddrClearBitset.to_ulong())} - ); - } - } - if (ioState.has_value()) { - auto portSetAddress = padDescriptor.gpioPortSetAddress.value(); + const auto portRegisterAddress = padDescriptor.gpioPortAddress.value(); + const auto portRegisterValue = this->readMemory(TargetMemoryType::RAM, portRegisterAddress, 1); - if (ioState == TargetPinState::IoState::HIGH - || !padDescriptor.gpioPortClearAddress.has_value() - || padDescriptor.gpioPortClearAddress == portSetAddress - ) { - if (padDescriptor.gpioPortClearAddress != portSetAddress) { - /* - * We don't need to read the SET register if the SET and CLEAR operations are performed via - * different registers. - * - * Instead, we can just set the appropriate bit against the SET register. - */ - this->writeMemory( - TargetMemoryType::RAM, - portSetAddress, - {static_cast(0x01 << pinNumber)} - ); - - } else { - auto portSetRegisterValue = this->readMemory( - TargetMemoryType::RAM, - portSetAddress, - 1 - ); - - if (portSetRegisterValue.empty()) { - throw Exception("Failed to read PORT register value"); - } - - auto portSetBitset = std::bitset::digits>( - portSetRegisterValue.front() - ); - if (portSetBitset.test(pinNumber) != (ioState == TargetPinState::IoState::HIGH)) { - // PORT set register needs updating - portSetBitset.set(pinNumber, (ioState == TargetPinState::IoState::HIGH)); - - this->writeMemory( - TargetMemoryType::RAM, - portSetAddress, - {static_cast(portSetBitset.to_ulong())} - ); - } - } + if (portRegisterValue.empty()) { + throw Exception("Failed to read PORT register value"); } - /* - * We only need to update the PORT clear register if the IO state was set to LOW, and the clear register is - * not the same register as the set register. - */ - if (ioState == TargetPinState::IoState::LOW - && padDescriptor.gpioPortClearAddress.has_value() - && padDescriptor.gpioPortClearAddress != portSetAddress - ) { - // We also need to ensure the PORT clear register value is correct - auto portClearAddress = padDescriptor.gpioPortClearAddress.value(); + auto portRegisterValueBitset = std::bitset::digits>( + portRegisterValue.front() + ); + + if (portRegisterValueBitset.test(pinNumber) != (ioState == TargetPinState::IoState::HIGH)) { + // PORT set register needs updating + portRegisterValueBitset.set(pinNumber, (ioState == TargetPinState::IoState::HIGH)); this->writeMemory( TargetMemoryType::RAM, - portClearAddress, - {static_cast(0x01 << pinNumber)} + portRegisterAddress, + {static_cast(portRegisterValueBitset.to_ulong())} ); } } diff --git a/src/Targets/Microchip/AVR/AVR8/PadDescriptor.hpp b/src/Targets/Microchip/AVR/AVR8/PadDescriptor.hpp index 86d7ef44..5704cdea 100644 --- a/src/Targets/Microchip/AVR/AVR8/PadDescriptor.hpp +++ b/src/Targets/Microchip/AVR/AVR8/PadDescriptor.hpp @@ -21,22 +21,8 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit std::string name; std::optional gpioPinNumber; - - /** - * AVR8 GPIO pins can be manipulated by writing to an IO register address. The gpioPortAddress member - * holds this address. - */ - std::optional gpioPortSetAddress; + std::optional gpioPortAddress; std::optional gpioPortInputAddress; - - std::optional gpioPortClearAddress; - - /** - * The data direction of a pin is configured via a data direction register (DDR), which, like the - * gpioPortSetAddress, is an IO register. - */ - std::optional ddrSetAddress; - - std::optional ddrClearAddress; + std::optional gpioDdrAddress; }; } diff --git a/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp b/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp index 177ed96b..642c6090 100644 --- a/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp +++ b/src/Targets/Microchip/AVR/AVR8/TargetDescription/TargetDescriptionFile.cpp @@ -441,8 +441,7 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit::TargetDescription for (const auto& [registerName, portRegister] : registerGroup.registersMappedByName) { if (registerName.find("port") == 0) { // This is the data register for the port - padDescriptor.gpioPortSetAddress = portRegister.offset; - padDescriptor.gpioPortClearAddress = portRegister.offset; + padDescriptor.gpioPortAddress = portRegister.offset; } else if (registerName.find("pin") == 0) { // This is the input data register for the port @@ -450,8 +449,7 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit::TargetDescription } else if (registerName.find("ddr") == 0) { // This is the data direction register for the port - padDescriptor.ddrSetAddress = portRegister.offset; - padDescriptor.ddrClearAddress = portRegister.offset; + padDescriptor.gpioDdrAddress = portRegister.offset; } } @@ -460,46 +458,31 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit::TargetDescription auto registerGroup = portModule->registerGroupsMappedByName.find("port")->second; for (const auto& [registerName, portRegister] : registerGroup.registersMappedByName) { - if (registerName.find("outset") == 0) { + if (registerName == "out") { // Include the port register offset - padDescriptor.gpioPortSetAddress = (portPeripheralRegisterGroup.has_value() - && portPeripheralRegisterGroup->offset.has_value()) ? - portPeripheralRegisterGroup->offset.value_or(0) : 0; + padDescriptor.gpioPortAddress = ( + portPeripheralRegisterGroup.has_value() + && portPeripheralRegisterGroup->offset.has_value() + ) + ? portPeripheralRegisterGroup->offset.value_or(0) + portRegister.offset + : 0 + portRegister.offset; - padDescriptor.gpioPortSetAddress = padDescriptor.gpioPortSetAddress.value() - + portRegister.offset; - } else if (registerName.find("outclr") == 0) { - padDescriptor.gpioPortClearAddress = (portPeripheralRegisterGroup.has_value() - && portPeripheralRegisterGroup->offset.has_value()) ? - portPeripheralRegisterGroup->offset.value_or(0) : 0; - - padDescriptor.gpioPortClearAddress = padDescriptor.gpioPortClearAddress.value() - + portRegister.offset; - - } else if (registerName.find("dirset") == 0) { - padDescriptor.ddrSetAddress = (portPeripheralRegisterGroup.has_value() - && portPeripheralRegisterGroup->offset.has_value()) ? - portPeripheralRegisterGroup->offset.value_or(0) : 0; - - padDescriptor.ddrSetAddress = padDescriptor.ddrSetAddress.value() - + portRegister.offset; - - } else if (registerName.find("dirclr") == 0) { - padDescriptor.ddrClearAddress = (portPeripheralRegisterGroup.has_value() - && portPeripheralRegisterGroup->offset.has_value()) ? - portPeripheralRegisterGroup->offset.value_or(0) : 0; - - padDescriptor.ddrClearAddress = padDescriptor.ddrClearAddress.value() - + portRegister.offset; + } else if (registerName == "dir") { + padDescriptor.gpioDdrAddress = ( + portPeripheralRegisterGroup.has_value() + && portPeripheralRegisterGroup->offset.has_value() + ) + ? portPeripheralRegisterGroup->offset.value_or(0) + portRegister.offset + : 0 + portRegister.offset; } else if (registerName == "in") { - padDescriptor.gpioPortInputAddress = (portPeripheralRegisterGroup.has_value() - && portPeripheralRegisterGroup->offset.has_value()) ? - portPeripheralRegisterGroup->offset.value_or(0) : 0; - - padDescriptor.gpioPortInputAddress = padDescriptor.gpioPortInputAddress.value() - + portRegister.offset; + padDescriptor.gpioPortInputAddress = ( + portPeripheralRegisterGroup.has_value() + && portPeripheralRegisterGroup->offset.has_value() + ) + ? portPeripheralRegisterGroup->offset.value_or(0) + portRegister.offset + : 0 + portRegister.offset; } } } @@ -570,7 +553,7 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit::TargetDescription if (this->padDescriptorsByName.contains(targetPin.padName)) { const auto& pad = this->padDescriptorsByName.at(targetPin.padName); - if (pad.gpioPortSetAddress.has_value() && pad.ddrSetAddress.has_value()) { + if (pad.gpioPortAddress.has_value() && pad.gpioDdrAddress.has_value()) { targetPin.type = TargetPinType::GPIO; } }