rtu: getConfig shall return unique_ptr
Update the getConfig in PortFactory and related classes to return a
unique_ptr instead of a variable on stack to preserve polymorphic
behavior of the return port config. Without this change the values from
USBPort are dropped when casting from USBPort to PortFactoryConfig.
Change-Id: Ia250771a7fd7aedb133598446aab1e120aaf36b5
Signed-off-by: Jagpal Singh Gill <paligill@gmail.com>
diff --git a/rtu/port/base_port.hpp b/rtu/port/base_port.hpp
index 6a30dab..b4ca052 100644
--- a/rtu/port/base_port.hpp
+++ b/rtu/port/base_port.hpp
@@ -32,6 +32,8 @@
PortMode portMode = PortMode::unknown;
uint32_t baudRate = 0;
uint16_t rtsDelay = 0;
+
+ virtual ~Config() = default;
};
template <typename T>
diff --git a/rtu/port/port_factory.cpp b/rtu/port/port_factory.cpp
index b3b5d32..8db7ac3 100644
--- a/rtu/port/port_factory.cpp
+++ b/rtu/port/port_factory.cpp
@@ -18,25 +18,18 @@
auto PortFactory::getConfig(sdbusplus::async::context& ctx,
const sdbusplus::message::object_path& objectPath,
const std::string& interfaceName)
- -> sdbusplus::async::task<std::optional<config::PortFactoryConfig>>
+ -> sdbusplus::async::task<std::unique_ptr<config::PortFactoryConfig>>
{
if (interfaceName == USBPortConfigIntf::interface)
{
- auto res = co_await USBPort::getConfig(ctx, objectPath);
- if (!res)
- {
- co_return std::nullopt;
- }
- config::PortFactoryConfig config = res.value();
- config.portType = config::PortType::usb;
- co_return config;
+ co_return co_await USBPort::getConfig(ctx, objectPath);
}
- co_return std::nullopt;
+ co_return std::nullptr_t();
}
auto PortFactory::create(sdbusplus::async::context& ctx,
- config::PortFactoryConfig& config)
+ const config::PortFactoryConfig& config)
-> std::unique_ptr<BasePort>
{
if (config.portType == config::PortType::usb)
diff --git a/rtu/port/port_factory.hpp b/rtu/port/port_factory.hpp
index b46ab7b..ff1080c 100644
--- a/rtu/port/port_factory.hpp
+++ b/rtu/port/port_factory.hpp
@@ -22,6 +22,8 @@
struct PortFactoryConfig : public Config
{
PortType portType = PortType::unknown;
+
+ virtual ~PortFactoryConfig() = default;
};
} // namespace config
@@ -34,10 +36,10 @@
static auto getConfig(sdbusplus::async::context& ctx,
const sdbusplus::message::object_path& objectPath,
const std::string& interfaceName)
- -> sdbusplus::async::task<std::optional<config::PortFactoryConfig>>;
+ -> sdbusplus::async::task<std::unique_ptr<config::PortFactoryConfig>>;
static auto create(sdbusplus::async::context& ctx,
- config::PortFactoryConfig& config)
+ const config::PortFactoryConfig& config)
-> std::unique_ptr<BasePort>;
};
diff --git a/rtu/port/usb_port.cpp b/rtu/port/usb_port.cpp
index 57a7a8c..77f226a 100644
--- a/rtu/port/usb_port.cpp
+++ b/rtu/port/usb_port.cpp
@@ -29,14 +29,18 @@
std::string address = "unknown";
uint16_t port = 0;
uint16_t interface = 0;
+
+ virtual ~USBPortConfig() = default;
};
} // namespace config
-static auto getDevicePath(const config::Config& inConfig) -> std::string
+static auto getDevicePath(const config::PortFactoryConfig& inConfig)
+ -> std::string
{
namespace fs = std::filesystem;
auto config = static_cast<const config::USBPortConfig&>(inConfig);
+
std::regex pattern(
std::format("platform-{}\\.usb-usb.*{}-port{}", config.address,
config.interface, config.port));
@@ -59,7 +63,7 @@
}
USBPort::USBPort(sdbusplus::async::context& ctx,
- config::PortFactoryConfig& config) :
+ const config::PortFactoryConfig& config) :
BasePort(ctx, config, getDevicePath(config))
{
info("USB port {NAME} created successfully", "NAME", config.name);
@@ -67,9 +71,13 @@
auto USBPort::getConfig(sdbusplus::async::context& ctx,
const sdbusplus::message::object_path& objectPath)
- -> sdbusplus::async::task<std::optional<config::PortFactoryConfig>>
+ -> sdbusplus::async::task<std::unique_ptr<config::PortFactoryConfig>>
{
- config::USBPortConfig config = {};
+ auto config = std::make_unique<config::USBPortConfig>();
+ if (!config)
+ {
+ co_return std::nullptr_t();
+ }
auto properties =
co_await USBPortConfigIntf(ctx)
@@ -77,22 +85,24 @@
.path(objectPath.str)
.properties();
- auto res = updateBaseConfig(config, properties);
+ auto res = updateBaseConfig(*config, properties);
if (!res)
{
- co_return std::nullopt;
+ co_return std::nullptr_t();
}
- config.address = properties.device_address;
- config.port = properties.port;
- config.interface = properties.device_interface;
+ config->address = properties.device_address;
+ config->port = properties.port;
+ config->interface = properties.device_interface;
+ config->portType = config::PortType::usb;
debug(
"USB port config: {NAME} {PORT_TYPE} {PORT_MODE} {ADDRESS} {PORT} {INTERFACE} {BAUD_RATE} {RTS_DELAY}",
- "NAME", config.name, "PORT_TYPE", config.portType, "PORT_MODE",
- config.portMode, "ADDRESS", config.address, "PORT", config.port,
- "INTERFACE", config.interface, "BAUD_RATE", config.baudRate,
- "RTS_DELAY", config.rtsDelay);
+ "NAME", config->name, "PORT_TYPE", config->portType, "PORT_MODE",
+ config->portMode, "ADDRESS", config->address, "PORT", config->port,
+ "INTERFACE", config->interface, "BAUD_RATE", config->baudRate,
+ "RTS_DELAY", config->rtsDelay);
+
co_return config;
}
diff --git a/rtu/port/usb_port.hpp b/rtu/port/usb_port.hpp
index 5f9cff5..b2bc070 100644
--- a/rtu/port/usb_port.hpp
+++ b/rtu/port/usb_port.hpp
@@ -18,11 +18,11 @@
{
public:
explicit USBPort(sdbusplus::async::context& ctx,
- config::PortFactoryConfig& config);
+ const config::PortFactoryConfig& config);
static auto getConfig(sdbusplus::async::context& ctx,
const sdbusplus::message::object_path& objectPath)
- -> sdbusplus::async::task<std::optional<config::PortFactoryConfig>>;
+ -> sdbusplus::async::task<std::unique_ptr<config::PortFactoryConfig>>;
};
} // namespace phosphor::modbus::rtu::port
diff --git a/tests/test_port.cpp b/tests/test_port.cpp
index 32b9b49..d5c25a0 100644
--- a/tests/test_port.cpp
+++ b/tests/test_port.cpp
@@ -1,16 +1,25 @@
+#include "common/entity_manager_interface.hpp"
#include "modbus_server_tester.hpp"
-#include "port/base_port.hpp"
+#include "port/port_factory.hpp"
#include <fcntl.h>
+#include <xyz/openbmc_project/Configuration/USBPort/aserver.hpp>
+#include <xyz/openbmc_project/Inventory/Item/client.hpp>
+
#include <gtest/gtest.h>
using namespace std::literals;
+class PortTest;
+
namespace TestIntf = phosphor::modbus::test;
namespace PortIntf = phosphor::modbus::rtu::port;
namespace PortConfigIntf = PortIntf::config;
namespace RTUIntf = phosphor::modbus::rtu;
+using PortFactoryIntf = PortIntf::PortFactory;
+using USBPortConfigServerIntf =
+ sdbusplus::aserver::xyz::openbmc_project::configuration::USBPort<PortTest>;
struct properties_t
{
@@ -41,6 +50,7 @@
int fdClient = -1;
std::unique_ptr<TestIntf::ServerTester> serverTester;
int fdServer = -1;
+ bool getPortConfigPassed = false;
PortTest()
{
@@ -86,6 +96,11 @@
kill(socat_pid, SIGTERM);
}
+ void SetUp() override
+ {
+ getPortConfigPassed = false;
+ }
+
auto TestHoldingRegisters(PortConfigIntf::Config& config, MockPort& port,
uint16_t registerOffset, bool res)
-> sdbusplus::async::task<void>
@@ -112,6 +127,42 @@
co_return;
}
+
+ template <typename Config, typename Properties>
+ static inline void VerifyConfig(const Config& config,
+ const Properties& property)
+ {
+ EXPECT_EQ(config, property);
+ }
+
+ auto TestGetUSBPortConfig(
+ const USBPortConfigServerIntf::properties_t properties, bool shouldPass)
+ -> sdbusplus::async::task<void>
+ {
+ static constexpr auto objectPath =
+ "/xyz/openbmc_project/inventory/system/board/Ventura_Modbus/DevTTYUSB0";
+
+ auto configServer = std::make_unique<USBPortConfigServerIntf>(
+ ctx, objectPath, properties);
+
+ auto config = co_await PortFactoryIntf::getConfig(
+ ctx, std::string(objectPath), USBPortConfigServerIntf::interface);
+
+ if (!shouldPass)
+ {
+ VerifyConfig(config, nullptr);
+ co_return;
+ }
+
+ VerifyConfig(config->name, properties.name);
+ VerifyConfig(config->portMode, PortConfigIntf::PortMode::rs485);
+ VerifyConfig(config->baudRate, properties.baud_rate);
+ VerifyConfig(config->rtsDelay, properties.rts_delay);
+
+ getPortConfigPassed = true;
+
+ co_return;
+ }
};
TEST_F(PortTest, TestUpdateConfig)
@@ -126,6 +177,60 @@
EXPECT_EQ(config.rtsDelay, properties.rts_delay);
}
+TEST_F(PortTest, TestGetUSBPortConfigSucess)
+{
+ using InventoryIntf =
+ sdbusplus::client::xyz::openbmc_project::inventory::Item<>;
+ sdbusplus::server::manager_t manager{ctx, InventoryIntf::namespace_path};
+
+ const USBPortConfigServerIntf::properties_t properties = {
+ .type = "USBPort",
+ .name = "USBPort1",
+ .device_address = "0xa",
+ .device_interface = 1,
+ .port = 1,
+ .mode = "RS485",
+ .baud_rate = 115200,
+ .rts_delay = 100};
+
+ ctx.spawn(TestGetUSBPortConfig(properties, true));
+
+ ctx.spawn(sdbusplus::async::sleep_for(ctx, 1s) |
+ sdbusplus::async::execution::then([&]() { ctx.request_stop(); }));
+
+ ctx.request_name(entity_manager::EntityManagerInterface::serviceName);
+ ctx.run();
+
+ EXPECT_EQ(getPortConfigPassed, true);
+}
+
+TEST_F(PortTest, TestGetUSBPortConfigFailureForInvalidPortMode)
+{
+ using InventoryIntf =
+ sdbusplus::client::xyz::openbmc_project::inventory::Item<>;
+ sdbusplus::server::manager_t manager{ctx, InventoryIntf::namespace_path};
+
+ const USBPortConfigServerIntf::properties_t properties = {
+ .type = "USBPort",
+ .name = "USBPort1",
+ .device_address = "0xa",
+ .device_interface = 1,
+ .port = 1,
+ .mode = "RSXXX", // Invalid port mode
+ .baud_rate = 115200,
+ .rts_delay = 100};
+
+ ctx.spawn(TestGetUSBPortConfig(properties, false));
+
+ ctx.spawn(sdbusplus::async::sleep_for(ctx, 1s) |
+ sdbusplus::async::execution::then([&]() { ctx.request_stop(); }));
+
+ ctx.request_name(entity_manager::EntityManagerInterface::serviceName);
+ ctx.run();
+
+ EXPECT_EQ(getPortConfigPassed, false);
+}
+
TEST_F(PortTest, TestReadHoldingRegisterSuccess)
{
PortConfigIntf::Config config = {};