Channel enable failed only after renaming a channel

[MarcelRobitaille]

I compiled sigrok and pulseview based on your fork. Everything is working great, except that I've noticed that I get crashes after renaming channels. For example, I open pulseview with this command:

pulseview --driver raspberrypi-pico:conn=/dev/serial/by-id/usb-Raspberry_Pi_Pico_E66038B7137F2B38-if00:serialcomm=115200/flow=0  --loglevel 5

When the software opens, I can press "Run" and record data.

If I then change the name of the first channel to "MOSI" and press "Run" again, I get a dialog box showing an error message. When I click ok, the program crashes.

Here are the relevant logs with --loglevel 5:

Acquisition took 0.42 s
sr: [02:38.984483] srpico: start config_list with key 753E
sr: [02:38.984527] hwdriver: sr_config_list(): key 30014 (triggermatch) sdi 0x561b15c8bdd0 cg NULL -> [1, 2, 3, 4, 5]
sr: [02:44.370878] srpico: at config_get key 30000
sr: [02:44.370910] srpico: sample rate get of 5000
sr: [02:44.370920] hwdriver: sr_config_get(): key 30000 (samplerate) sdi 0x561b15c8bdd0 cg NULL -> uint64 5000
sr: [02:44.370947] session: Creating our own main context.
sr: [02:44.370969] session: Starting.
sr: [02:44.370977] hwdriver: raspberrypi-pico: Starting acquisition.
sr: [02:44.370985] srpico: Enter acq start
sr: [02:44.370991] srpico: dsbstart 3
sr: [02:44.370998] serial: Draining serial port /dev/serial/by-id/usb-Raspberry_Pi_Pico_E66038B7137F2B38-if00.
sr: [02:44.371028] serial: Wrote 1/1 bytes.
sr: [02:44.371036] serial: Draining serial port /dev/serial/by-id/usb-Raspberry_Pi_Pico_E66038B7137F2B38-if00.
sr: [02:44.371134] srpico: c 0 enabled 0 name A0
sr: [02:44.371162] srpico: Channel enable masks D 0x1FF A 0x0
sr: [02:44.381285] serial: Wrote 4/4 bytes.
sr: [02:44.382404] serial: Read 1/1 bytes.
sr: [02:44.382451] srpico: c 1 enabled 0 name A1
sr: [02:44.382468] srpico: Channel enable masks D 0x1FF A 0x0
sr: [02:44.392631] serial: Wrote 4/4 bytes.
sr: [02:44.393375] serial: Read 1/1 bytes.
sr: [02:44.393412] srpico: c 2 enabled 0 name A2
sr: [02:44.393463] srpico: Channel enable masks D 0x1FF A 0x0
sr: [02:44.403578] serial: Wrote 4/4 bytes.
sr: [02:44.404497] serial: Read 1/1 bytes.
sr: [02:44.404520] srpico: c 0 enabled 1 name MOSI
sr: [02:44.404528] srpico: Channel enable masks D 0x1FF A 0x0
sr: [02:44.414647] serial: Wrote 4/4 bytes.
sr: [02:45.415507] srpico: ERROR:Serial_w_ack M10 failed (0).
sr: [02:45.415551] srpico: ERROR:Channel enable fail
sr: [02:45.415563] session: Could not start raspberrypi-pico device /dev/serial/by-id/usb-Raspberry_Pi_Pico_E66038B7137F2B38-if00 acquisition.
sr: [02:45.415581] hwdriver: raspberrypi-pico: Stopping acquisition.
sr: [02:45.415592] srpico: ****at dev_acquisition_stop
sr: [02:45.415601] session: bus: Received SR_DF_END packet.
sr: [02:45.515777] session: Cannot remove non-existing event source 0x561b15aa72e0.

Please let me know if I should report this upstream instead

[LouDnl]

Same error here, I compiled a fresh nightly from Sigrok git including the libs.
When I open pulseview and connect to the Pico I can run a trace without issues.
Disabling channels does not throw any errors either.
As soon as I rename a single channel an error is thrown.

sr: srpico: ERROR: Serial_w_ack a10 failed (1).
sr: srpico: ack resp char 2 d 50
sr: srpico: ERROR: Channel enable fail
sr: session: Could not start raspberrypi-pico device /dev/ttyACM0 acquisition.
sr: srpico: Dropping 6 device bytes
sr: session: Cannot remove non-existing event source 0x5a9c73966f50.
Notifying user of session error:  "Capture failed" ;  "generic/unspecified error"

[pico-coder]

So as kind of a workaround I think it works to just leave the "D" at the start of the signal name.
i.e. "D2" becomes "D2_MOSI". But long term better naming of the signals between pulseview and the device are in order...

[mpictor]

First off, thanks for all your effort on pico support in sigrok!

I think the naming problem comes from this line?
https://github.com/sigrokproject/libsigrok/blob/master/src/hardware/raspberrypi-pico/api.c#L507

At least, the Serial_w_ack error can be printed from a call just below that.

Looks like you're using the first character of the name to indicate analog or digital. Surely there's another way to look that up, such as from ch->index (which I assume the user cannot change)?

I'd really like to be able to set arbitrary names so I don't have to manually rename in pulseview before I can get a protocol decoder, such as GPIB, to load.

---

A nice-to-have would be the ability to specify an arbitrary ordering for channels. I assume such an order would carry over from sigrok-cli --channels x,y,z... to pulseview. It's not currently possible; I think that's because the contiguous-channels logic equates unsorted with discontiguous.

For example, I'd be able to do sigrok-cli --channels D18=DIO1,D4=DIO2,D5=DIO3,...,D8=NRFD,... to fix a case where I connected the signals in a weird order. Then (I hope) pulseview would show DIO1, then DIO2, then DIO3, and so on.

[pico-coder]

So, fixing the channel renaming will require a change to the libsigrok code, simple, but it will need to through the main libsigrok curation. Will post a link when I have it.
As for the arbitrary reordering that likely won't happen. In order to compensate for the very slow 12MBit/s USB rates I implemented run length encodings at certain numbers of channels. While it's theoretically possible to remap arbitrary channels there is lots of code involved on both the sender and receiver to pull it off, and would be a lot more work (and risk of releasing something broken) than having the user move a jumper :).

[mpictor]

I see, I'd assumed the re-ordering could stay on the driver side and the pico need not know anything about it.

[pico-coder]

sorry for the delay, sigrokproject/libsigrok#256 has the change to allow renaming to work better.

[pico-coder]

I just released a large change which was primarily to support RP2350 and to do a major DMA rewrite. But, it also added protocol changes to allow the device to supply channel names. However, to make use of that I'll need additional libsigrok changes that I'll get to...eventually...