libsigrok, channel naming

[rgrr]

Hi pico-coder,

because you have not enabled "Issues" in your libsigrok fork, I'm writing here.

The offset "2" in channel naming creates a dependency between libsigrok and sigrok-pico which does not necessarily must use D2 in other usage scenarios.

So how about adding a small feature for channel naming?

Either the probe could provide the actual names as string or as a minimum solution provide the offset for the naming, so that e.g. D10... would become possible.

[pico-coder]

That naming convention was picked so that the pin naming and pulseview naming were the same and also because it made the programming of the DMA engine and PIO much more sane. Specifically you have to enable channels starting with D2 and work your way up. Trying to mask/shift out disabled channels was too painful.
So, having just D10 might be an easy decode problem in terms of passing strings around through sigrok, but would make the PICO coding a bit of a mess to ignore other pins. (Well, ok, maybe just one channel would be easy, but arbitrary channel enables would be hard/impossible to efficiently mux out).
IIRC, you can also create new math signals and/or give channels special names in pulseview.
But I'm not sure I know exactly what you are after here...

[rgrr]

Sorry that I am unclear.
You introduced in libsigrok the offset "2" to get as you said correct channel naming. I'm talking about this line of code in api.c

channel_name = g_strdup_printf("D%d", i + 2);

You are assuming there in libsigrok something about the probe implementation.

Because I like your sigrok protocol and your probe implementation I want to do my own one. But GP2 is already in use. First spare channel for sigrok usage could be GP10.

So my idea is as follows: if the probe tells sigrok it's signal naming, your libsigrok/raspberry-pico/api.c and the probe implementation could be independent.

Absolutely simplest approach would be to add an GP offset to the probes output of the "i" command, so that your probe would respond with "SRPICO,A031D210202" and mine with "SRPICO,A031D080210" (last two digits are the GP offset).

And in the code line above mentioned, the "2" offset would be replaced by the offset from the "SRPICO" response.

If there is no offset in the "SRPICO" response, the offset would default to "2" to have downward compatibility.

Should I provide a PR to make things clear?

PS: unfortunately I cannot push :-/

[pico-coder]

That makes sense. Since I'm trying to get a pull request into the main repo I have to be a bit careful about changes, but let me a have a look. ...

[rgrr]

Because I cannot push, here is the patch

diff --git a/src/hardware/raspberrypi-pico/api.c b/src/hardware/raspberrypi-pico/api.c
index e6b03648..19305626 100644
--- a/src/hardware/raspberrypi-pico/api.c
+++ b/src/hardware/raspberrypi-pico/api.c
@@ -136,7 +136,8 @@ static GSList *scan(struct sr_dev_driver *di, GSList * options)
 	const char *conn, *serialcomm;
 	char buf[32];
 	int len;
-	uint8_t num_a, num_d, a_size;
+	uint8_t version;
+	uint8_t num_a, num_d, a_size, offset_a;
 	gchar *channel_name;
 
 	conn = serialcomm = NULL;
@@ -197,22 +198,35 @@ static GSList *scan(struct sr_dev_driver *di, GSList * options)
 			return NULL;
 		}
 	}
-	//Expected ID response is SRPICO,AxxyDzz,VV 
+	//Expected ID response is SRPICO,AxxyDzz,VV[,oo]
 	//where xx are number of analog channels, y is bytes per analog sample
-	//and zz is number of digital channels, and VV is two digit version# which must be 02
+	//and zz is number of digital channels, and VV is two digit version# which must be 02 or 03
+	//oo is the offset for channel naming and defaults to 2 (protocol 03)
 	if ((num_read < 16)
 	    || (strncmp(buf, "SRPICO,A", 8))
-	    || (buf[11] != 'D')
-	    || (buf[15] != '0')
-	    || (buf[16] != '2')) {
+	    || (buf[11] != 'D')) {
 		sr_err("ERROR:Bad response string %s %d", buf, num_read);
 		return NULL;
 	}
 	a_size = buf[10] - '0';
-	buf[10] = '\0';		//Null to end the str for atois
-	buf[14] = '\0';		//Null to end the str for atois
+	buf[10] = '\0';		//Null to end the str for atoi
+	buf[14] = '\0';		//Null to end the str for atoi
+	buf[17] = '\0';     //Null to end the str for atoi
 	num_a = atoi(&buf[8]);
 	num_d = atoi(&buf[12]);
+	version = atoi(&buf[15]);
+	if (version != 2  &&  version != 3) {
+        sr_err("ERROR:Bad version in response string %s %d", buf, num_read);
+        return NULL;
+	}
+
+    offset_a = 2;
+	if (version >= 3) {
+	    num_read = serial_read_blocking(serial, buf, 3, 10);
+	    if (num_read == 3) {
+	        offset_a = atoi(&buf[1]);
+	    }
+	}
 
 	sdi = g_malloc0(sizeof(struct sr_dev_inst));
 	sdi->status = SR_ST_INACTIVE;
@@ -282,7 +296,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList * options)
 	if (devc->num_d_channels > 0) {
 		for (i = 0; i < devc->num_d_channels; i++) {
 			//Name digital channels starting at D2 to match pico board pin names
-			channel_name = g_strdup_printf("D%d", i + 2);
+			channel_name = g_strdup_printf("D%d", i + offset_a);
 			sr_channel_new(sdi, i, SR_CHANNEL_LOGIC, TRUE,
 				       channel_name);
 			g_free(channel_name);

I had to introduce a new protocol version, because the strings have no end of line delimiter (or something else). But perhaps it's also ok to wait for a 10ms timeout and leave the protocol version.

[rgrr]

Hmmm... not sure if the above is really a good solution. I think it is smarter to issue extra commands.

Problem with the code above:

• old version of libsigrok, new probe version
• libsigrok sends "i\n"
• probe responds with "SRPICO,AxxyDzz,03,oo" (without linefeed!)
• libsigrok is confused because on the next USB read it gets ",oo" (or it will deny connection because of ill version)

My (new) proposal: use extra commands to get the channel names, so old libsigrok versions will default to "D2"...

New command:

• analog channel names: "NA\n" -> probe returns "\n"
• digital channel names: "ND\n" -> probe returns "\n"

I will make a test implementation of this.

This solution will prevent you from changing your pull request @libsigrok and the actual changes can be submitted later on.

[pico-coder]

Yeah, so the sigrok driver sends "NAxx\n" or "NDxx\n" where XX is a two character channel number, and then the pico returns a string with the pin name.
And yes, it's much easier to release uf2 files so we can release a new uf2 that is compatible with old or new sigrok/pulseview builds since the "N" will be optional.

The pin name should only have printable ascii characters, but we can probably just have comments to that effect in the device code, rather than forcing it in the sigrok code.
I don't think the sigrok code has any limitations on the length of the name string, but you might want to enforce a limit of 8 characters or so...
Thanks for giving this a shot. It was one of those things I thought would be nice, but was waiting for someone to care.... :)

[rgrr]

I have put the changes into a PR for your libsigrok fork.

[pico-coder]

I don't think what is proposed here is exactly what is implemented, and sigrokproject/libsigrok#256 still needs to be accepted in mainline for renaming to work.
But, I think once that is accepted the PICO device should be able to communicate arbitrary pin names, or the user can rename the pins in pulseview, which I think enables most use cases. Closing this. If I missed something that isn't address with "256" open a new issue.