jostlowe / Pico-DMX

A library for inputting and outputting the DMX512-A lighting control protocol from a Raspberry Pi Pico
BSD 3-Clause "New" or "Revised" License
187 stars 21 forks source link

Question: Reason of a specific line calling the irq handler manually #13

Closed kripton closed 2 years ago

kripton commented 2 years ago

https://github.com/jostlowe/Pico-DMX/blob/10d37a4fd0fbdc0a47721df9fc639654024404e9/src/DmxInput.cpp#L157 calls dmxinput_dma_handler(); directly and I don't really understand why. I mean, of course, the write address of the DMA channel has not been set and this happens in the IRQ handler. But I don't understand why/how this works. At this line, the state machine is not yet running, it's only enabled in the line below. So, this DMA channel will NOT trigger any interrupts. That means that inside the IRQ handler, the DMA channel's bit in the INTS-register will not be set. So, when the IRQ handler is called in line 157, it will loop through all DMA channels (https://github.com/jostlowe/Pico-DMX/blob/10d37a4fd0fbdc0a47721df9fc639654024404e9/src/DmxInput.cpp#L104) and handle only those with their INTS-bits set (https://github.com/jostlowe/Pico-DMX/blob/10d37a4fd0fbdc0a47721df9fc639654024404e9/src/DmxInput.cpp#L105). So, in my eyes, calling the IRQ handler manually doesn't make sense.

In one of my branches of my fork, I've replaced the call by those four lines: https://github.com/kripton/Pico-DMX/commit/a5ec3a373fbf2714c5ac3ed0a23cf37e32b22aad#diff-42a9a8ae12afd6562fa620becac01e3c263d29fd4cbcd832a35abbca6cd76fa6R184 and it seems to work now.

Any explanation on why it works in the examples?

By the way, great library, thanks for making it! I'm currently coding a USB->DMX interface that outputs 16 universes and does Wireless DMX in addition (using optional nRF24 modules) (and has an integrated web server and speaks ArtNet and sACN): https://github.com/kripton/rp2040-dongle/tree/rewrite. I planned to do DMX input as well (so it could also do RDM if direction switching (DMX output to DMX input) doesn't take too long). However, I didn't have any real solution to do DMX inputs using the PIOs. I'm really happy I found your library and DMX input was working today for the very first time! :smiley: By the way, schematics for the modular interface is at https://github.com/kripton/rp2040-dongle/tree/kripton-hardware/hardware (Kicad) and some renderings are here: https://github.com/OpenLightingProject/rp2040-dongle/discussions/1#discussioncomment-519937

jostlowe commented 2 years ago

Hi, @kripton and thanks for the nice feedback! It's nice to see that the library is used for something useful :) The dongle you're making looks really interesting. I've been meaning to make something similiar, but haven gotten around to ordering the PCBs. I'll definetly dig through your code looking for inspiration :)

@BollaBerg was previously looking at adding RDM support to the library if that might be of interest to you

jostlowe commented 2 years ago

@functionpointer, it seems these lines came from one of your pull requests :) Maybe you can clarify why the handler is explicitly called here?

functionpointer commented 2 years ago

I think it was to set the write address. You are right about the interrupt bit not being set tough. Now I wonder if we lose all data before the first DMA interrupt

jostlowe commented 2 years ago

Is it a quick fix, or would it require a larger rewrite to fix this? Do you have a chance to take a look at it?

functionpointer commented 2 years ago

Quick fix. Also in real use cases you won't even notice one missing frame right after boot.

kripton commented 2 years ago

Current state of the code sounds like a deadlock to me: The interrupt handler is called explicitely but doesn't do anything since the INTS bit is not set. And the DMA channel's write address is only set in the IRQ handler, so it probably won't be set anywhere. I wouldn't know why the DMA's IRQ would fire if no write address has been set. So I wonder why the code in the current state works at all. Missing a single (the first) frame is not a problem in most "normal" situations where all DMX frames have the start code 0. When talking about RDM, things look different. If you lose the one frame (device replies to controller) you (as the controller) asked for before, this is a problem.

Quick fix proposal is here: https://github.com/jostlowe/Pico-DMX/compare/main...kripton:kripton-multipleFixes Basically do parts of what is done in the IRQ handler in the async_read function: Reset the state machine, clear its FIFOs, set the DMA's write address, trigger it and start the state machine. It could be that we could do with less code in read_async (since there is the call to dma_channel_configure and the one to dma_channel_set_write_addr that might be combined). However, this code makes the DMX input work for me :)

functionpointer commented 2 years ago

Yeah not sure...i kind of suspect it writes it somewhere random and we are lucky it doesnt destroy everything.

Looks like a good fix to me. The real pro gamer move would be to test it with two rpi picos. Get one to output exactly one DMX frame and see if the other one captures it.

kripton commented 2 years ago

The real pro gamer move would be to test it with two rpi picos. Get one to output exactly one DMX frame and see if the other one captures it.

Good idea, I'll do that in the upcoming days. I also have an Arduino-based "RDM responder" that might work quite well for such a test.

kripton commented 2 years ago

Test done, looking quite promising: image

Explanation: D3 of logic analyzer screenshot is DMX data generated by the "device under test" (the one running the DmxInput-code of this library) just to see that its alive. D0 is the DMX data that is being fed into to the device under test by another Pico board. D1 is not used, D2 is the most interesting one: I've modified the DmxInput IRQ handler to pull a GPIO high when entering the IRQ handler and pulling it low when exiting the IRQ handler. As you can see, it triggers reliably in every DMX packet incoming. The position where it triggers also looks good, I've asked read_async to capture only the first 10 channels.

The code of Pico-DMX is https://github.com/kripton/Pico-DMX/commit/57165002135d15162f6e765e50cef0eab6517ae4 plus the minimal changes to have the GPIO high while in the IRQ handler.

Side story: At first, I asked read_async to read 512 channels. Effect was that the GPIO was pulled HIGH only at the beginning of every second DMX frame received. Will be investigated but that story's on another page ;)

kripton commented 2 years ago

Edit: Hm, this is the picture I actually wanted: image

functionpointer commented 2 years ago

Fantastic! Now i am curious, how bad is the current master really?