rdtech-dps: layer separation between API and protocol, style nits
authorGerhard Sittig <gerhard.sittig@gmx.net>
Tue, 13 Apr 2021 21:52:59 +0000 (23:52 +0200)
committerGerhard Sittig <gerhard.sittig@gmx.net>
Sun, 25 Apr 2021 10:10:22 +0000 (12:10 +0200)
The existing power supply driver for Riden DPS/DPH devices unfortunately
duplicated intimate details of modbus communication and register layout
including interpretation of raw register values across several source
files. Do separate the physical transport and the register layout and
register fields interpretation layers. This brings the driver in line
with the usual api.c and protocol.c arrangement of responsibilities, and
prepares the future addition of other similar devices.

Address a few style nits while we are here. Include <config.h> in all
source files, and alpha-sort include directives. Address minor data type
nits. Reduce indentation in code paths. Propagate more error codes.
Update comments which lagged behind the code, adjust grammer (eliminate
"actual" false friends). Eliminate a few magic numbers and redundancies,
switch to the more recent endianess aware byte stream reader API where
beneficial, to eliminate redundant offset math.

Other nits seem to have gone unnoticed before: The indices of all sigrok
channels were identical (all zero). Signed print formats were used for
unsigned data.

This implementation compiles, but wasn't runtime tested due to lack of
hardware. A rough estimate of transfer volume and transport throughput
suggests that the 10ms cycle time no longer can be kept, may end up
around 25ms (40 cycles per second). This can get avoided by adding more
code to tell the configuration requests, the acquisition start, and the
measurements grabbing during acquisition apart. It's assumed though that
the 10ms glib poll interval did not translate to such fast measurement
gathering. The firmware may typically provide data at lower rates anyway.

The generic source code may look overly complicated or redundant at
first glance, but compilers' generated code will greatly get optimized.
Simplified maintenance because of reduced cognitive load is considered
more important.

src/hardware/rdtech-dps/api.c
src/hardware/rdtech-dps/protocol.c
src/hardware/rdtech-dps/protocol.h

index fad31d629434ef5ae99f166305fd6ba1e7a5e5ec..dda75896140cbd0efaed57d9b589934ed43bbd51 100644 (file)
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2018 James Churchill <pelrun@gmail.com>
  * Copyright (C) 2019 Frank Stettner <frank-stettner@gmx.net>
+ * Copyright (C) 2021 Gerhard Sittig <gerhard.sittig@gmx.net>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  */
 
 #include <config.h>
+
 #include <math.h>
+#include <string.h>
+
 #include "protocol.h"
 
 static const uint32_t scanopts[] = {
@@ -48,7 +52,7 @@ static const uint32_t devopts[] = {
        SR_CONF_OVER_CURRENT_PROTECTION_THRESHOLD | SR_CONF_GET | SR_CONF_SET,
 };
 
-/* Model ID, model name, max current, max voltage, max power */
+/* Model ID, model name, max current/voltage/power, current/voltage digits. */
 static const struct rdtech_dps_model supported_models[] = {
        { 3005, "DPS3005",  5, 30,  160, 3, 2 },
        { 5005, "DPS5005",  5, 50,  250, 3, 2 },
@@ -62,39 +66,42 @@ static struct sr_dev_driver rdtech_dps_driver_info;
 
 static struct sr_dev_inst *probe_device(struct sr_modbus_dev_inst *modbus)
 {
-       const struct rdtech_dps_model *model = NULL;
-       struct dev_context *devc;
-       struct sr_dev_inst *sdi;
        uint16_t id, version;
-       unsigned int i;
+       int ret;
+       const struct rdtech_dps_model *model;
+       size_t i;
+       struct sr_dev_inst *sdi;
+       struct dev_context *devc;
 
-       int ret = rdtech_dps_get_model_version(modbus, &id, &version);
+       ret = rdtech_dps_get_model_version(modbus, &id, &version);
        if (ret != SR_OK)
                return NULL;
-       for (i = 0; i < ARRAY_SIZE(supported_models); i++)
+       model = NULL;
+       for (i = 0; i < ARRAY_SIZE(supported_models); i++) {
                if (id == supported_models[i].id) {
                        model = &supported_models[i];
                        break;
                }
-       if (model == NULL) {
-               sr_err("Unknown model: %d.", id);
+       }
+       if (!model) {
+               sr_err("Unknown model: %u.", id);
                return NULL;
        }
 
-       sdi = g_malloc0(sizeof(struct sr_dev_inst));
+       sdi = g_malloc0(sizeof(*sdi));
        sdi->status = SR_ST_INACTIVE;
        sdi->vendor = g_strdup("RDTech");
        sdi->model = g_strdup(model->name);
-       sdi->version = g_strdup_printf("v%d", version);
+       sdi->version = g_strdup_printf("v%u", version);
        sdi->conn = modbus;
        sdi->driver = &rdtech_dps_driver_info;
        sdi->inst_type = SR_INST_MODBUS;
 
        sr_channel_new(sdi, 0, SR_CHANNEL_ANALOG, TRUE, "V");
-       sr_channel_new(sdi, 0, SR_CHANNEL_ANALOG, TRUE, "I");
-       sr_channel_new(sdi, 0, SR_CHANNEL_ANALOG, TRUE, "P");
+       sr_channel_new(sdi, 1, SR_CHANNEL_ANALOG, TRUE, "I");
+       sr_channel_new(sdi, 2, SR_CHANNEL_ANALOG, TRUE, "P");
 
-       devc = g_malloc0(sizeof(struct dev_context));
+       devc = g_malloc0(sizeof(*devc));
        sr_sw_limits_init(&devc->limits);
        devc->model = model;
        devc->current_multiplier = pow(10.0, model->current_digits);
@@ -121,8 +128,9 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
                .key = SR_CONF_MODBUSADDR,
                .data = g_variant_new_uint64(1),
        };
-       GSList *opts = options, *devices;
+       GSList *opts, *devices;
 
+       opts = options;
        if (!g_slist_find_custom(options, &default_serialcomm, config_compare))
                opts = g_slist_prepend(opts, &default_serialcomm);
        if (!g_slist_find_custom(options, &default_modbusaddr, config_compare))
@@ -140,12 +148,20 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
 
 static int dev_open(struct sr_dev_inst *sdi)
 {
-       struct sr_modbus_dev_inst *modbus = sdi->conn;
+       struct sr_modbus_dev_inst *modbus;
+       struct rdtech_dps_state state;
+       int ret;
 
+       modbus = sdi->conn;
        if (sr_modbus_open(modbus) < 0)
                return SR_ERR;
 
-       rdtech_dps_set_reg(sdi, REG_LOCK, 1);
+       memset(&state, 0, sizeof(state));
+       state.lock = TRUE;
+       state.mask |= STATE_LOCK;
+       ret = rdtech_dps_set_state(sdi, &state);
+       if (ret != SR_OK)
+               return ret;
 
        return SR_OK;
 }
@@ -153,12 +169,16 @@ static int dev_open(struct sr_dev_inst *sdi)
 static int dev_close(struct sr_dev_inst *sdi)
 {
        struct sr_modbus_dev_inst *modbus;
+       struct rdtech_dps_state state;
 
        modbus = sdi->conn;
        if (!modbus)
                return SR_ERR_BUG;
 
-       rdtech_dps_set_reg(sdi, REG_LOCK, 0);
+       memset(&state, 0, sizeof(state));
+       state.lock = FALSE;
+       state.mask |= STATE_LOCK;
+       (void)rdtech_dps_set_state(sdi, &state);
 
        return sr_modbus_close(modbus);
 }
@@ -167,8 +187,9 @@ static int config_get(uint32_t key, GVariant **data,
        const struct sr_dev_inst *sdi, const struct sr_channel_group *cg)
 {
        struct dev_context *devc;
+       struct rdtech_dps_state state;
        int ret;
-       uint16_t ivalue;
+       const char *cc_text;
 
        (void)cg;
 
@@ -181,51 +202,101 @@ static int config_get(uint32_t key, GVariant **data,
                ret = sr_sw_limits_config_get(&devc->limits, key, data);
                break;
        case SR_CONF_ENABLED:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_ENABLE, &ivalue)) == SR_OK)
-                       *data = g_variant_new_boolean(ivalue);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_OUTPUT_ENABLED))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_boolean(state.output_enabled);
                break;
        case SR_CONF_REGULATION:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_CV_CC, &ivalue)) != SR_OK)
-                       break;
-               *data = g_variant_new_string((ivalue == MODE_CC) ? "CC" : "CV");
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_REGULATION_CC))
+                       return SR_ERR_DATA;
+               cc_text = state.regulation_cc ? "CC" : "CV";
+               *data = g_variant_new_string(cc_text);
                break;
        case SR_CONF_VOLTAGE:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_UOUT, &ivalue)) == SR_OK)
-                       *data = g_variant_new_double((float)ivalue / devc->voltage_multiplier);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_VOLTAGE))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_double(state.voltage);
                break;
        case SR_CONF_VOLTAGE_TARGET:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_USET, &ivalue)) == SR_OK)
-                       *data = g_variant_new_double((float)ivalue / devc->voltage_multiplier);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_VOLTAGE_TARGET))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_double(state.voltage_target);
                break;
        case SR_CONF_CURRENT:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_IOUT, &ivalue)) == SR_OK)
-                       *data = g_variant_new_double((float)ivalue / devc->current_multiplier);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_CURRENT))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_double(state.current);
                break;
        case SR_CONF_CURRENT_LIMIT:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_ISET, &ivalue)) == SR_OK)
-                       *data = g_variant_new_double((float)ivalue / devc->current_multiplier);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_CURRENT_LIMIT))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_double(state.current_limit);
                break;
        case SR_CONF_OVER_VOLTAGE_PROTECTION_ENABLED:
-               *data = g_variant_new_boolean(TRUE);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_PROTECT_ENABLED))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_boolean(state.protect_enabled);
                break;
        case SR_CONF_OVER_VOLTAGE_PROTECTION_ACTIVE:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_PROTECT, &ivalue)) == SR_OK)
-                       *data = g_variant_new_boolean(ivalue == STATE_OVP);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_PROTECT_OVP))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_boolean(state.protect_ovp);
                break;
        case SR_CONF_OVER_VOLTAGE_PROTECTION_THRESHOLD:
-               if ((ret = rdtech_dps_get_reg(sdi, PRE_OVPSET, &ivalue)) == SR_OK)
-                       *data = g_variant_new_double((float)ivalue / devc->voltage_multiplier);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_OUTPUT_ENABLED))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_double(state.ovp_threshold);
                break;
        case SR_CONF_OVER_CURRENT_PROTECTION_ENABLED:
-               *data = g_variant_new_boolean(TRUE);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_PROTECT_ENABLED))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_boolean(state.protect_enabled);
                break;
        case SR_CONF_OVER_CURRENT_PROTECTION_ACTIVE:
-               if ((ret = rdtech_dps_get_reg(sdi, REG_PROTECT, &ivalue)) == SR_OK)
-                       *data = g_variant_new_boolean(ivalue == STATE_OCP);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_PROTECT_OCP))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_boolean(state.protect_ocp);
                break;
        case SR_CONF_OVER_CURRENT_PROTECTION_THRESHOLD:
-               if ((ret = rdtech_dps_get_reg(sdi, PRE_OCPSET, &ivalue)) == SR_OK)
-                       *data = g_variant_new_double((float)ivalue / devc->current_multiplier);
+               ret = rdtech_dps_get_state(sdi, &state);
+               if (ret != SR_OK)
+                       return ret;
+               if (!(state.mask & STATE_OCP_THRESHOLD))
+                       return SR_ERR_DATA;
+               *data = g_variant_new_double(state.ocp_threshold);
                break;
        default:
                return SR_ERR_NA;
@@ -238,29 +309,37 @@ static int config_set(uint32_t key, GVariant *data,
        const struct sr_dev_inst *sdi, const struct sr_channel_group *cg)
 {
        struct dev_context *devc;
+       struct rdtech_dps_state state;
 
        (void)cg;
 
        devc = sdi->priv;
+       memset(&state, 0, sizeof(state));
 
        switch (key) {
        case SR_CONF_LIMIT_SAMPLES:
        case SR_CONF_LIMIT_MSEC:
                return sr_sw_limits_config_set(&devc->limits, key, data);
        case SR_CONF_ENABLED:
-               return rdtech_dps_set_reg(sdi, REG_ENABLE, g_variant_get_boolean(data));
+               state.output_enabled = g_variant_get_boolean(data);
+               state.mask |= STATE_OUTPUT_ENABLED;
+               return rdtech_dps_set_state(sdi, &state);
        case SR_CONF_VOLTAGE_TARGET:
-               return rdtech_dps_set_reg(sdi, REG_USET,
-                       g_variant_get_double(data) * devc->voltage_multiplier);
+               state.voltage_target = g_variant_get_double(data);
+               state.mask |= STATE_VOLTAGE_TARGET;
+               return rdtech_dps_set_state(sdi, &state);
        case SR_CONF_CURRENT_LIMIT:
-               return rdtech_dps_set_reg(sdi, REG_ISET,
-                       g_variant_get_double(data) * devc->current_multiplier);
+               state.current_limit = g_variant_get_double(data);
+               state.mask |= STATE_CURRENT_LIMIT;
+               return rdtech_dps_set_state(sdi, &state);
        case SR_CONF_OVER_VOLTAGE_PROTECTION_THRESHOLD:
-               return rdtech_dps_set_reg(sdi, PRE_OVPSET,
-                       g_variant_get_double(data) * devc->voltage_multiplier);
+               state.ovp_threshold = g_variant_get_double(data);
+               state.mask |= STATE_OVP_THRESHOLD;
+               return rdtech_dps_set_state(sdi, &state);
        case SR_CONF_OVER_CURRENT_PROTECTION_THRESHOLD:
-               return rdtech_dps_set_reg(sdi, PRE_OCPSET,
-                       g_variant_get_double(data) * devc->current_multiplier);
+               state.ocp_threshold = g_variant_get_double(data);
+               state.mask |= STATE_OCP_THRESHOLD;
+               return rdtech_dps_set_state(sdi, &state);
        default:
                return SR_ERR_NA;
        }
@@ -298,25 +377,20 @@ static int dev_acquisition_start(const struct sr_dev_inst *sdi)
 {
        struct dev_context *devc;
        struct sr_modbus_dev_inst *modbus;
-       uint16_t registers[3];
        int ret;
 
        modbus = sdi->conn;
        devc = sdi->priv;
 
-       /* Prefill actual states */
-       g_mutex_lock(&devc->rw_mutex);
-       ret = rdtech_dps_read_holding_registers(modbus, REG_PROTECT, 3, registers);
-       g_mutex_unlock(&devc->rw_mutex);
+       /* Seed internal state from current data. */
+       ret = rdtech_dps_seed_receive(sdi);
        if (ret != SR_OK)
                return ret;
-       devc->actual_ovp_state = RB16(registers + 0) == STATE_OVP;
-       devc->actual_ocp_state = RB16(registers + 0) == STATE_OCP;
-       devc->actual_regulation_state = RB16(registers + 1);
-       devc->actual_output_state = RB16(registers + 2);
 
-       if ((ret = sr_modbus_source_add(sdi->session, modbus, G_IO_IN, 10,
-                       rdtech_dps_receive_data, (void *)sdi)) != SR_OK)
+       /* Register the periodic data reception callback. */
+       ret = sr_modbus_source_add(sdi->session, modbus, G_IO_IN, 10,
+                       rdtech_dps_receive_data, (void *)sdi);
+       if (ret != SR_OK)
                return ret;
 
        sr_sw_limits_acquisition_start(&devc->limits);
index 65f40ab9297ad681117a06d9f99f2c489bf7c750..d8bd5e82175fe979312df61909966d534a012938 100644 (file)
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2018 James Churchill <pelrun@gmail.com>
  * Copyright (C) 2019 Frank Stettner <frank-stettner@gmx.net>
+ * Copyright (C) 2021 Gerhard Sittig <gerhard.sittig@gmx.net>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  */
 
 #include <config.h>
+
+#include <string.h>
+
 #include "protocol.h"
 
-SR_PRIV int rdtech_dps_read_holding_registers(struct sr_modbus_dev_inst *modbus,
-               int address, int nb_registers, uint16_t *registers)
+enum rdtech_dps_register {
+       REG_USET       = 0x00, /* Mirror of 0x50 */
+       REG_ISET       = 0x01, /* Mirror of 0x51 */
+       REG_UOUT       = 0x02,
+       REG_IOUT       = 0x03,
+       REG_POWER      = 0x04,
+       REG_UIN        = 0x05,
+       REG_LOCK       = 0x06,
+       REG_PROTECT    = 0x07,
+       REG_CV_CC      = 0x08,
+       REG_ENABLE     = 0x09,
+       REG_BACKLIGHT  = 0x0A, /* Mirror of 0x55 */
+       REG_MODEL      = 0x0B,
+       REG_VERSION    = 0x0C,
+
+       REG_PRESET     = 0x23, /* Loads a preset into preset 0. */
+
+       /*
+        * Add (preset * 0x10) to each of the following, for preset 1-9.
+        * Preset 0 regs below are the active output settings.
+        */
+       PRE_USET       = 0x50,
+       PRE_ISET       = 0x51,
+       PRE_OVPSET     = 0x52,
+       PRE_OCPSET     = 0x53,
+       PRE_OPPSET     = 0x54,
+       PRE_BACKLIGHT  = 0x55,
+       PRE_DISABLE    = 0x56, /* Disable output if 0 is copied here from a preset (1 is no change). */
+       PRE_BOOT       = 0x57, /* Enable output at boot if 1. */
+};
+#define REG_PRESET_STRIDE 0x10
+
+enum rdtech_dps_protect_state {
+       STATE_NORMAL = 0,
+       STATE_OVP    = 1,
+       STATE_OCP    = 2,
+       STATE_OPP    = 3,
+};
+
+enum rdtech_dps_regulation_mode {
+       MODE_CV      = 0,
+       MODE_CC      = 1,
+};
+
+/* Retries failed modbus read attempts for improved reliability. */
+static int rdtech_dps_read_holding_registers(struct sr_modbus_dev_inst *modbus,
+       int address, int nb_registers, uint16_t *registers)
 {
-       int i, ret;
+       size_t retries;
+       int ret;
 
-       i = 0;
-       do {
+       retries = 3;
+       while (retries--) {
                ret = sr_modbus_read_holding_registers(modbus,
                        address, nb_registers, registers);
-               ++i;
-       } while (ret != SR_OK && i < 3);
+               if (ret == SR_OK)
+                       return ret;
+       }
 
        return ret;
 }
 
-SR_PRIV int rdtech_dps_get_reg(const struct sr_dev_inst *sdi,
-               uint16_t address, uint16_t *value)
+/* Get one 16bit register. */
+static int rdtech_dps_get_reg(const struct sr_dev_inst *sdi,
+       uint16_t address, uint16_t *value)
 {
        struct dev_context *devc;
        struct sr_modbus_dev_inst *modbus;
        uint16_t registers[1];
        int ret;
+       const uint8_t *rdptr;
 
        devc = sdi->priv;
        modbus = sdi->conn;
 
        g_mutex_lock(&devc->rw_mutex);
-       ret = rdtech_dps_read_holding_registers(modbus, address, 1, registers);
+       ret = rdtech_dps_read_holding_registers(modbus,
+               address, ARRAY_SIZE(registers), registers);
        g_mutex_unlock(&devc->rw_mutex);
-       *value = RB16(registers + 0);
+
+       rdptr = (void *)registers;
+       *value = read_u16le(rdptr);
+
        return ret;
 }
 
-SR_PRIV int rdtech_dps_set_reg(const struct sr_dev_inst *sdi,
-               uint16_t address, uint16_t value)
+/* Set one 16bit register. */
+static int rdtech_dps_set_reg(const struct sr_dev_inst *sdi,
+       uint16_t address, uint16_t value)
 {
        struct dev_context *devc;
        struct sr_modbus_dev_inst *modbus;
        uint16_t registers[1];
        int ret;
+       uint8_t *wrptr;
 
        devc = sdi->priv;
        modbus = sdi->conn;
 
-       WB16(registers, value);
+       wrptr = (void *)registers;
+       write_u16le(wrptr, value);
+
        g_mutex_lock(&devc->rw_mutex);
-       ret = sr_modbus_write_multiple_registers(modbus, address, 1, registers);
+       ret = sr_modbus_write_multiple_registers(modbus, address,
+               ARRAY_SIZE(registers), registers);
        g_mutex_unlock(&devc->rw_mutex);
+
        return ret;
 }
 
+/* Get DPS model number and firmware version from a connected device. */
 SR_PRIV int rdtech_dps_get_model_version(struct sr_modbus_dev_inst *modbus,
-               uint16_t *model, uint16_t *version)
+       uint16_t *model, uint16_t *version)
 {
-       uint16_t registers[2];
+       uint16_t registers[REG_VERSION + 1 - REG_MODEL];
        int ret;
+       const uint8_t *rdptr;
+
+       /* Silence a compiler warning about an unused routine. */
+       (void)rdtech_dps_get_reg;
 
        /*
-        * No mutex here, because there is no sr_dev_inst when this function
-        * is called.
+        * Get the MODEL and VERSION registers. No mutex here, because
+        * there is no sr_dev_inst when this function is called.
         */
-       ret = rdtech_dps_read_holding_registers(modbus, REG_MODEL, 2, registers);
-       if (ret == SR_OK) {
-               *model = RB16(registers + 0);
-               *version = RB16(registers + 1);
-               sr_info("RDTech PSU model: %d version: %d", *model, *version);
-       }
+       ret = rdtech_dps_read_holding_registers(modbus,
+               REG_MODEL, ARRAY_SIZE(registers), registers);
+       if (ret != SR_OK)
+               return ret;
+
+       rdptr = (void *)registers;
+       *model = read_u16le_inc(&rdptr);
+       *version = read_u16le_inc(&rdptr);
+       sr_info("RDTech DPS PSU model: %u version: %u", *model, *version);
+
        return ret;
 }
 
-static void send_value(const struct sr_dev_inst *sdi, struct sr_channel *ch,
-               float value, enum sr_mq mq, enum sr_mqflag mqflags,
-               enum sr_unit unit, int digits)
+/* Send a measured value to the session feed. */
+static int send_value(const struct sr_dev_inst *sdi,
+       struct sr_channel *ch, float value,
+       enum sr_mq mq, enum sr_mqflag mqflags,
+       enum sr_unit unit, int digits)
 {
        struct sr_datafeed_packet packet;
        struct sr_datafeed_analog analog;
        struct sr_analog_encoding encoding;
        struct sr_analog_meaning meaning;
        struct sr_analog_spec spec;
+       int ret;
 
        sr_analog_init(&analog, &encoding, &meaning, &spec, digits);
        analog.meaning->channels = g_slist_append(NULL, ch);
@@ -111,79 +186,274 @@ static void send_value(const struct sr_dev_inst *sdi, struct sr_channel *ch,
 
        packet.type = SR_DF_ANALOG;
        packet.payload = &analog;
-       sr_session_send(sdi, &packet);
+       ret = sr_session_send(sdi, &packet);
+
        g_slist_free(analog.meaning->channels);
+
+       return ret;
+}
+
+/*
+ * Get the device's current state. Exhaustively, relentlessly.
+ * Concentrate all details of communication in the physical transport,
+ * register layout interpretation, and potential model dependency in
+ * this central spot, to simplify maintenance.
+ *
+ * TODO Optionally limit the transfer volume depending on caller's spec
+ * which detail level is desired? Is 10 registers each 16bits an issue
+ * 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 dev_context *devc;
+       struct sr_modbus_dev_inst *modbus;
+       uint16_t registers[REG_ENABLE + 1 - REG_USET];
+       int ret;
+       const uint8_t *rdptr;
+       uint16_t uset_raw, iset_raw, uout_raw, iout_raw, power_raw;
+       uint16_t reg_val, reg_state, out_state, ovpset_raw, ocpset_raw;
+       gboolean is_lock, is_out_enabled, is_reg_cc;
+       gboolean uses_ovp, uses_ocp;
+       float volt_target, curr_limit;
+       float ovp_threshold, ocp_threshold;
+       float curr_voltage, curr_current, curr_power;
+
+       if (!sdi || !sdi->priv || !sdi->conn)
+               return SR_ERR_ARG;
+       devc = sdi->priv;
+       modbus = sdi->conn;
+       if (!state)
+               return SR_ERR_ARG;
+
+       /* Transfer a chunk of registers in a single call. */
+       g_mutex_lock(&devc->rw_mutex);
+       ret = rdtech_dps_read_holding_registers(modbus,
+               REG_USET, ARRAY_SIZE(registers), registers);
+       g_mutex_unlock(&devc->rw_mutex);
+       if (ret != SR_OK)
+               return ret;
+
+       /* Interpret the registers' values. */
+       rdptr = (const void *)registers;
+       uset_raw = read_u16le_inc(&rdptr);
+       volt_target = uset_raw / devc->voltage_multiplier;
+       iset_raw = read_u16le_inc(&rdptr);
+       curr_limit = iset_raw / devc->current_multiplier;
+       uout_raw = read_u16le_inc(&rdptr);
+       curr_voltage = uout_raw / devc->voltage_multiplier;
+       iout_raw = read_u16le_inc(&rdptr);
+       curr_current = iout_raw / devc->current_multiplier;
+       power_raw = read_u16le_inc(&rdptr);
+       curr_power = power_raw / 100.0f;
+       (void)read_u16le_inc(&rdptr); /* UIN */
+       reg_val = read_u16le_inc(&rdptr); /* LOCK */
+       is_lock = reg_val != 0;
+       reg_val = read_u16le_inc(&rdptr); /* PROTECT */
+       uses_ovp = reg_val == STATE_OVP;
+       uses_ocp = reg_val == STATE_OCP;
+       reg_state = read_u16le_inc(&rdptr); /* CV_CC */
+       is_reg_cc = reg_state == MODE_CC;
+       out_state = read_u16le_inc(&rdptr); /* ENABLE */
+       is_out_enabled = out_state != 0;
+
+       /*
+        * Transfer another chunk of registers in a single call.
+        * TODO Unfortunately this call site open codes a fixed number
+        * of registers to read. But there is already some leakage of
+        * the register layout in this routine, and adding more device
+        * types in the future will make things "worse". So accept it.
+        */
+       g_mutex_lock(&devc->rw_mutex);
+       ret = rdtech_dps_read_holding_registers(modbus,
+               PRE_OVPSET, 2, registers);
+       g_mutex_unlock(&devc->rw_mutex);
+       if (ret != SR_OK)
+               return ret;
+
+       /* Interpret the registers' values. */
+       rdptr = (const void *)registers;
+       ovpset_raw = read_u16le_inc(&rdptr); /* PRE OVPSET */
+       ovp_threshold = ovpset_raw * devc->voltage_multiplier;
+       ocpset_raw = read_u16le_inc(&rdptr); /* PRE OCPSET */
+       ocp_threshold = ocpset_raw * devc->current_multiplier;
+
+       /* Store gathered details in the high level container. */
+       memset(state, 0, sizeof(*state));
+       state->lock = is_lock;
+       state->mask |= STATE_LOCK;
+       state->output_enabled = is_out_enabled;
+       state->mask |= STATE_OUTPUT_ENABLED;
+       state->regulation_cc = is_reg_cc;
+       state->mask |= STATE_REGULATION_CC;
+       state->protect_ovp = uses_ovp;
+       state->mask |= STATE_PROTECT_OVP;
+       state->protect_ocp = uses_ocp;
+       state->mask |= STATE_PROTECT_OCP;
+       state->protect_enabled = TRUE;
+       state->mask |= STATE_PROTECT_ENABLED;
+       state->voltage_target = volt_target;
+       state->mask |= STATE_VOLTAGE_TARGET;
+       state->current_limit = curr_limit;
+       state->mask |= STATE_CURRENT_LIMIT;
+       state->ovp_threshold = ovp_threshold;
+       state->mask |= STATE_OVP_THRESHOLD;
+       state->ocp_threshold = ocp_threshold;
+       state->mask |= STATE_OCP_THRESHOLD;
+       state->voltage = curr_voltage;
+       state->mask |= STATE_VOLTAGE;
+       state->current = curr_current;
+       state->mask |= STATE_CURRENT;
+       state->power = curr_power;
+       state->mask |= STATE_POWER;
+
+       return SR_OK;
+}
+
+/* Setup device's parameters. Selectively, from caller specs. */
+SR_PRIV int rdtech_dps_set_state(const struct sr_dev_inst *sdi,
+       struct rdtech_dps_state *state)
+{
+       struct dev_context *devc;
+       uint16_t reg_value;
+       int ret;
+
+       if (!sdi || !sdi->priv || !sdi->conn)
+               return SR_ERR_ARG;
+       devc = sdi->priv;
+       if (!state)
+               return SR_ERR_ARG;
+
+       /* Only a subset of known values is settable. */
+       if (state->mask & STATE_OUTPUT_ENABLED) {
+               reg_value = state->output_enabled ? 1 : 0;
+               ret = rdtech_dps_set_reg(sdi, REG_ENABLE, reg_value);
+               if (ret != SR_OK)
+                       return ret;
+       }
+       if (state->mask & STATE_VOLTAGE_TARGET) {
+               reg_value = state->voltage_target * devc->voltage_multiplier;
+               ret = rdtech_dps_set_reg(sdi, REG_USET, reg_value);
+               if (ret != SR_OK)
+                       return ret;
+       }
+       if (state->mask & STATE_CURRENT_LIMIT) {
+               reg_value = state->current_limit * devc->current_multiplier;
+               ret = rdtech_dps_set_reg(sdi, REG_ISET, reg_value);
+               if (ret != SR_OK)
+                       return ret;
+       }
+       if (state->mask & STATE_OVP_THRESHOLD) {
+               reg_value = state->ovp_threshold * devc->voltage_multiplier;
+               ret = rdtech_dps_set_reg(sdi, PRE_OVPSET, reg_value);
+               if (ret != SR_OK)
+                       return ret;
+       }
+       if (state->mask & STATE_OCP_THRESHOLD) {
+               reg_value = state->ocp_threshold * devc->current_multiplier;
+               ret = rdtech_dps_set_reg(sdi, PRE_OCPSET, reg_value);
+               if (ret != SR_OK)
+                       return ret;
+       }
+       if (state->mask & STATE_LOCK) {
+               reg_value = state->lock ? 1 : 0;
+               ret = rdtech_dps_set_reg(sdi, REG_LOCK, reg_value);
+               if (ret != SR_OK)
+                       return ret;
+       }
+
+       return SR_OK;
 }
 
+/* Get the current state when acquisition starts. */
+SR_PRIV int rdtech_dps_seed_receive(const struct sr_dev_inst *sdi)
+{
+       struct dev_context *devc;
+       struct rdtech_dps_state state;
+       int ret;
+
+       ret = rdtech_dps_get_state(sdi, &state);
+       if (ret != SR_OK)
+               return ret;
+
+       if (state.mask & STATE_PROTECT_OVP)
+               devc->curr_ovp_state = state.protect_ovp;
+       if (state.mask & STATE_PROTECT_OCP)
+               devc->curr_ocp_state = state.protect_ocp;
+       if (state.mask & STATE_REGULATION_CC)
+               devc->curr_cc_state = state.regulation_cc;
+       if (state.mask & STATE_OUTPUT_ENABLED)
+               devc->curr_out_state = state.output_enabled;
+
+       return SR_OK;
+}
+
+/* Get measurements, track state changes during acquisition. */
 SR_PRIV int rdtech_dps_receive_data(int fd, int revents, void *cb_data)
 {
        struct sr_dev_inst *sdi;
        struct dev_context *devc;
-       struct sr_modbus_dev_inst *modbus;
-       uint16_t registers[8];
+       struct rdtech_dps_state state;
        int ret;
+       struct sr_channel *ch;
+       const char *regulation_text;
 
        (void)fd;
        (void)revents;
 
-       if (!(sdi = cb_data))
+       sdi = cb_data;
+       if (!sdi)
                return TRUE;
-
-       modbus = sdi->conn;
        devc = sdi->priv;
 
-       g_mutex_lock(&devc->rw_mutex);
-       /*
-        * Using the libsigrok function here, because it doesn't matter if the
-        * reading fails. It will be done again in the next acquision cycle anyways.
-        */
-       ret = sr_modbus_read_holding_registers(modbus, REG_UOUT, 8, registers);
-       g_mutex_unlock(&devc->rw_mutex);
+       /* Get the device's current state. */
+       ret = rdtech_dps_get_state(sdi, &state);
+       if (ret != SR_OK)
+               return ret;
 
-       if (ret == SR_OK) {
-               /* Send channel values */
-               std_session_send_df_frame_begin(sdi);
-
-               send_value(sdi, sdi->channels->data,
-                       RB16(registers + 0) / devc->voltage_multiplier,
-                       SR_MQ_VOLTAGE, SR_MQFLAG_DC, SR_UNIT_VOLT,
-                       devc->model->voltage_digits);
-               send_value(sdi, sdi->channels->next->data,
-                       RB16(registers + 1) / devc->current_multiplier,
-                       SR_MQ_CURRENT, SR_MQFLAG_DC, SR_UNIT_AMPERE,
-                       devc->model->current_digits);
-               send_value(sdi, sdi->channels->next->next->data,
-                       RB16(registers + 2) / 100.0f,
-                       SR_MQ_POWER, 0, SR_UNIT_WATT, 2);
-
-               std_session_send_df_frame_end(sdi);
-
-               /* Check for state changes */
-               if (devc->actual_ovp_state != (RB16(registers + 5) == STATE_OVP)) {
-                       devc->actual_ovp_state = RB16(registers + 5) == STATE_OVP;
-                       sr_session_send_meta(sdi, SR_CONF_OVER_VOLTAGE_PROTECTION_ACTIVE,
-                               g_variant_new_boolean(devc->actual_ovp_state));
-               }
-               if (devc->actual_ocp_state != (RB16(registers + 5) == STATE_OCP)) {
-                       devc->actual_ocp_state = RB16(registers + 5) == STATE_OCP;
-                       sr_session_send_meta(sdi, SR_CONF_OVER_CURRENT_PROTECTION_ACTIVE,
-                               g_variant_new_boolean(devc->actual_ocp_state));
-               }
-               if (devc->actual_regulation_state != RB16(registers + 6)) {
-                       devc->actual_regulation_state = RB16(registers + 6);
-                       sr_session_send_meta(sdi, SR_CONF_REGULATION,
-                               g_variant_new_string(
-                                       devc->actual_regulation_state == MODE_CC ? "CC" : "CV"));
-               }
-               if (devc->actual_output_state != RB16(registers + 7)) {
-                       devc->actual_output_state = RB16(registers + 7);
-                       sr_session_send_meta(sdi, SR_CONF_ENABLED,
-                               g_variant_new_boolean(devc->actual_output_state));
-               }
-
-               sr_sw_limits_update_samples_read(&devc->limits, 1);
+
+       /* Submit measurement data to the session feed. */
+       std_session_send_df_frame_begin(sdi);
+       ch = g_slist_nth_data(sdi->channels, 0);
+       send_value(sdi, ch, state.voltage,
+               SR_MQ_VOLTAGE, SR_MQFLAG_DC, SR_UNIT_VOLT,
+               devc->model->voltage_digits);
+       ch = g_slist_nth_data(sdi->channels, 1);
+       send_value(sdi, ch, state.current,
+               SR_MQ_CURRENT, SR_MQFLAG_DC, SR_UNIT_AMPERE,
+               devc->model->current_digits);
+       ch = g_slist_nth_data(sdi->channels, 2);
+       send_value(sdi, ch, state.power,
+               SR_MQ_POWER, 0, SR_UNIT_WATT, 2);
+       std_session_send_df_frame_end(sdi);
+
+       /* Check for state changes. */
+       if (devc->curr_ovp_state != state.protect_ovp) {
+               (void)sr_session_send_meta(sdi,
+                       SR_CONF_OVER_VOLTAGE_PROTECTION_ACTIVE,
+                       g_variant_new_boolean(state.protect_ovp));
+               devc->curr_ovp_state = state.protect_ovp;
+       }
+       if (devc->curr_ocp_state != state.protect_ocp) {
+               (void)sr_session_send_meta(sdi,
+                       SR_CONF_OVER_CURRENT_PROTECTION_ACTIVE,
+                       g_variant_new_boolean(state.protect_ocp));
+               devc->curr_ocp_state = state.protect_ocp;
+       }
+       if (devc->curr_cc_state != state.regulation_cc) {
+               regulation_text = state.regulation_cc ? "CC" : "CV";
+               (void)sr_session_send_meta(sdi, SR_CONF_REGULATION,
+                       g_variant_new_string(regulation_text));
+               devc->curr_cc_state = state.regulation_cc;
+       }
+       if (devc->curr_out_state != state.output_enabled) {
+               (void)sr_session_send_meta(sdi, SR_CONF_ENABLED,
+                       g_variant_new_boolean(state.output_enabled));
+               devc->curr_out_state = state.output_enabled;
        }
 
+       /* Check optional acquisition limits. */
+       sr_sw_limits_update_samples_read(&devc->limits, 1);
        if (sr_sw_limits_check(&devc->limits)) {
                sr_dev_acquisition_stop(sdi);
                return TRUE;
index d6619e5301321d109b68e3daa5156697a134e3d4..fc23ed3ab1c087e2fdbe86484e330c4728386dc4 100644 (file)
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2018 James Churchill <pelrun@gmail.com>
  * Copyright (C) 2019 Frank Stettner <frank-stettner@gmx.net>
+ * Copyright (C) 2021 Gerhard Sittig <gerhard.sittig@gmx.net>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
 #ifndef LIBSIGROK_HARDWARE_RDTECH_DPS_PROTOCOL_H
 #define LIBSIGROK_HARDWARE_RDTECH_DPS_PROTOCOL_H
 
-#include <stdint.h>
+#include <config.h>
+
 #include <glib.h>
 #include <libsigrok/libsigrok.h>
+#include <stdint.h>
+
 #include "libsigrok-internal.h"
 
 #define LOG_PREFIX "rdtech-dps"
@@ -40,68 +44,49 @@ struct rdtech_dps_model {
 
 struct dev_context {
        const struct rdtech_dps_model *model;
-       struct sr_sw_limits limits;
-       GMutex rw_mutex;
        double current_multiplier;
        double voltage_multiplier;
-       gboolean actual_ovp_state;
-       gboolean actual_ocp_state;
-       uint16_t actual_regulation_state;
-       uint16_t actual_output_state;
-};
-
-enum rdtech_dps_register {
-       REG_USET       = 0x00, /* Mirror of 0x50 */
-       REG_ISET       = 0x01, /* Mirror of 0x51 */
-       REG_UOUT       = 0x02,
-       REG_IOUT       = 0x03,
-       REG_POWER      = 0x04,
-       REG_UIN        = 0x05,
-       REG_LOCK       = 0x06,
-       REG_PROTECT    = 0x07,
-       REG_CV_CC      = 0x08,
-       REG_ENABLE     = 0x09,
-       REG_BACKLIGHT  = 0x0A, /* Mirror of 0x55 */
-       REG_MODEL      = 0x0B,
-       REG_VERSION    = 0x0C,
-
-       REG_PRESET     = 0x23, /* Loads a preset into preset 0. */
-
-/*
- * Add (preset * 0x10) to each of the following, for preset 1-9.
- * Preset 0 regs below are the active output settings.
- */
-       PRE_USET       = 0x50,
-       PRE_ISET       = 0x51,
-       PRE_OVPSET     = 0x52,
-       PRE_OCPSET     = 0x53,
-       PRE_OPPSET     = 0x54,
-       PRE_BACKLIGHT  = 0x55,
-       PRE_DISABLE    = 0x56, /* Disable output if 0 is copied here from a preset (1 is no change). */
-       PRE_BOOT       = 0x57, /* Enable output at boot if 1. */
-};
-
-enum rdtech_dps_state {
-       STATE_NORMAL = 0,
-       STATE_OVP    = 1,
-       STATE_OCP    = 2,
-       STATE_OPP    = 3,
+       struct sr_sw_limits limits;
+       GMutex rw_mutex;
+       gboolean curr_ovp_state;
+       gboolean curr_ocp_state;
+       gboolean curr_cc_state;
+       gboolean curr_out_state;
 };
 
-enum rdtech_dps_mode {
-       MODE_CV      = 0,
-       MODE_CC      = 1,
+/* Container to get and set parameter values. */
+struct rdtech_dps_state {
+       enum rdtech_dps_state_mask {
+               STATE_LOCK = 1 << 0,
+               STATE_OUTPUT_ENABLED = 1 << 1,
+               STATE_REGULATION_CC = 1 << 2,
+               STATE_PROTECT_OVP = 1 << 3,
+               STATE_PROTECT_OCP = 1 << 4,
+               STATE_PROTECT_ENABLED = 1 << 5,
+               STATE_VOLTAGE_TARGET = 1 << 6,
+               STATE_CURRENT_LIMIT = 1 << 7,
+               STATE_OVP_THRESHOLD = 1 << 8,
+               STATE_OCP_THRESHOLD = 1 << 9,
+               STATE_VOLTAGE = 1 << 10,
+               STATE_CURRENT = 1 << 11,
+               STATE_POWER = 1 << 12,
+       } mask;
+       gboolean lock;
+       gboolean output_enabled, regulation_cc;
+       gboolean protect_ovp, protect_ocp, protect_enabled;
+       float voltage_target, current_limit;
+       float ovp_threshold, ocp_threshold;
+       float voltage, current, power;
 };
 
-SR_PRIV int rdtech_dps_read_holding_registers(struct sr_modbus_dev_inst *modbus,
-               int address, int nb_registers, uint16_t *registers);
-
-SR_PRIV int rdtech_dps_get_reg(const struct sr_dev_inst *sdi, uint16_t address, uint16_t *value);
-SR_PRIV int rdtech_dps_set_reg(const struct sr_dev_inst *sdi, uint16_t address, uint16_t value);
+SR_PRIV int rdtech_dps_get_state(const struct sr_dev_inst *sdi,
+       struct rdtech_dps_state *state);
+SR_PRIV int rdtech_dps_set_state(const struct sr_dev_inst *sdi,
+       struct rdtech_dps_state *state);
 
 SR_PRIV int rdtech_dps_get_model_version(struct sr_modbus_dev_inst *modbus,
-               uint16_t *model, uint16_t *version);
-
+       uint16_t *model, uint16_t *version);
+SR_PRIV int rdtech_dps_seed_receive(const struct sr_dev_inst *sdi);
 SR_PRIV int rdtech_dps_receive_data(int fd, int revents, void *cb_data);
 
 #endif