Questionable code

[rgrr]

Hi,

in check_half() there is some questionable code contained:

uint8_t myachan=(((uint32_t) tstsa0)>>6)&0xF;
uint8_t otherachan=(((uint32_t) tstsa1)>>6)&0xF;
uint8_t mydchan=(((uint32_t)tstsd0)>>6)&0xF;
uint8_t otherdchan=(((uint32_t)tstsd1)>>6)&0xF;

I think this should be something like (stress is on "[0]")

uint8_t myachan = ((tstsa0[0] & DMA_CH0_CTRL_TRIG_RING_SIZE_BITS) >> DMA_CH0_CTRL_TRIG_RING_SIZE_LSB);
uint8_t mydchan = ((tstsd0[0] & DMA_CH0_CTRL_TRIG_RING_SIZE_BITS) >> DMA_CH0_CTRL_TRIG_RING_SIZE_LSB);

Also the proc_fail expression throws a compiler warning. My guess is, that this should be

bool proc_fail =    (d->a_mask != 0  &&  (*tstsa1 & DMA_CH0_CTRL_TRIG_BUSY_BITS) == 0)
                 || (d->d_mask != 0  &&  (*tstsd1 & DMA_CH0_CTRL_TRIG_BUSY_BITS) == 0);

[pico-coder]

First off my apologies for some horrendous code. One of the "big features" I implemented was the ability to infinitely/continuously stream samples from the device at sample counts far greater than the RAM depth of the rp2040, and doing so without losing samples. That required all the ugly chaining and status checking, which ideally should have eventually been migrated to being interrupt based. As usual, I got it working, so I shipped it.....

Regarding myachan and other signals, it is ugly, but it works. tstsa0 is a pointer, and as you mention it is typical to dereference the pointer to get it's value. However, this is kind of a hack to get the channel number that the pointer points to by extracting address bits from it. Notice how all the CH*_CTRL_TRIG registers are staggered by 0x40 in the address map, so you shift by 6 and mask with 0xF to get the 4 bit channel number. The channel number is then used to establish the chain with the other dma controller. If check_half() was provided with the actual channel number it could use that, but the pointer was available so I used that.

proc_fail will take some further squinting, trying to properly detect when we had real sample losses and not was a bit problematic. When I compile I don't get any warnings, what warnings do you see?

[rgrr]

I fully understand that you are trying to squeeze the last bit out of the device, so no apologies required. Actually I have to, because I did not get the trick ;-) So you are actually extracting the DMA number from the address, right?

Concerning the compiler warning: it complains as told about the proc_fail assignment:

/mnt/d/u/pico/picoprobe/src/pico-sigrok/sigrok.c: In function 'check_half':
/mnt/d/u/pico/picoprobe/src/pico-sigrok/sigrok.c:559:26: warning: '~' on a boolean expression [-Wbool-operation]
  559 |         proc_fail =     (~(((((*tstsa1) >> 24) & 1)  ||  (d->a_mask == 0))
      |                          ^
/mnt/d/u/pico/picoprobe/src/pico-sigrok/sigrok.c:559:26: note: did you mean to use logical not?
  559 |         proc_fail =     (~(((((*tstsa1) >> 24) & 1)  ||  (d->a_mask == 0))
      |                          ^
      |                          !

To my understanding you are checking if the DMA is still running after setup of the ring logic. But perhaps I'm mistaken again.

[pico-coder]

Yes, extracting the DMA number from the address. As for the proc_fail looks like I just need replace a ~ with a !, though that code could use a cleanup to be more readable. The idea with proc_fail is that when one DMA transfer completes you have to read and process all the data, and send it out to USB and start up the DMA engine that is idle. If the 2nd dma engine finishes in that time then it means that neither DMA engine was running and data was likely lost and you have to report an abort to the host that samples were missed.

[rgrr]

Thanks for the explanation. A real hack and I like it ;-)

Just to tell: I'm trying to integrate your module into my current project: https://github.com/rgrr/yapicoprobe

At the moment it seems that I have broken something due to my modifications, but I will it.

I'm closing this issue.