From 6add9aad5d56e0c6360f438a9f3a40163138d644 Mon Sep 17 00:00:00 2001 From: Dhanya Thattil Date: Mon, 21 Oct 2024 16:25:07 +0200 Subject: [PATCH] Dev/proper free (#1005) * first draft of fixing the free function available within the class * removed class member function freeSharedmemory for both Detector and Module; made the free function freeSharedmemory accessible to python interface; setHostname if there is already a module in shm will recreate the Detector object while freeing shm completely and keeping detsize and intitialchecks (previous commit), sethostname called from DetectorClass in virtual command to have one point of entry (previous commit), testing Module class frees shared memory using free function * Detector class: added copy and move constructor and assignmentoperators due to explicit destructor (DetectorImpl fwd declared), DetectorImpl class: included ZmqSocket to remove destructor (should not be virtual in any case), Module class: removed explciit destructor to allow compiler generated constructor and operators * formatting * minor fix for readme autocomplete * updated client version date --- python/src/detector.cpp | 6 +- python/src/detector_in.cpp | 4 +- slsDetectorSoftware/generator/readme.md | 2 +- slsDetectorSoftware/include/sls/Detector.h | 12 ++-- slsDetectorSoftware/src/Detector.cpp | 35 ++++++++--- slsDetectorSoftware/src/DetectorImpl.cpp | 68 ++-------------------- slsDetectorSoftware/src/DetectorImpl.h | 22 +------ slsDetectorSoftware/src/Module.cpp | 8 --- slsDetectorSoftware/src/Module.h | 6 -- slsDetectorSoftware/tests/CMakeLists.txt | 1 + slsDetectorSoftware/tests/test-Module.cpp | 25 ++++++-- slsSupportLib/include/sls/versionAPI.h | 2 +- 12 files changed, 71 insertions(+), 120 deletions(-) diff --git a/python/src/detector.cpp b/python/src/detector.cpp index fb96c3579..5ca05412b 100644 --- a/python/src/detector.cpp +++ b/python/src/detector.cpp @@ -21,11 +21,13 @@ void init_det(py::module &m) { using sls::Positions; using sls::Result; + m.def("freeSharedMemory", + (void (*)(const int, const int)) & sls::freeSharedMemory, + py::arg() = 0, py::arg() = -1); + py::class_ CppDetectorApi(m, "CppDetectorApi"); CppDetectorApi.def(py::init()); - CppDetectorApi.def("freeSharedMemory", - (void (Detector::*)()) & Detector::freeSharedMemory); CppDetectorApi.def("loadConfig", (void (Detector::*)(const std::string &)) & Detector::loadConfig, diff --git a/python/src/detector_in.cpp b/python/src/detector_in.cpp index ce784ee0e..e19f3c103 100644 --- a/python/src/detector_in.cpp +++ b/python/src/detector_in.cpp @@ -18,8 +18,10 @@ void init_det(py::module &m) { using sls::Positions; using sls::Result; + m.def("freeSharedMemory", (void (*)(const int, const int)) &sls::freeSharedMemory, py::arg() = 0, py::arg() = -1); + py::class_ CppDetectorApi(m, "CppDetectorApi"); CppDetectorApi.def(py::init()); - + [[FUNCTIONS]] } diff --git a/slsDetectorSoftware/generator/readme.md b/slsDetectorSoftware/generator/readme.md index 106d50e2c..2f8db837b 100644 --- a/slsDetectorSoftware/generator/readme.md +++ b/slsDetectorSoftware/generator/readme.md @@ -28,7 +28,7 @@ The dump.json is the AST of the file `slsDetectorPackage/slsSupportLib/src/ToStr ```sh # to generate the dump.json file cd slsSupportLib/src -clang++ -Xclang -ast-dump=json -Xclang -ast-dump-filter -Xclang StringTo -c ToString.cpp -I ../include/ -std=gnu++11 > ../../slsDetectorSoftware/generator/autocomplete/dump.json +clang++ -Xclang -ast-dump=json -Xclang -ast-dump-filter -Xclang StringTo -c ToString.cpp -I ../include/ -std=gnu++11 > ../../slsDetectorSoftware/generator/autocomplete/dump.json # clang version used: 14.0.0-1ubuntu1.1 ``` diff --git a/slsDetectorSoftware/include/sls/Detector.h b/slsDetectorSoftware/include/sls/Detector.h index a17b23a53..04abc8dba 100644 --- a/slsDetectorSoftware/include/sls/Detector.h +++ b/slsDetectorSoftware/include/sls/Detector.h @@ -20,7 +20,7 @@ class IpAddr; // Free function to avoid dependence on class // and avoid the option to free another objects // shm by mistake -void freeSharedMemory(int detectorIndex, int moduleIndex = -1); +void freeSharedMemory(const int detectorIndex = 0, const int moduleIndex = -1); /** * \class Detector @@ -46,9 +46,13 @@ class Detector { Detector(int shm_id = 0); ~Detector(); - /** Free the shared memory of this detector and all modules - belonging to it */ - void freeSharedMemory(); + // Disable copy since SharedMemory object is unique in DetectorImpl + Detector(const Detector &other) = delete; + Detector &operator=(const Detector &other) = delete; + + // Move constructor and assignment operator + Detector(Detector &&other) noexcept; + Detector &operator=(Detector &&other) noexcept; /** Frees shared memory before loading configuration file. Set up once normally */ diff --git a/slsDetectorSoftware/src/Detector.cpp b/slsDetectorSoftware/src/Detector.cpp index 00ddf5b47..a8e2ad2e2 100644 --- a/slsDetectorSoftware/src/Detector.cpp +++ b/slsDetectorSoftware/src/Detector.cpp @@ -23,7 +23,7 @@ namespace sls { -void freeSharedMemory(int detectorIndex, int moduleIndex) { +void freeSharedMemory(const int detectorIndex, const int moduleIndex) { // single module if (moduleIndex >= 0) { @@ -34,10 +34,10 @@ void freeSharedMemory(int detectorIndex, int moduleIndex) { return; } - // detector - multi module - get number of detectors from shm - SharedMemory detectorShm(detectorIndex, -1); int numDetectors = 0; + // detector - multi module - get number of detectors from shm + SharedMemory detectorShm(detectorIndex, -1); if (detectorShm.exists()) { detectorShm.openSharedMemory(false); numDetectors = detectorShm()->totalNumberOfModules; @@ -58,15 +58,19 @@ void freeSharedMemory(int detectorIndex, int moduleIndex) { using defs = slsDetectorDefs; Detector::Detector(int shm_id) : pimpl(make_unique(shm_id)) {} - Detector::~Detector() = default; +// Move constructor +Detector::Detector(Detector &&other) noexcept = default; + +// Move assignment operator +Detector &Detector::operator=(Detector &&other) noexcept = default; + // Configuration -void Detector::freeSharedMemory() { pimpl->freeSharedMemory(); } void Detector::loadConfig(const std::string &fname) { int shm_id = getShmId(); - freeSharedMemory(); + freeSharedMemory(shm_id); pimpl = make_unique(shm_id); LOG(logINFO) << "Loading configuration file: " << fname; loadParameters(fname); @@ -105,13 +109,30 @@ Result Detector::getHostname(Positions pos) const { } void Detector::setHostname(const std::vector &hostname) { + if (pimpl->hasModulesInSharedMemory()) { + LOG(logWARNING) << "There are already module(s) in shared memory." + "Freeing Shared memory now."; + auto numChannels = getDetectorSize(); + auto initialChecks = getInitialChecks(); + freeSharedMemory(getShmId()); + pimpl = make_unique(getShmId()); + setDetectorSize(numChannels); + setInitialChecks(initialChecks); + } pimpl->setHostname(hostname); } void Detector::setVirtualDetectorServers(int numServers, uint16_t startingPort) { validatePortRange(startingPort, numServers * 2); - pimpl->setVirtualDetectorServers(numServers, startingPort); + + std::vector hostnames; + for (int i = 0; i < numServers; ++i) { + // * 2 is for control and stop port + hostnames.push_back(std::string("localhost:") + + std::to_string(startingPort + i * 2)); + } + setHostname(hostnames); } int Detector::getShmId() const { return pimpl->getDetectorIndex(); } diff --git a/slsDetectorSoftware/src/DetectorImpl.cpp b/slsDetectorSoftware/src/DetectorImpl.cpp index c9e078034..e16e57f53 100644 --- a/slsDetectorSoftware/src/DetectorImpl.cpp +++ b/slsDetectorSoftware/src/DetectorImpl.cpp @@ -37,8 +37,6 @@ DetectorImpl::DetectorImpl(int detector_index, bool verify, bool update) setupDetector(verify, update); } -DetectorImpl::~DetectorImpl() = default; - void DetectorImpl::setupDetector(bool verify, bool update) { initSharedMemory(verify); initializeMembers(verify); @@ -59,51 +57,6 @@ void DetectorImpl::setAcquiringFlag(bool flag) { shm()->acquiringFlag = flag; } int DetectorImpl::getDetectorIndex() const { return detectorIndex; } -void DetectorImpl::freeSharedMemory(int detectorIndex, int detPos) { - // single - if (detPos >= 0) { - SharedMemory moduleShm(detectorIndex, detPos); - if (moduleShm.exists()) { - moduleShm.removeSharedMemory(); - } - return; - } - - // multi - get number of modules from shm - SharedMemory detectorShm(detectorIndex, -1); - int numModules = 0; - - if (detectorShm.exists()) { - detectorShm.openSharedMemory(false); - numModules = detectorShm()->totalNumberOfModules; - detectorShm.removeSharedMemory(); - } - - for (int i = 0; i < numModules; ++i) { - SharedMemory moduleShm(detectorIndex, i); - moduleShm.removeSharedMemory(); - } - - SharedMemory ctbShm(detectorIndex, -1, CtbConfig::shm_tag()); - if (ctbShm.exists()) - ctbShm.removeSharedMemory(); -} - -void DetectorImpl::freeSharedMemory() { - zmqSocket.clear(); - for (auto &module : modules) { - module->freeSharedMemory(); - } - modules.clear(); - - // clear detector shm - shm.removeSharedMemory(); - client_downstream = false; - - if (ctb_shm.exists()) - ctb_shm.removeSharedMemory(); -} - std::string DetectorImpl::getUserDetails() { if (modules.empty()) { return std::string("none"); @@ -242,24 +195,11 @@ std::string DetectorImpl::exec(const char *cmd) { return result; } -void DetectorImpl::setVirtualDetectorServers(const int numdet, - const uint16_t port) { - std::vector hostnames; - for (int i = 0; i < numdet; ++i) { - // * 2 is for control and stop port - hostnames.push_back(std::string("localhost:") + - std::to_string(port + i * 2)); - } - setHostname(hostnames); +bool DetectorImpl::hasModulesInSharedMemory() { + return (shm.exists() && shm()->totalNumberOfModules > 0); } void DetectorImpl::setHostname(const std::vector &name) { - // do not free always to allow the previous detsize/ initialchecks command - if (shm.exists() && shm()->totalNumberOfModules != 0) { - LOG(logWARNING) << "There are already module(s) in shared memory." - "Freeing Shared memory now."; - freeSharedMemory(); - } // could be called after freeing shm from API if (!shm.exists()) { setupDetector(); @@ -292,8 +232,8 @@ void DetectorImpl::addModule(const std::string &name) { // gotthard cannot have more than 2 modules (50um=1, 25um=2 if ((type == GOTTHARD || type == GOTTHARD2) && modules.size() > 2) { - freeSharedMemory(); - throw RuntimeError("Gotthard cannot have more than 2 modules"); + throw RuntimeError("Gotthard cannot have more than 2 modules. Please " + "free the shared memory and start again."); } auto pos = modules.size(); diff --git a/slsDetectorSoftware/src/DetectorImpl.h b/slsDetectorSoftware/src/DetectorImpl.h index a4484c243..c42d9767d 100644 --- a/slsDetectorSoftware/src/DetectorImpl.h +++ b/slsDetectorSoftware/src/DetectorImpl.h @@ -5,6 +5,7 @@ #include "CtbConfig.h" #include "SharedMemory.h" #include "sls/Result.h" +#include "sls/ZmqSocket.h" #include "sls/logger.h" #include "sls/sls_detector_defs.h" @@ -19,7 +20,6 @@ namespace sls { -class ZmqSocket; class detectorData; class Module; @@ -79,11 +79,6 @@ class DetectorImpl : public virtual slsDetectorDefs { explicit DetectorImpl(int detector_index = 0, bool verify = true, bool update = true); - /** - * Destructor - */ - virtual ~DetectorImpl(); - template struct NonDeduced { using type = CT; }; template Result Parallel(RT (Module::*somefunc)(CT...), @@ -198,14 +193,6 @@ class DetectorImpl : public virtual slsDetectorDefs { /** return detector index in shared memory */ int getDetectorIndex() const; - /** Free specific shared memory from the command line without creating - * object */ - static void freeSharedMemory(int detectorIndex, int detPos = -1); - - /** Free all modules from current multi Id shared memory and delete members - */ - void freeSharedMemory(); - /** Get user details of shared memory */ std::string getUserDetails(); @@ -215,12 +202,7 @@ class DetectorImpl : public virtual slsDetectorDefs { * default enabled */ void setInitialChecks(const bool value); - /** - * Connect to Virtual Detector Servers at local host - * @param numdet number of modules - * @param port starting port number - */ - void setVirtualDetectorServers(const int numdet, const uint16_t port); + bool hasModulesInSharedMemory(); /** Sets the hostname of all sls modules in shared memory and updates * local cache */ diff --git a/slsDetectorSoftware/src/Module.cpp b/slsDetectorSoftware/src/Module.cpp index 202209919..93a50789e 100644 --- a/slsDetectorSoftware/src/Module.cpp +++ b/slsDetectorSoftware/src/Module.cpp @@ -53,14 +53,6 @@ Module::Module(int det_id, int module_index, bool verify) initSharedMemory(type, det_id, verify); } -Module::~Module() = default; - -void Module::freeSharedMemory() { - if (shm.exists()) { - shm.removeSharedMemory(); - } -} - bool Module::isFixedPatternSharedMemoryCompatible() const { return (shm()->shmversion >= MODULE_SHMAPIVERSION); } diff --git a/slsDetectorSoftware/src/Module.h b/slsDetectorSoftware/src/Module.h index b9ccaf8bc..aa6f02f1a 100644 --- a/slsDetectorSoftware/src/Module.h +++ b/slsDetectorSoftware/src/Module.h @@ -75,12 +75,6 @@ class Module : public virtual slsDetectorDefs { verify is if shared memory version matches existing one */ explicit Module(int det_id = 0, int module_index = 0, bool verify = true); - virtual ~Module(); - - /** Frees shared memory and deletes shared memory structure - Safe to call only if detector shm also deleted or its numberOfModules is - updated */ - void freeSharedMemory(); bool isFixedPatternSharedMemoryCompatible() const; std::string getHostname() const; diff --git a/slsDetectorSoftware/tests/CMakeLists.txt b/slsDetectorSoftware/tests/CMakeLists.txt index a255c12b0..521deec4d 100755 --- a/slsDetectorSoftware/tests/CMakeLists.txt +++ b/slsDetectorSoftware/tests/CMakeLists.txt @@ -33,6 +33,7 @@ target_sources(tests PRIVATE target_include_directories(tests PUBLIC "$" + "$" PRIVATE ${SLS_INTERNAL_RAPIDJSON_DIR} ) \ No newline at end of file diff --git a/slsDetectorSoftware/tests/test-Module.cpp b/slsDetectorSoftware/tests/test-Module.cpp index 203ee30f3..5e9767c36 100644 --- a/slsDetectorSoftware/tests/test-Module.cpp +++ b/slsDetectorSoftware/tests/test-Module.cpp @@ -1,5 +1,6 @@ // SPDX-License-Identifier: LGPL-3.0-or-other // Copyright (C) 2021 Contributors to the SLS Detector Package +#include "Detector.h" #include "Module.h" #include "SharedMemory.h" #include "catch.hpp" @@ -10,7 +11,9 @@ using dt = slsDetectorDefs::detectorType; TEST_CASE("Construction with a defined detector type") { Module m(dt::EIGER); REQUIRE(m.getDetectorType() == dt::EIGER); - m.freeSharedMemory(); // clean up + freeSharedMemory(0, 0); // clean up + SharedMemory moduleShm(0, 0); + REQUIRE(moduleShm.exists() == false); } TEST_CASE("Read back detector type from shm") { @@ -23,7 +26,9 @@ TEST_CASE("Read back detector type from shm") { // Now both objects point to the same shm so we can only // free one! - m2.freeSharedMemory(); + freeSharedMemory(0, 0); + SharedMemory moduleShm(0, 0); + REQUIRE(moduleShm.exists() == false); } TEST_CASE("Is shm fixed pattern shm compatible") { @@ -41,25 +46,33 @@ TEST_CASE("Is shm fixed pattern shm compatible") { // Should fail since version is set to 0 REQUIRE(m.isFixedPatternSharedMemoryCompatible() == false); - m.freeSharedMemory(); + freeSharedMemory(0, 0); + SharedMemory moduleShm(0, 0); + REQUIRE(moduleShm.exists() == false); } TEST_CASE("Get default control port") { Module m(dt::MYTHEN3); REQUIRE(m.getControlPort() == 1952); - m.freeSharedMemory(); + freeSharedMemory(0, 0); + SharedMemory moduleShm(0, 0); + REQUIRE(moduleShm.exists() == false); } TEST_CASE("Get default stop port") { Module m(dt::GOTTHARD2); REQUIRE(m.getStopPort() == 1953); - m.freeSharedMemory(); + freeSharedMemory(0, 0); + SharedMemory moduleShm(0, 0); + REQUIRE(moduleShm.exists() == false); } TEST_CASE("Get default receiver TCP port") { Module m(dt::MYTHEN3); REQUIRE(m.getReceiverPort() == 1954); - m.freeSharedMemory(); + freeSharedMemory(0, 0); + SharedMemory moduleShm(0, 0); + REQUIRE(moduleShm.exists() == false); } } // namespace sls diff --git a/slsSupportLib/include/sls/versionAPI.h b/slsSupportLib/include/sls/versionAPI.h index d0ba5af7f..3e79c970f 100644 --- a/slsSupportLib/include/sls/versionAPI.h +++ b/slsSupportLib/include/sls/versionAPI.h @@ -10,5 +10,5 @@ #define APIMYTHEN3 "developer 0x241001" #define APIJUNGFRAU "developer 0x241001" #define APIGOTTHARD2 "developer 0x241007" -#define APILIB "developer 0x241014" #define APIRECEIVER "developer 0x241014" +#define APILIB "developer 0x241021"