c0z3n / pySimpleDMX

pysimpledmx is a simple dmx control module in python, for the Enttec DMX USB Pro
58 stars 22 forks source link

DMX indexing is broken #8

Open generalelectrix opened 9 years ago

generalelectrix commented 9 years ago

You specify that addresses are to be given on the range [1,512], yet you use them to directly index into an array of length 512. Unsurprisingly, trying to set channel 512 raises a IndexError.

I would PR but the change it so trivial it would take me more time to fork this repo than for you to just fix it.

c0z3n commented 9 years ago

Thanks for the heads up on this, clearly I missed the disconnection between the docs and the code. I fixed the code to match the docs, which means that applications correcting for this quirk on their own will probably be broken. This may have been a poor way for me to go about it.

generalelectrix commented 9 years ago

Heh, it gets weirder than that, though. I need to dig into the drivers I've written for the enttec in other languages for what's going on behind the scenes, but using your code I actually have to address into the frame as dmx_frame[1] to write to the first channel in the enttec output. So, I think there may also be a missing byte or something in how you're writing to the header. So, this is actually two bugs which combined give you the expected behavior, except for when trying to write to channel 512.

generalelectrix commented 9 years ago

I've also refactored to make the render command more efficient, using a generator expression instead of a list, and creating the packet start and end lists only once to avoid unnecessary allocations during render. Here's the gist of it:

packet_start = [START_VAL,
                LABELS['TX_DMX_PACKET'],
                DMX_SIZE & 0xFF,
                (DMX_SIZE >> 8) & 0xFF,]
packet_start = ''.join([chr(v) for v in packet_start])
packet_end = chr(END_VAL)

class DMXConnection(object):
    # ...
    def render(self):
        ''''
        Updates the DMX output from the USB DMX Pro with the values from self.dmx_frame.
        '''
        dmx_payload = (chr(v) for v in self.dmx_frame)

        self.com.write(packet_start + ''.join(dmx_payload) + packet_end)
    # ...
c0z3n commented 9 years ago

unfortunately this sounds like it goes deeper than I am able to debug without the ENTTEC box itself, which I have loaned out to a friend. If you are able to figure out what the issue is and fix it in a way that seems to make the most sense to you, I would be happy to merge any pull requests you come up with!

generalelectrix commented 9 years ago

I've decided to just port my existing enttec implemention for Rust to Python, which exposes more of the underlying port options (truncated universes, setting the hardware DMX framerate, etc).

Should have a finished thing up by later today. https://github.com/generalelectrix/pyenttec

On Sun, Jul 12, 2015 at 12:52 PM, c0z3n notifications@github.com wrote:

unfortunately this sounds like it goes deeper than I am able to debug without the ENTTEC box itself, which I have loaned out to a friend. If you are able to figure out what the issue is and fix it in a way that seems to make the most sense to you, I would be happy to merge any pull requests you come up with!

— Reply to this email directly or view it on GitHub https://github.com/c0z3n/pySimpleDMX/issues/8#issuecomment-120756678.

c0z3n commented 9 years ago

awesome, I'm psyched to have someone who is more familiar with the ENTTEC hardware than I am working on this. It may have been easier to do it as a fork so we could pull things back into one place easier but maybe you had some other logic there that I'm not seeing. Either way, we'll make it work.
:smile:

generalelectrix commented 9 years ago

I decided a re-write and a fresh codebase was the way to go. My re-write is full of breaking changes compared to this codebase: DMX indexing from 0, no clamping of DMX values (not easy to support for clients writing directly to the dmx_frame buffer using a slice), raising exceptions instead of print/system exit calls, refactored pep8 method names, removal of "autorender".

c0z3n commented 9 years ago

fair enough. My code was always meant as a just-good-enough-to-get-by solution anyways, so it'll be good to have a more polished option. Looking forward to reading through what you put together

generalelectrix commented 9 years ago

It's up. I may also post a generic multiprocessing tool that you can call which spins up the port in its own process to enforce timely rendering, with queue-based communication. Usually this process would want to be doing more than just storing values and writing to serial, though.

I'd be interested in benchmarking the version I posted vs. yours; I don't know how fast/slow writing to serial is in python, but I wouldn't be surprised if the reason that your "autorender" feature was slow was the memory allocations in render().

On Mon, Jul 13, 2015 at 11:10 AM, c0z3n notifications@github.com wrote:

fair enough. My code was always meant as a just-good-enough-to-get-by solution anyways, so it'll be good to have a more polished option. Looking forward to reading through what you put together

— Reply to this email directly or view it on GitHub https://github.com/c0z3n/pySimpleDMX/issues/8#issuecomment-121009797.

generalelectrix commented 9 years ago

Oh, also, the source of your address-off-by-one bug is that the packet length should actually be len(dmx_frame)+1, and requires a pad byte between the two length bytes and the start of the DMX payload.

(Sorry, posted this first from the wrong github account)

On Mon, Jul 13, 2015 at 11:14 AM, chris macklin general.electrix@gmail.com wrote:

It's up. I may also post a generic multiprocessing tool that you can call which spins up the port in its own process to enforce timely rendering, with queue-based communication. Usually this process would want to be doing more than just storing values and writing to serial, though.

I'd be interested in benchmarking the version I posted vs. yours; I don't know how fast/slow writing to serial is in python, but I wouldn't be surprised if the reason that your "autorender" feature was slow was the memory allocations in render().

On Mon, Jul 13, 2015 at 11:10 AM, c0z3n notifications@github.com wrote:

fair enough. My code was always meant as a just-good-enough-to-get-by solution anyways, so it'll be good to have a more polished option. Looking forward to reading through what you put together

— Reply to this email directly or view it on GitHub https://github.com/c0z3n/pySimpleDMX/issues/8#issuecomment-121009797.