From 5e742bb49ba9fe13086ebe338161ae101ee71935 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89rico=20Nogueira?= Date: Mon, 28 Oct 2024 17:47:15 -0300 Subject: [PATCH] utcaApp: remove usage of sentinel parameters. This is a code cleanup, to avoid having to expose UDriver::parameter_props to specific drivers. The cleanup moves all handling of special cases (decoder_controller and read_only) into the ParamInit class, instead of requiring passing UDriver static inline members to it. This allowed us to add a ParamInit::set_dc() function, which is now used by the drivers which previously had to access parameter_props directly. This also simplifies the UDriver constructor, since it can now determine the need for p_tmp more easily, as well as the parameter properties. It also allowed us to add more sanity checks, in this case to UDriver::writeGeneral(), to report writes into read-only parameters. --- utcaApp/src/afc_timing.cpp | 4 +-- utcaApp/src/fmcpico1m_4ch.cpp | 4 +-- utcaApp/src/rtmlamp.cpp | 4 +-- utcaApp/src/udriver.h | 56 +++++++++++++++++++++++++++-------- 4 files changed, 46 insertions(+), 22 deletions(-) diff --git a/utcaApp/src/afc_timing.cpp b/utcaApp/src/afc_timing.cpp index 6cecfc9..ec9852c 100644 --- a/utcaApp/src/afc_timing.cpp +++ b/utcaApp/src/afc_timing.cpp @@ -56,7 +56,7 @@ class AFCTiming: public UDriver { {"CH_POL", p_decoder_controller}, {"CH_LOG", p_decoder_controller}, {"CH_ITL", p_decoder_controller}, - {"CH_SRC", p_ch_src}, + ParamInit{"CH_SRC", p_ch_src}.set_dc(), {"CH_DIR", p_decoder_controller}, {"CH_PULSES", p_decoder_controller}, ParamInit{"CH_COUNT_RST", p_decoder_controller}.set_wo(), @@ -72,8 +72,6 @@ class AFCTiming: public UDriver { dec.set_devinfo(v); ctl.set_devinfo(v); - parameter_props.at(p_ch_src).write_decoder_controller = true; - createParam("FREQ", asynParamFloat64, &p_freq); read_parameters(); diff --git a/utcaApp/src/fmcpico1m_4ch.cpp b/utcaApp/src/fmcpico1m_4ch.cpp index 6b21055..af2164f 100644 --- a/utcaApp/src/fmcpico1m_4ch.cpp +++ b/utcaApp/src/fmcpico1m_4ch.cpp @@ -20,7 +20,7 @@ class FMCPico: public UDriver { ::number_of_channels, { }, { - {"RANGE", p_range}, + ParamInit{"RANGE", p_range}.set_dc(), }, &ctl), dec(bars), @@ -30,8 +30,6 @@ class FMCPico: public UDriver { dec.set_devinfo(v); ctl.set_devinfo(v); - parameter_props.at(p_range).write_decoder_controller = true; - read_parameters(); } diff --git a/utcaApp/src/rtmlamp.cpp b/utcaApp/src/rtmlamp.cpp index 10ed018..611c010 100644 --- a/utcaApp/src/rtmlamp.cpp +++ b/utcaApp/src/rtmlamp.cpp @@ -29,7 +29,7 @@ class RtmLamp: public UDriver { {"AMP_STATUS", p_read_only}, {"AMP_STATUS_LATCH", p_read_only}, {"AMP_EN", p_decoder_controller}, - {"MODE", p_mode}, + ParamInit{"MODE", p_mode}.set_dc(), {"TRIG_EN", p_decoder_controller}, {"RST_LATCH", p_decoder_controller}, {"PI_KP", p_decoder_controller}, @@ -51,8 +51,6 @@ class RtmLamp: public UDriver { dec.set_devinfo(v); ctl.set_devinfo(v); - parameter_props.at(p_mode).write_decoder_controller = true; - read_parameters(); } diff --git a/utcaApp/src/udriver.h b/utcaApp/src/udriver.h index cc6149c..424a9b4 100644 --- a/utcaApp/src/udriver.h +++ b/utcaApp/src/udriver.h @@ -24,20 +24,47 @@ #include "enumerate.h" #include "pcie-single.h" +namespace { +struct read_only_struct {} const p_read_only; +struct decoder_controller_struct {} const p_decoder_controller; +} + struct ParamInit { const char *name; - int ¶meter; + int *parameter = nullptr; std::optional number_of_channels = std::nullopt; asynParamType type; + bool decoder_controller = false; + bool read_only = false; bool write_only = false; ParamInit(const char *name, int ¶meter, asynParamType type = asynParamInt32): name(name), - parameter(parameter), + parameter(¶meter), type(type) { } + ParamInit(const char *name, const decoder_controller_struct &, asynParamType type = asynParamInt32): + name(name), + type(type), + decoder_controller(true) + { + } + + ParamInit(const char *name, const read_only_struct &, asynParamType type = asynParamInt32): + name(name), + type(type), + read_only(true) + { + } + + ParamInit set_dc() + { + decoder_controller = true; + return *this; + } + ParamInit set_nc(unsigned new_number_of_channels) { number_of_channels = new_number_of_channels; @@ -59,17 +86,17 @@ class UDriver: public asynPortDriver { static const unsigned UDRIVER_PARAMS = 2; unsigned scan_counter = 0; - protected: struct parameter_props { unsigned number_of_channels; asynParamType type; bool is_general; + bool read_only; bool write_only; bool write_decoder_controller; }; std::vector parameter_props; - static inline int p_read_only = 0, p_decoder_controller = 0; + protected: unsigned number_of_channels; int port_number; @@ -105,18 +132,13 @@ class UDriver: public asynPortDriver { parameter_props.resize(UDRIVER_PARAMS); auto create_params = [this](auto const &nai, bool is_general) { - for (auto &&[i, v]: enumerate(nai)) { + for (auto &[str, p, nc, t, dc, ro, wo]: nai) { int p_tmp; - auto &&[str, p, nc, t, wo] = v; - /* if p is p_read_only or p_decoder_controller, we use a - * temporary variable to avoid any issues with simultaneous - * access to them */ - bool use_tmp = &p == &p_read_only || &p == &p_decoder_controller; - int *pp = use_tmp ? &p_tmp : &p; + int *pp = p ? p : &p_tmp; createParam(str, t, pp); - if (&p == &p_decoder_controller) + if (dc) assert(this->generic_decoder_controller); assert((int)parameter_props.size() == *pp); @@ -124,8 +146,9 @@ class UDriver: public asynPortDriver { .number_of_channels = nc ? *nc : this->number_of_channels, .type = t, .is_general = is_general, + .read_only = ro, .write_only = wo, - .write_decoder_controller = (&p == &p_decoder_controller), + .write_decoder_controller = dc, }); } }; @@ -285,6 +308,13 @@ class UDriver: public asynPortDriver { if ((unsigned)function >= parameter_props.size()) return call_write_impl(); + if (parameter_props.at(function).read_only) { + epicsSnprintf(pasynUser->errorMessage, pasynUser->errorMessageSize, + "writeGeneral: %s: read-only parameter", param_name); + + return asynError; + } + if (parameter_props.at(function).is_general && addr != 0) { epicsSnprintf(pasynUser->errorMessage, pasynUser->errorMessageSize, "writeGeneral: %s: general parameter with addr=%d (should be 0)", param_name, addr);