From 7a78fd56f70b4739d6002ae6bd8ca2a70a2e96e3 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Sun, 25 Apr 2021 10:03:47 +0200 Subject: [PATCH] rdtech-dps: research to reduce serial comm transfer volume again The DPS devices default to a rather low UART communication bitrate. The generous retrieval of more Modbus registers than strictly necessary for the periodic acquisition of analog channel values and state tracking may or may not be an issue. Instrument all call sites in api.c to the device state gathering calls such that the protocol.c side transparently can reduce the transfer volume depending on which level of detail the caller may expect given its current context. Under optimal circumstances this might cut the traffic in half, which DPS devices may benefit from. This commit adds the infrastructure to potentially reduce transfer volume, but does not change behaviour. The "config" reasons may need further partitioning if Modbus register access really turns out to be a severe bottleneck. The API lends itself to that extension easily. The approach which gets prepared in this commit needs to get runtime tested by those who got access to the hardware. --- src/hardware/rdtech-dps/api.c | 24 +++++++-------- src/hardware/rdtech-dps/protocol.c | 49 +++++++++++++++++++++++++----- src/hardware/rdtech-dps/protocol.h | 8 ++++- 3 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/hardware/rdtech-dps/api.c b/src/hardware/rdtech-dps/api.c index 568ba575..1c67ea49 100644 --- a/src/hardware/rdtech-dps/api.c +++ b/src/hardware/rdtech-dps/api.c @@ -279,7 +279,7 @@ static int config_get(uint32_t key, GVariant **data, ret = sr_sw_limits_config_get(&devc->limits, key, data); break; case SR_CONF_ENABLED: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_OUTPUT_ENABLED)) @@ -287,7 +287,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_boolean(state.output_enabled); break; case SR_CONF_REGULATION: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_REGULATION_CC)) @@ -296,7 +296,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_string(cc_text); break; case SR_CONF_VOLTAGE: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_VOLTAGE)) @@ -304,7 +304,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_double(state.voltage); break; case SR_CONF_VOLTAGE_TARGET: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_VOLTAGE_TARGET)) @@ -312,7 +312,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_double(state.voltage_target); break; case SR_CONF_CURRENT: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_CURRENT)) @@ -320,7 +320,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_double(state.current); break; case SR_CONF_CURRENT_LIMIT: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_CURRENT_LIMIT)) @@ -328,7 +328,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_double(state.current_limit); break; case SR_CONF_OVER_VOLTAGE_PROTECTION_ENABLED: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_PROTECT_ENABLED)) @@ -336,7 +336,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_boolean(state.protect_enabled); break; case SR_CONF_OVER_VOLTAGE_PROTECTION_ACTIVE: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_PROTECT_OVP)) @@ -344,7 +344,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_boolean(state.protect_ovp); break; case SR_CONF_OVER_VOLTAGE_PROTECTION_THRESHOLD: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_OUTPUT_ENABLED)) @@ -352,7 +352,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_double(state.ovp_threshold); break; case SR_CONF_OVER_CURRENT_PROTECTION_ENABLED: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_PROTECT_ENABLED)) @@ -360,7 +360,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_boolean(state.protect_enabled); break; case SR_CONF_OVER_CURRENT_PROTECTION_ACTIVE: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_PROTECT_OCP)) @@ -368,7 +368,7 @@ static int config_get(uint32_t key, GVariant **data, *data = g_variant_new_boolean(state.protect_ocp); break; case SR_CONF_OVER_CURRENT_PROTECTION_THRESHOLD: - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_CONFIG); if (ret != SR_OK) return ret; if (!(state.mask & STATE_OCP_THRESHOLD)) diff --git a/src/hardware/rdtech-dps/protocol.c b/src/hardware/rdtech-dps/protocol.c index 9f45fc4f..7005a7a1 100644 --- a/src/hardware/rdtech-dps/protocol.c +++ b/src/hardware/rdtech-dps/protocol.c @@ -254,10 +254,11 @@ static int send_value(const struct sr_dev_inst *sdi, * when the UART bitrate is only 9600bps? */ SR_PRIV int rdtech_dps_get_state(const struct sr_dev_inst *sdi, - struct rdtech_dps_state *state) + struct rdtech_dps_state *state, enum rdtech_dps_state_context reason) { struct dev_context *devc; struct sr_modbus_dev_inst *modbus; + gboolean get_config, get_init_state, get_curr_meas; uint16_t registers[12]; int ret; const uint8_t *rdptr; @@ -276,6 +277,37 @@ SR_PRIV int rdtech_dps_get_state(const struct sr_dev_inst *sdi, if (!state) return SR_ERR_ARG; + /* Determine the requested level of response detail. */ + get_config = FALSE; + get_init_state = FALSE; + get_curr_meas = FALSE; + switch (reason) { + case ST_CTX_CONFIG: + get_config = TRUE; + get_init_state = TRUE; + get_curr_meas = TRUE; + break; + case ST_CTX_PRE_ACQ: + get_init_state = TRUE; + get_curr_meas = TRUE; + break; + case ST_CTX_IN_ACQ: + get_curr_meas = TRUE; + break; + default: + /* EMPTY */ + break; + } + /* + * TODO Make use of this information to reduce the transfer + * volume, especially on low bitrate serial connections. Though + * the device firmware's samplerate is probably more limiting + * than communication bandwidth is. + */ + (void)get_config; + (void)get_init_state; + (void)get_curr_meas; + switch (devc->model->model_type) { case MODEL_DPS: /* @@ -285,9 +317,6 @@ SR_PRIV int rdtech_dps_get_state(const struct sr_dev_inst *sdi, * and the sequence of the registers and how to interpret * their bit fields. But then this is not too unusual for * a hardware specific device driver ... - * - * TODO Optionally reduce the transfer volume depending - * on the caller specified state query context. */ g_mutex_lock(&devc->rw_mutex); ret = rdtech_dps_read_holding_registers(modbus, @@ -393,7 +422,13 @@ SR_PRIV int rdtech_dps_get_state(const struct sr_dev_inst *sdi, return SR_ERR_ARG; } - /* Store gathered details in the high level container. */ + /* + * Store gathered details in the high level container. + * + * TODO Make use of the caller's context. The register access + * code path above need not have gathered every detail in every + * invocation. + */ memset(state, 0, sizeof(*state)); state->lock = is_lock; state->mask |= STATE_LOCK; @@ -551,7 +586,7 @@ SR_PRIV int rdtech_dps_seed_receive(const struct sr_dev_inst *sdi) struct rdtech_dps_state state; int ret; - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_PRE_ACQ); if (ret != SR_OK) return ret; @@ -586,7 +621,7 @@ SR_PRIV int rdtech_dps_receive_data(int fd, int revents, void *cb_data) devc = sdi->priv; /* Get the device's current state. */ - ret = rdtech_dps_get_state(sdi, &state); + ret = rdtech_dps_get_state(sdi, &state, ST_CTX_IN_ACQ); if (ret != SR_OK) return ret; diff --git a/src/hardware/rdtech-dps/protocol.h b/src/hardware/rdtech-dps/protocol.h index 26c196ff..456e2041 100644 --- a/src/hardware/rdtech-dps/protocol.h +++ b/src/hardware/rdtech-dps/protocol.h @@ -86,8 +86,14 @@ struct rdtech_dps_state { float voltage, current, power; }; +enum rdtech_dps_state_context { + ST_CTX_NONE, + ST_CTX_CONFIG, + ST_CTX_PRE_ACQ, + ST_CTX_IN_ACQ, +}; SR_PRIV int rdtech_dps_get_state(const struct sr_dev_inst *sdi, - struct rdtech_dps_state *state); + struct rdtech_dps_state *state, enum rdtech_dps_state_context reason); SR_PRIV int rdtech_dps_set_state(const struct sr_dev_inst *sdi, struct rdtech_dps_state *state); -- 2.30.2