From ea40b9819e324fd82ff12a74de4046c1e5bf9a5c Mon Sep 17 00:00:00 2001 From: Michael D Starch Date: Thu, 16 May 2024 14:41:27 -0700 Subject: [PATCH] Fixing review comments --- Drv/Ip/test/ut/SocketTestHelper.cpp | 7 +++++++ Drv/Ip/test/ut/SocketTestHelper.hpp | 6 ++++++ Drv/LinuxGpioDriver/LinuxGpioDriverComponentImpl.hpp | 4 ++-- Drv/TcpClient/test/ut/TcpClientTester.cpp | 3 +-- Drv/TcpServer/test/ut/TcpServerTester.cpp | 6 ++---- Drv/Udp/test/ut/UdpTester.cpp | 3 +-- Os/Posix/Task.cpp | 2 +- Os/Posix/Task.hpp | 3 ++- Os/Stub/test/DefaultTask.cpp | 6 ++++-- Os/Stub/test/ut/StubFileTests.cpp | 1 + Os/Stub/test/ut/StubTaskTests.cpp | 1 + Os/test/ut/task/CommonTests.cpp | 2 +- Os/test/ut/task/TaskRules.cpp | 9 --------- Ref/Top/RefTopology.hpp | 3 +-- Svc/LinuxTimer/LinuxTimerComponentImplTaskDelay.cpp | 2 +- config/IpCfg.hpp | 2 +- 16 files changed, 32 insertions(+), 28 deletions(-) diff --git a/Drv/Ip/test/ut/SocketTestHelper.cpp b/Drv/Ip/test/ut/SocketTestHelper.cpp index 919bb714a3..f2df0e7741 100644 --- a/Drv/Ip/test/ut/SocketTestHelper.cpp +++ b/Drv/Ip/test/ut/SocketTestHelper.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace Drv { namespace Test { @@ -80,5 +81,11 @@ bool wait_on_started(Drv::IpSocket &socket, bool open, U32 iterations) { return false; } +U64 get_configured_delay_ms() { + printf("%llu %llu\n", static_cast(SOCKET_RETRY_INTERVAL.getSeconds()), static_cast(SOCKET_RETRY_INTERVAL.getUSeconds())); + return (static_cast(SOCKET_RETRY_INTERVAL.getSeconds()) * 1000) + + (static_cast(SOCKET_RETRY_INTERVAL.getUSeconds()) / 1000); +} + }; }; diff --git a/Drv/Ip/test/ut/SocketTestHelper.hpp b/Drv/Ip/test/ut/SocketTestHelper.hpp index 39583abda2..ad6e3a2de7 100644 --- a/Drv/Ip/test/ut/SocketTestHelper.hpp +++ b/Drv/Ip/test/ut/SocketTestHelper.hpp @@ -65,6 +65,12 @@ bool wait_on_change(Drv::IpSocket &socket, bool open, U32 iterations); */ bool wait_on_started(Drv::IpSocket &socket, bool open, U32 iterations); +/** + * Get the configured delay, converted to milliseconds + * @return SOCKET_RETRY_INTERVAL converted to milliseconds + */ +U64 get_configured_delay_ms(); + }; }; #endif diff --git a/Drv/LinuxGpioDriver/LinuxGpioDriverComponentImpl.hpp b/Drv/LinuxGpioDriver/LinuxGpioDriverComponentImpl.hpp index 9b811dd902..46c40318dc 100644 --- a/Drv/LinuxGpioDriver/LinuxGpioDriverComponentImpl.hpp +++ b/Drv/LinuxGpioDriver/LinuxGpioDriverComponentImpl.hpp @@ -46,8 +46,8 @@ namespace Drv { //! Start interrupt task Os::Task::Status startIntTask(Os::Task::ParamType priority = Os::Task::TASK_DEFAULT, - Os::Task::ParamType stackSize = Os::Task::TASK_DEFAULT, - Os::Task::ParamType cpuAffinity = Os::Task::TASK_DEFAULT); + Os::Task::ParamType stackSize = Os::Task::TASK_DEFAULT, + Os::Task::ParamType cpuAffinity = Os::Task::TASK_DEFAULT); //! configure GPIO enum GpioDirection { diff --git a/Drv/TcpClient/test/ut/TcpClientTester.cpp b/Drv/TcpClient/test/ut/TcpClientTester.cpp index 9199de4172..b50f0f180d 100644 --- a/Drv/TcpClient/test/ut/TcpClientTester.cpp +++ b/Drv/TcpClient/test/ut/TcpClientTester.cpp @@ -16,7 +16,6 @@ #include Os::Log logger; -const U64 SOCKET_RETRY_INTERVAL_MS = (SOCKET_RETRY_INTERVAL.getSeconds() * 1000) + (SOCKET_RETRY_INTERVAL.getUSeconds()/1000); namespace Drv { @@ -56,7 +55,7 @@ void TcpClientTester ::test_with_loop(U32 iterations, bool recv_thread) { if (not recv_thread) { status1 = this->component.open(); } else { - EXPECT_TRUE(Drv::Test::wait_on_change(this->component.getSocketHandler(), true, SOCKET_RETRY_INTERVAL_MS/10 + 1)); + EXPECT_TRUE(Drv::Test::wait_on_change(this->component.getSocketHandler(), true, Drv::Test::get_configured_delay_ms()/10 + 1)); } EXPECT_TRUE(this->component.getSocketHandler().isOpened()); status2 = server.open(); diff --git a/Drv/TcpServer/test/ut/TcpServerTester.cpp b/Drv/TcpServer/test/ut/TcpServerTester.cpp index 3d64883549..4572cf6705 100644 --- a/Drv/TcpServer/test/ut/TcpServerTester.cpp +++ b/Drv/TcpServer/test/ut/TcpServerTester.cpp @@ -17,8 +17,6 @@ Os::Log logger; -const U64 SOCKET_RETRY_INTERVAL_MS = (SOCKET_RETRY_INTERVAL.getSeconds() * 1000) + (SOCKET_RETRY_INTERVAL.getUSeconds()/1000); - namespace Drv { // ---------------------------------------------------------------------- @@ -40,7 +38,7 @@ void TcpServerTester ::test_with_loop(U32 iterations, bool recv_thread) { if (recv_thread) { Os::TaskString name("receiver thread"); this->component.start(name, true, Os::Task::TASK_DEFAULT, Os::Task::TASK_DEFAULT); - EXPECT_TRUE(Drv::Test::wait_on_started(this->component.getSocketHandler(), true, SOCKET_RETRY_INTERVAL_MS/10 + 1)); + EXPECT_TRUE(Drv::Test::wait_on_started(this->component.getSocketHandler(), true, Drv::Test::get_configured_delay_ms()/10 + 1)); } else { serverStat = this->component.startup(); ASSERT_EQ(serverStat, SOCK_SUCCESS) @@ -61,7 +59,7 @@ void TcpServerTester ::test_with_loop(U32 iterations, bool recv_thread) { if (not recv_thread) { status1 = this->component.open(); } else { - EXPECT_TRUE(Drv::Test::wait_on_change(this->component.getSocketHandler(), true, SOCKET_RETRY_INTERVAL_MS/10 + 1)); + EXPECT_TRUE(Drv::Test::wait_on_change(this->component.getSocketHandler(), true, Drv::Test::get_configured_delay_ms()/10 + 1)); } EXPECT_TRUE(this->component.getSocketHandler().isOpened()); diff --git a/Drv/Udp/test/ut/UdpTester.cpp b/Drv/Udp/test/ut/UdpTester.cpp index 2cac8ce653..d42de17d98 100644 --- a/Drv/Udp/test/ut/UdpTester.cpp +++ b/Drv/Udp/test/ut/UdpTester.cpp @@ -17,7 +17,6 @@ #include Os::Log logger; -const U64 SOCKET_RETRY_INTERVAL_MS = (SOCKET_RETRY_INTERVAL.getSeconds() * 1000) + (SOCKET_RETRY_INTERVAL.getUSeconds()/1000); namespace Drv { @@ -70,7 +69,7 @@ void UdpTester::test_with_loop(U32 iterations, bool recv_thread) { << "Port2: " << port2; } else { - EXPECT_TRUE(Drv::Test::wait_on_change(this->component.getSocketHandler(), true, SOCKET_RETRY_INTERVAL_MS/10 + 1)); + EXPECT_TRUE(Drv::Test::wait_on_change(this->component.getSocketHandler(), true, Drv::Test::get_configured_delay_ms()/10 + 1)); } EXPECT_TRUE(this->component.getSocketHandler().isOpened()); diff --git a/Os/Posix/Task.cpp b/Os/Posix/Task.cpp index 7157b7dc9d..e28266b1ae 100644 --- a/Os/Posix/Task.cpp +++ b/Os/Posix/Task.cpp @@ -17,7 +17,7 @@ namespace Os { namespace Posix { namespace Task { - bool PosixTask::s_permissions_reported = false; + std::atomic PosixTask::s_permissions_reported(false); static const PlatformIntType SCHED_POLICY = SCHED_RR; typedef void* (*pthread_func_ptr)(void*); diff --git a/Os/Posix/Task.hpp b/Os/Posix/Task.hpp index f97ed284cb..f040233574 100644 --- a/Os/Posix/Task.hpp +++ b/Os/Posix/Task.hpp @@ -5,6 +5,7 @@ #ifndef Os_Posix_Task_hpp_ #define Os_Posix_Task_hpp_ +#include #include #include @@ -105,7 +106,7 @@ namespace Task { Status create(const Os::Task::Arguments& arguments, const PosixTask::PermissionExpectation permissions); PosixTaskHandle m_handle; //!< Posix task tracking - static bool s_permissions_reported; //!< Permission errors have been reported + static std::atomic s_permissions_reported; //!< Permission errors have been reported }; } // end namespace Task } // end namespace Posix diff --git a/Os/Stub/test/DefaultTask.cpp b/Os/Stub/test/DefaultTask.cpp index 9961862737..32a5b3036c 100644 --- a/Os/Stub/test/DefaultTask.cpp +++ b/Os/Stub/test/DefaultTask.cpp @@ -1,6 +1,6 @@ // ====================================================================== -// \title Os/Posix/DefaultTask.cpp -// \brief sets default Os::Task to posix implementation via linker +// \title Os/Stub/test/DefaultTask.cpp +// \brief sets default Os::Task to test stub implementation via linker // ====================================================================== #include #include "Os/Task.hpp" @@ -13,6 +13,8 @@ namespace Os { Os::Stub::Task::Test::StaticData::data.lastCalled = Os::Stub::Task::Test::StaticData::LastFn::DELAY_FN; Os::Stub::Task::Test::StaticData::data.delay = interval; + // For testing stub, the default implementation of delay for a "task" is a busy wait. This acts as a synthetic + // albeit inefficient implementation. timeval start; timeval end; if (gettimeofday(&start, nullptr) == 0) { diff --git a/Os/Stub/test/ut/StubFileTests.cpp b/Os/Stub/test/ut/StubFileTests.cpp index 8ec6f35744..caea882ee3 100644 --- a/Os/Stub/test/ut/StubFileTests.cpp +++ b/Os/Stub/test/ut/StubFileTests.cpp @@ -211,5 +211,6 @@ TEST(FppTypes, FileModeEnum) { int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); + STest::Random::seed(); return RUN_ALL_TESTS(); } diff --git a/Os/Stub/test/ut/StubTaskTests.cpp b/Os/Stub/test/ut/StubTaskTests.cpp index 8e5d0d55d5..39ef52bd20 100644 --- a/Os/Stub/test/ut/StubTaskTests.cpp +++ b/Os/Stub/test/ut/StubTaskTests.cpp @@ -139,5 +139,6 @@ TEST_F(Interface, RegistryRemove) { int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); + STest::Random::seed(); return RUN_ALL_TESTS(); } diff --git a/Os/test/ut/task/CommonTests.cpp b/Os/test/ut/task/CommonTests.cpp index 33a44956e3..23927a9a44 100644 --- a/Os/test/ut/task/CommonTests.cpp +++ b/Os/test/ut/task/CommonTests.cpp @@ -8,7 +8,7 @@ #include "Os/test/ut/task/RulesHeaders.hpp" #include "Fw/Types/String.hpp" -static const U32 RANDOM_BOUND = 1000; +static constexpr U32 RANDOM_BOUND = 1000; namespace Os { namespace Test { diff --git a/Os/test/ut/task/TaskRules.cpp b/Os/test/ut/task/TaskRules.cpp index fd40b7c23b..bca1289696 100644 --- a/Os/test/ut/task/TaskRules.cpp +++ b/Os/test/ut/task/TaskRules.cpp @@ -42,8 +42,6 @@ bool Os::Test::Task::Tester::Start::precondition( void Os::Test::Task::Tester::Start::action( Os::Test::Task::Tester &state //!< The test state ) { - printf("--> Rule: Start \n"); - std::shared_ptr new_task = std::make_shared(); state.m_tasks.push_back(new_task); @@ -76,7 +74,6 @@ bool Os::Test::Task::Tester::Join::precondition( void Os::Test::Task::Tester::Join::action( Os::Test::Task::Tester &state //!< The test state ) { - printf("--> Rule: Join\n"); TestTaskInfo joiner_task; const U32 random_index = STest::Pick::lowerUpper(0, state.m_tasks.size() - 1); @@ -121,7 +118,6 @@ bool Os::Test::Task::Tester::CheckState::precondition( void Os::Test::Task::Tester::CheckState::action( Os::Test::Task::Tester &state //!< The test state ) { - printf("--> Rule: Check State\n"); std::shared_ptr task; const U32 random_index = STest::Pick::lowerUpper(0, state.m_tasks.size()); @@ -153,7 +149,6 @@ bool Os::Test::Task::Tester::Delay::precondition( void Os::Test::Task::Tester::Delay::action( Os::Test::Task::Tester &state //!< The test state ) { - printf("--> Rule: Delay\n"); const U32 delay_micro_seconds = 5; //STest::Pick::lowerUpper(0, MAX_DELAY_MICRO_SECONDS); Fw::Time delay(delay_micro_seconds / 1000000, delay_micro_seconds % 1000000); @@ -187,7 +182,6 @@ bool Os::Test::Task::Tester::CheckTaskCount::precondition( void Os::Test::Task::Tester::CheckTaskCount::action( Os::Test::Task::Tester &state //!< The test state ) { - printf("--> Rule: CheckTaskCount \n"); FwSizeType count = TestTaskInfo::s_task_count; ASSERT_EQ(Os::Task::getNumTasks(), count) << "Task count miss-match"; } @@ -212,8 +206,6 @@ bool Os::Test::Task::Tester::JoinInvalidState::precondition( void Os::Test::Task::Tester::JoinInvalidState::action( Os::Test::Task::Tester &state //!< The test state ) { - printf("--> Rule: JoinInvalidStatus\n"); - std::shared_ptr new_task = std::make_shared(); ASSERT_NE(new_task->m_task.join(), Os::Task::Status::OP_OK); } @@ -238,7 +230,6 @@ bool Os::Test::Task::Tester::StartIllegalRoutine::precondition( void Os::Test::Task::Tester::StartIllegalRoutine::action( Os::Test::Task::Tester &state //!< The test state ) { - printf("--> Rule: StartIllegalRoutine \n"); std::shared_ptr new_task = std::make_shared(); state.m_tasks.push_back(new_task); diff --git a/Ref/Top/RefTopology.hpp b/Ref/Top/RefTopology.hpp index 1be102ef19..7c7d676348 100644 --- a/Ref/Top/RefTopology.hpp +++ b/Ref/Top/RefTopology.hpp @@ -69,14 +69,13 @@ void teardownTopology(const TopologyState& state); * * The reference topology does not have a true 1Hz input clock for the rate group driver because it is designed to * operate across various computing endpoints (e.g. laptops) where a clear 1Hz source may not be easily and generically - * achieved. This function mimics the cycling via a Task::delay(milliseconds) loop that manually invokes the ISR call + * achieved. This function mimics the cycling via a Task::delay(Fw::Time()) loop that manually invokes the ISR call * to the example block driver. * * This loop is stopped via a startSimulatedCycle call. * * Note: projects should replace this with a component that produces an output port call at the appropriate frequency. * - * \param milliseconds: milliseconds to delay for each cycle. Default: 1000 or 1Hz. */ void startSimulatedCycle(Fw::Time interval); diff --git a/Svc/LinuxTimer/LinuxTimerComponentImplTaskDelay.cpp b/Svc/LinuxTimer/LinuxTimerComponentImplTaskDelay.cpp index 13a95afe20..ef2d02eb92 100644 --- a/Svc/LinuxTimer/LinuxTimerComponentImplTaskDelay.cpp +++ b/Svc/LinuxTimer/LinuxTimerComponentImplTaskDelay.cpp @@ -19,7 +19,7 @@ namespace Svc { void LinuxTimerComponentImpl::startTimer(NATIVE_INT_TYPE interval) { while (true) { - Os::Task::delay(Fw::Time(static_cast(interval/1000), static_cast((interval*1000) % 1000000))); + Os::Task::delay(Fw::Time(static_cast(interval/1000), static_cast((interval % 1000) * 1000))); this->m_mutex.lock(); bool quit = this->m_quit; this->m_mutex.unLock(); diff --git a/config/IpCfg.hpp b/config/IpCfg.hpp index 09c508384b..60daae5adb 100644 --- a/config/IpCfg.hpp +++ b/config/IpCfg.hpp @@ -21,7 +21,7 @@ enum IpCfg { SOCKET_MAX_ITERATIONS = 0xFFFF, // Maximum send/recv attempts before an error is returned SOCKET_MAX_HOSTNAME_SIZE = 256 // Maximum stored hostname }; -const Fw::Time SOCKET_RETRY_INTERVAL = Fw::Time(1, 0); +static const Fw::Time SOCKET_RETRY_INTERVAL = Fw::Time(1, 0); #endif //REF_IPCFG_HPP