Add support for RD6006P and RD6012P by mdjurfeldt · Pull Request #205 · sigrokproject/libsigrok

mdjurfeldt · 2023-01-16T22:57:33Z

This PR adds basic support for the models RD6006P and RD6012P. In order to get the current readings correct for RD6012P, I had to dynamically adapt the current_multiplier to which current range (6V/12V) is set for the PSU. In the future, one could also add the possibility to switch voltage range to the GUI.

This has been tested on an RD6012P but should work on all the other models.

fenugrec

Also I think the commit message's first line should be changed to something like "rdtech-dps: " - see other commits in the repo.

Disclaimer : I'm neither maintainer nor dev for this project.

src/hardware/rdtech-dps/protocol.c

fenugrec · 2023-01-22T15:28:23Z

src/hardware/rdtech-dps/protocol.c

I'd like a note that says whether these numeric values are arbitrary or part of the protocol. In contrast to the 'state mask' enum that appears to be arbitrary and only for maintaining local state ?

They are not arbitrary. These are the device family specific modbus registers. I think that you have a point that this should be stated in a comment. I'll do that.

fenugrec · 2023-01-22T15:31:54Z

src/hardware/rdtech-dps/protocol.c

I think magic number "60125" deserves a macro name....

Well, in the general case I of course agree that there should not be magic numbers in the code. The reason why I did it this way in this case is that 60125 is not magic but is the most specific name of the model. Thus, a suitable macro name could be MODEL_NUMBER_60125 which is kind of pointless.

What would you say about defining a macro (in protocol.c) containing 60125 but used in this location as:

if (IS_MODEL_6012P (devc->model))

?

not sure TBH, if it was my own code I would probably have an enum for known model numbers. Of course that would fall apart if it becomes necessary to parse model# as a string. At a glance "60125 means 6012P" was not obvious.
Another way of approaching this is to expand supported_models struct with bool flags for special features and then in the code, instead of comparing model# values like here, you check if a flag is set in the model descriptor.
Not recommending one or the other here, just discussing.

Right. I guess you have seen that the mapping between 60125 (which is set by the manufacturer; 60125 is the real model number) and "RD6012P" exists in the supported_models table? I agree that if it turns out that other models than RD6012P have this feature, then it would be a good solution to extend that table. I propose that we keep this solution until then.

mdjurfeldt · 2023-01-24T19:04:05Z

@fenugrec Thank you for your review.

I have responded to your questions and have pushed new changes that address the issues you raised. I had to force push to modify the commit messages to the rdtech-dps: style.

mdjurfeldt · 2023-01-25T16:24:28Z

Hi @fenugrec, are you OK with the current state of this PR? Meanwhile, I've learnt that the preferred way to report the review is not github but the mailinglist [email protected]. Can I ask you to send an email to that list saying that you have reviewed this change?

fenugrec · 2023-01-25T17:42:19Z

Can I ask you to send an email to that list saying that you have reviewed this change?

No time for that unfortunately. You can post yourself, AFAIK registering should be quick

Sidenote, now you will need much patience - sigrok is beyond understaffed.

mdjurfeldt · 2023-01-25T19:29:42Z

OK, done! Many thanks for your review. I'll keep your suggestion to use the table for feature flags in mind for the future.

knarfS · 2023-02-11T16:18:22Z

src/hardware/rdtech-dps/protocol.c

 {
 	size_t retries;
-	int ret;
+	int ret = SR_ERR; /* make the compiler happy */


Only variable declaration here

src/hardware/rdtech-dps/protocol.c

mdjurfeldt · 2023-02-17T00:34:09Z

@knarfS @fenugrec: Hi, I've enabled listing, setting and getting range now. I also hope that everything is sync:ed with the device now and that range meta packages are sent if range is changed.

mdjurfeldt · 2023-02-17T10:48:15Z

@knarfS @fenugrec: Maybe I have been overambitious regarding the updating of range and multipliers? I have operated under the assumption that range can change by manipulating the device manually during sigrok control. However, when sigrok opens the device it actually locks it. So, maybe we can make some assumptions regarding when range can change (and, thus, simplify the code and avoid some communication)?

knarfS · 2023-02-17T18:48:52Z

src/hardware/rdtech-dps/protocol.c

+	if (devc->curr_range != state.range) {
+		(void)sr_session_send_meta(sdi,
+			SR_CONF_RANGE,
+			g_variant_new_uint32(state.range));


Send the range string here

Ah... me stupid.

knarfS · 2023-02-17T19:07:01Z

src/hardware/rdtech-dps/api.c

 	SR_CONF_OVER_VOLTAGE_PROTECTION_THRESHOLD | SR_CONF_GET | SR_CONF_SET,
 	SR_CONF_OVER_CURRENT_PROTECTION_ACTIVE | SR_CONF_GET,
 	SR_CONF_OVER_CURRENT_PROTECTION_THRESHOLD | SR_CONF_GET | SR_CONF_SET,
+	SR_CONF_RANGE | SR_CONF_GET | SR_CONF_SET | SR_CONF_LIST,


I'd create a new devopts array, e.g. devopts_rd6012p or devopts_w_range only for this model (and other models with multiple ranges in the future), because the other existing models don't have this feature and therefore shouldn't expose it via the api.

But maybe that's something to discuss on IRC, to get an opinion of other devs.

knarfS · 2023-02-17T19:16:35Z

src/hardware/rdtech-dps/api.c

+			if (g_strcmp0(devc->model->ranges[i].range_str,
+				      range_str)
+			    == 0)
+			{


Style nit:

range = &devc->model->ranges[i]; if (g_strcmp0(range->range_str, range_str) == 0) {

knarfS · 2023-02-17T20:13:14Z

src/hardware/rdtech-dps/api.c

+		g_variant_builder_init(&gvb, G_VARIANT_TYPE_ARRAY);
+		for (i = 0; i < devc->model->n_ranges; ++i)
+			g_variant_builder_add(&gvb, "s",
+					      devc->model->ranges[i].range_str);


Style nit: Only indent one tab (4 tabs total)

knarfS · 2023-02-17T20:24:19Z

src/hardware/rdtech-dps/protocol.c

+/* These are the Modbus RTU registers for the family of rdtech-dps devices.
+ *
+ * Some registers are specific to a certain device. For example,
+ * REG_RD_RANGE is specific to RD6012P.


Move this part of the comment down to the rdtech_rd_register enum

knarfS · 2023-02-17T20:28:22Z

src/hardware/rdtech-dps/protocol.c

+	devc->voltage_multiplier = pow(10.0, range->voltage_digits);
+}
+
+/* Determine range of connected device. Don't do anything once


Style nit:

/* * Determine range of connected device. Don't do anything once * acquisition has started (since the range will then be tracked). */

knarfS · 2023-02-17T20:28:57Z

src/hardware/rdtech-dps/protocol.c

+	int ret;
+
+	devc = sdi->priv;
+	/* Only update range if there are multiple ranges and data


See style nit above

knarfS · 2023-02-17T20:57:37Z

src/hardware/rdtech-dps/protocol.c

 		ret = rdtech_dps_read_holding_registers(modbus,
-			REG_RD_VOLT_TGT, 11, registers);
+			REG_RD_VOLT_TGT,
+			devc->model->n_ranges > 1 ? 13 : 11,


Define constants in protocol.h for the various register lengths (also for the DPS models), that would be much easier to read (no magic numbers).

OK, I'll do this in protocol.c since the registers are listed there.

knarfS · 2023-02-17T21:10:09Z

src/hardware/rdtech-dps/protocol.c

+				devc->curr_range = reg_value ? 1 : 0;
+				rdtech_dps_update_multipliers(sdi);
+			}
+			/* We rely on the data acquisition to update


See style nit for comments above

knarfS · 2023-02-17T21:23:27Z

src/hardware/rdtech-dps/protocol.h

-		STATE_VOLTAGE = 1 << 10,
-		STATE_CURRENT = 1 << 11,
-		STATE_POWER = 1 << 12,
+		STATE_RANGE = 1 << 5,


Better append the new status at the end.

mdjurfeldt · 2023-02-17T23:13:32Z

@knarfS Hi, I believe that I have adressed all of the above now.

RD6012P has two current ranges, so support for getting/setting/listing range is added. This also means that the model table is reorganized to support multiple ranges. In order for the range setting and the current multiplier to be up to date, the function rdtech_dps_update_range is called when required. This function reads the range register of the device.

abraxa · 2023-09-27T21:18:56Z

Merged as https://sigrok.org/gitaction/libsigrok.git/48385ca..02a4f48 , thank you!

Also lots of thanks for @fenugrec and @knarfS for reviewing, made my life much easier!

fenugrec reviewed Jan 22, 2023

View reviewed changes

mdjurfeldt force-pushed the add-RD60nnP-support branch from 0f308d7 to a4bd8d0 Compare January 24, 2023 18:31

knarfS suggested changes Feb 11, 2023

View reviewed changes

knarfS suggested changes Feb 17, 2023

View reviewed changes

mdjurfeldt force-pushed the add-RD60nnP-support branch from f07fee4 to 8831cb6 Compare March 1, 2023 19:25

knarfS approved these changes Mar 1, 2023

View reviewed changes

abraxa closed this Sep 27, 2023

DidierA mentioned this pull request Feb 17, 2026

rdtech-rd: Fix <inf> samples for RD6006 #283

Open

Conversation

mdjurfeldt commented Jan 16, 2023

Uh oh!

fenugrec left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fenugrec Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjurfeldt commented Jan 24, 2023

Uh oh!

mdjurfeldt commented Jan 25, 2023

Uh oh!

fenugrec commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdjurfeldt commented Jan 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdjurfeldt commented Feb 17, 2023

Uh oh!

mdjurfeldt commented Feb 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjurfeldt commented Feb 17, 2023

Uh oh!

abraxa commented Sep 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fenugrec left a comment •

edited

Loading

fenugrec Jan 24, 2023 •

edited

Loading

fenugrec commented Jan 25, 2023 •

edited

Loading