Pico-DMX icon indicating copy to clipboard operation
Pico-DMX copied to clipboard

DmxInput: start_channel is not honored

Open kripton opened this issue 2 years ago • 15 comments

DmxInput::begin has a parameter called start_channel. It's meant to ignore the first X channels and only read input values from that value on. However, the value is not actually used as expected. It is however (incorrectly) used to calculate the buffer size here: https://github.com/jostlowe/Pico-DMX/blob/e493ea2923cf465cbb811836e098ff87e9f2ff20/src/DmxInput.cpp#L163

I wouldn't know a proper solution to tell the PIO state machine OR the DMA to ignore the first X bytes. Of course, we could let the DMX write into an intermediary buffer and then copy only the requested channels to the buffer provided by the library user. But in the worst case, that would mean copying 511 byte in an IRQ handler. Not what I would call ideal.

As such I would propose to remove the start_channel functionality

kripton avatar Dec 14 '21 22:12 kripton

It looks like the start_channel feature was in and working (only when using read) before #8, but was removed when read was re-written to use read_async to do its job: https://github.com/jostlowe/Pico-DMX/pull/8/files#diff-42a9a8ae12afd6562fa620becac01e3c263d29fd4cbcd832a35abbca6cd76fa6L84

kripton avatar Dec 15 '21 10:12 kripton

I can't get DMX channel one to be read properly from my Chauvet Light board. IF I use DMXOutput and DMXInput then it works "right?". Or are both off by one? How is start byte (normally 0) handled? Does the returned buffer put channel 1 at index 0 or 1? I think it would be clear if the entire DMX packet were read in including the Start byte so that channel 1 is indexed as [1] This would let one use RDM as well as avoiding confusion between index of array and channel number.

Is the problem here? in .cpp we have &_pio->rxf[_sm], // src DMXINPUT_BUFFER_SIZE(_start_channel, _num_channels), // transfer count, (is it src, dest, count?) the .h file defines #define DMXINPUT_BUFFER_SIZE (start_channel, num_channels) (num_channels+1) in the version I have, though it is different in the one referenced by kripton in the previous comment #define DMXINPUT_BUFFER_SIZE(start_channel, num_channels) ((start_channel+num_channels+1)+((4-(start_channel+num_channels+1)%4)%4)) Presumably this was before the 32 bit transfer not doing 513 bytes issue was resolved by doing 8 bit transfers

Gavin-Perry avatar Mar 24 '22 07:03 Gavin-Perry

How is start byte (normally 0) handled? Does the returned buffer put channel 1 at index 0 or 1? I think it would be clear if the entire DMX packet were read in including the Start byte so that channel 1 is indexed as [1] This would let one use RDM as well as avoiding confusion between index of array and channel number.

I didn't try it out lately but according to the Readme any my memory, the 0th byte in the buffer is the start code, the byte indexed 1 is the value of the 1st channel in the universe

kripton avatar Mar 27 '22 18:03 kripton

Yes, that is how it is supposed to work according to DMX standard. My question is how the buffer array is done in the library since it isn't working right.

-- Gavin Perry, PhD Chief Technology Officer Meridian Electric Company 2392 Grissom Drive St Louis MO 63146 http://meridianlighting.com cell: 314 406-0697 http://voice.google.com/calls?a=nc,%2B13144060697

On Sun, Mar 27, 2022 at 1:29 PM kripton @.***> wrote:

How is start byte (normally 0) handled? Does the returned buffer put channel 1 at index 0 or 1? I think it would be clear if the entire DMX packet were read in including the Start byte so that channel 1 is indexed as [1] This would let one use RDM as well as avoiding confusion between index of array and channel number.

I didn't try it out lately but according to the Readme any my memory, the 0th byte in the buffer is the start code, the byte indexed 1 is the value of the 1st channel in the universe

— Reply to this email directly, view it on GitHub https://github.com/jostlowe/Pico-DMX/issues/15#issuecomment-1079989741, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVR4PPZZIKTXL45JY3ILJCDVCCZILANCNFSM5KCB4KXQ . You are receiving this because you commented.Message ID: @.***>

Gavin-Perry avatar Apr 03 '22 18:04 Gavin-Perry

If it's not working right (= as expected), I would assume that the DMX sender sends "short frames" with fewer than 513 slots.This is perfectly fine according to the standard but the library currently cannot cope with it. As you said, it would be helpful to have an IRQ on BREAK or MAB but it'S currently just not implemented.

kripton avatar Apr 16 '22 19:04 kripton

Thanks for your help. As it turns out the pins I used for setting the address (GPIO 26-28) Don't work with digitalRead() or gpio_get()

Minimum code gpio_set_dir(26, GPIO_IN); gpio_pull_up(26); gpio_set_dir(27, GPIO_IN); gpio_pull_up(27); gpio_set_dir(28, GPIO_IN); gpio_pull_up(28); Serial.println(gpio_get(26)); Serial.println(gpio_get(27)); Serial.println(gpio_get(28));

Always return 0! So I wasn't setting the address I thought! Work around was to use analogRead() and set a threshold value (400) below = 0; above =1. Don't know if the problem is arduino-pico but I suspect not, since gpio_get is reaching through via the SDK and not using arduino code.

-- Gavin Perry, PhD Chief Technology Officer Meridian Electric Company 2392 Grissom Drive St Louis MO 63146 http://meridianlighting.com cell: 314 406-0697 http://voice.google.com/calls?a=nc,%2B13144060697

On Sat, Apr 16, 2022 at 2:56 PM kripton @.***> wrote:

If it's not working right (= as expected), I would assume that the DMX sender sends "short frames" with fewer than 513 slots.This is perfectly fine according to the standard but the library currently cannot cope with it. As you said, it would be helpful to have an IRQ on BREAK or MAB but it'S currently just not implemented.

— Reply to this email directly, view it on GitHub https://github.com/jostlowe/Pico-DMX/issues/15#issuecomment-1100744544, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVR4PP4RJAIDQOKVMI3YXMTVFMLQPANCNFSM5KCB4KXQ . You are receiving this because you commented.Message ID: @.***>

Gavin-Perry avatar Apr 17 '22 02:04 Gavin-Perry

@Gavin-Perry i've used 26-28 on my Pi Pico (non arduino) without any issues so I presume that the issues you're referencing are arduino specific

I haven't tried the ADC specifically however if adc_init is called from somewhere in the arduino source it may block the default GPIO functions... I had this issue with I2C

I can't confirm this just yet as I'm currently afk for a few days but suspect it may be the case

I also don't have an arduino pico setup so can't confirm that way however from what I do know, this is most likely the reason...

queekusme avatar Apr 20 '22 16:04 queekusme

Thanks for the idea. I don't use adc_init, and I explicitly did call gpio_set_dir(i,GPIO_OUT) and then gpio_pull {up or down} The pull worked for setting my defaults. I'd call gpio_set_function() but the choices don't include simple gpio input as far as I can tell. All the fancy uses are documented, not the simplest which should be (and claimed to be) default.

Gavin-Perry avatar Apr 20 '22 17:04 Gavin-Perry

@Gavin-Perry you shouldn't need to set the function,

The enum is defined here (https://raspberrypi.github.io/pico-sdk-doxygen/group__hardware__gpio.html) as gpio_function,

If you're just reading digital inputs then you can ignore function, that's for connecting the hardware spi/i2c/pwm modules to the pins, heck you shouldn't even need to call that for adc, as that's handled by adc_init

queekusme avatar Apr 21 '22 14:04 queekusme

I now realize that now. Just flailing at options to figure out the problem with digital input which should be the default. And learning some of the cool things that this baby can do.

Gavin-Perry avatar Apr 21 '22 16:04 Gavin-Perry

Looks like I need to use gpio_init() because the Arduino boot loader isn't enabling them.

Gavin-Perry avatar Apr 21 '22 20:04 Gavin-Perry

I'd like to mention that this issue is affecting my use of this library.

MarcusGarfunkel avatar Oct 11 '22 23:10 MarcusGarfunkel

I'm a bit short on time these days! @kripton would it be possible for you to author a pull request to remove/fix the feature?

jostlowe avatar Oct 12 '22 07:10 jostlowe

Unfortunately, I currently also don't have much spare time to donate to this project. So please don't expect a PR for this, soon.

However, I don't see why this is a big blocker: As long as there are "full packets" = start code + 512 channels on the wire, the reception should be fine. Yes, you will need to provide a 513 byte large buffer, receive the complete packet and then use only the parts you are interested in. In the worst case, it's a 512 byte large memcpy/memmove operation.

@MarcusGarfunkel: Any point I missed? Is that workaround not working for you?

kripton avatar Oct 13 '22 21:10 kripton