Add support for DALI 24-bit event messages

[sl-wallace]

IEC 62386 part 103 defines "event" messages, which can be sent by control devices to indicate various inputs or changes. This commit introduces support for encoding and decoding these messages, as well as a handful of new sequences for making it easier to work with them.

Support has been added for IEC 62386 part 301 "push button" devices, further device types should be simple enough to add in later. Unsupported types will be safely ignored by the library.

[sl-wallace]

Hi @sde1000, this is another go at my attempt to submit the code I've written to support event messages in python-dali. I closed #103 because I'd made too much of a mess of it, and like to keep a tidy commit history. ;)

There's a lot of code here, so I understand if you haven't got the time to review it. But, if you do, does it look like an appropriate style to merge into master? The only thing I wasn't really sure about was where the "event" code should live, I've got it in the general module under device - but it could be in a separate event module if that would help with usability. I figured though that the IEC standard has all of the part 103 things in the same document, so the base implementation should follow this and use just the one module.

[sl-wallace]

Also I've got it marked as a draft for now, I think I should add a few more unit tests before requesting it to be merged - but I was keen to get something out for discussion

[sde1000]

Firstly, thank you very much for working on this! It's been on my "to-do" list for ages, but I haven't been able to get hold of any 24-bit DALI hardware to test with. (Which means that I won't be able to sanity-check the code against real hardware; I'll be relying on a good "fakes" implementation...)

Regarding commit history: please feel free to rebase, force-push, etc. to keep a clean commit history for the eventual merge. Or maybe, when the code is ready go through an interactive rebase to reorder and squash commits as necessary?

Design notes:

I don't think dali.device.general.device_instance_map can be a global. What happens when a program using python-dali needs to deal with more than one DALI bus? I think it should be a parameter to from_frame.

I think having the various classes in dali.address try to work for both gear and devices was a design error on my part. dali.address.Group(25) unambiguously refers to a device; dali.address.Group(10) could refer to gear or a device depending on context. dali.address.Short(anything) is ambiguous. I think it would be better to have (for example) dali.address.GearGroup() and dali.address.DeviceGroup() to remove the ambiguity. (Likewise dali.address.GearShort(), dali.address.DeviceShort(), dali.address.GearBroadcast(), dali.address.DeviceBroadcast(), etc.)

Sequences: if the address classes are no longer ambiguous, it should be possible to make the sequences in dali.sequences work for both gear and devices, issuing the appropriate commands depending on the type of address passed in.

I see you've created dali.device.sequences; I think that's fine for device-specific sequences. (And if we end up with any gear-specific sequences they can go in dali.gear.sequences.)

no_register — the code for this appears to have been repeated redundantly through the _DeviceCommand subclasses? And I think it could do with a different name (eg. instance_specific?) that declares why the subclass needs to be treated differently, not how to do that.

[sl-wallace]

Regarding commit history: please feel free to rebase, force-push, etc. to keep a clean commit history for the eventual merge. Or maybe, when the code is ready go through an interactive rebase to reorder and squash commits as necessary?

Yeah sounds like a plan, I think I was a bit premature in opening and closing #104 , I'll just rebase and squash to make things neat.

I don't think dali.device.general.device_instance_map can be a global.

Fair point, I wasn't quite sure about this one and am always hesitant of globals anyway. I'll play with making it a parameter to from_frame, I think that should work nicely.

I think it would be better to have (for example) dali.address.GearGroup() and dali.address.DeviceGroup() to remove the ambiguity

Yeah I think so too, there were a few parts already where I had to add some extra checks to avoid ambiguity. The only problem would be backward compatibility – would it be better to just leave the dali.address.Short as-is, and add DeviceShort? Otherwise everything will break. Or, at the very least, redefine to GearShort but still keep Short as an alias, probably with a deprecation warning.

I see you've created dali.device.sequences; I think that's fine for device-specific sequences

Yeah I figured this made sense. The only thing I wasn't sure about, there's sequences specific to the device types too, e.g. SetPushbuttonEventFilters. This can't be generic because each type can have different filters, and can use one to three bytes to do so in DRT0-DTR2. Ideally this could be referenced something like dali.device.pushbutton.sequences.SetEventFilters, but that's probably too long, right? I don't know how strict to be in separating things by functionality.

[sde1000]

redefine to GearShort but still keep Short as an alias, probably with a deprecation warning

I'd do this, but without a deprecation warning. Keeping both names shouldn't be a problem.

Ideally this could be referenced something like dali.device.pushbutton.sequences.SetEventFilters, but that's probably too long, right? I don't know how strict to be in separating things by functionality.

No need to be pedantic about having all sequences be in modules called sequences! The memory subsystem defines lots of sequences for reading and writing memory (eg. dali.memory.info.GTIN.write()) without doing this; just put them wherever is sensible.

[sl-wallace]

@sde1000 I'd be interested to know what you think of my implementation of the separate gear and device addresses in 4e72d72... :smile:

[sde1000]

I would include an additional pair of address classes: GearAddress and DeviceAddress that inherit directly from Address, and then have the rest of the classes inherit from the appropriate one of those. (Perhaps set an attribute like required_frame_size in them? At some point it's probably worth optimising Address.from_frame() in the same way I did Command.from_frame() in a4dc1853a3989314856029429e931d6c34a29106 and this attribute would be useful for that.)

It would then be easier to make some of the generic sequences work for both gear and devices: check whether the address is a GearAddress or DeviceAddress and choose the appropriate set of commands. This would be particularly useful in the memory subsystem — memory banks work exactly the same way on gear and devices.

[sl-wallace]

I would include an additional pair of address classes: GearAddress and DeviceAddress that inherit directly from Address

Yeah good idea, I've made some changes to implement this.

This would be particularly useful in the memory subsystem — memory banks work exactly the same way on gear and devices.

I've also made changes to begin to implement this very thing, I realised that the system I'm making will need to read from the memory banks to pull information on the manufacturer and serial number etc. So I've made changes to the memory module (and tests) to make the methods able to use either 16 or 24 bit commands as necessary.

[sl-wallace]

I think it's pretty close to being ready now, I just need to smooth a few rough edges and add some more tests and examples.

[sl-wallace]

Probably the only big thing I see that's missing now is some sort of documentation. Maybe I at least create an issue in GitHub for that, and pursue it separately? I've learnt a lot about python-dali over the last couple of months, and I'm sure I could write some of that down. The only other issue I've realised is that I don't know which of the other drivers and devices support 24 bit commands - as we noticed, the Tridonic HID driver doesn't do it (yet). That's not really an issue with this PR I suppose, but it's still a shame that it's pretty much unusable without a Lunatone LUBA interface.

[sde1000]

I'm happy for the documentation to be written later! I plan to take some time soon to go over the code in this pull request more closely, but from what I've read so far I expect I'll be merging it as-is.

None of the other drivers support 24 bit commands. Nobody has had any 24 bit hardware to test with! Thank you for sending me the two devices to test with: I'll put my test rig back together and try to get the DALI USB device working with them.

[sde1000]

General comment: with a couple of small exceptions, python-dali is formatted to a maximum line length of 80. Could you get your code to fit in with this?

[sde1000]

"Aliased" commands: can we do better than just delcaring that these will only ever decode to the command they are an alias of? Could we make use of the DeviceInstanceTypeMapper, for example?

[sl-wallace]

Should be 64 to cover addresses 0 to 63 inclusive!

...formatted to a maximum line length of 80

Thanks for that, I'll fix up the range(64) mistakes and reformat to 80 characters width. :smile:

---

"Aliased" commands: can we do better than just delcaring that these will only ever decode to the command they are an alias of? Could we make use of the DeviceInstanceTypeMapper, for example?

I'll have a look and see if I can just get rid of it, or replace it with something a bit neater - the only place I ended up using the "alias" concept was in the QueryEventFilterPushbutton message, which is really a QueryEventFilterZeroToSeven. I imagine there will be further messages like this for other event types (e.g. I'll work on motion sensors next), but there's also the possibility that event filters for some types can actually be up to 3 bytes - I haven't seen that yet in Part 301-304, but if there was a type which used more than one byte of event filters then it would be neatest to use a sequence to do it all at once and return the full set of enabled event filters. So I think what I might do is remove QueryEventFilterPushbutton as a command class, and instead have a sequence that does the same job, still decoding the response into an EventFilterPushbutton enum. Do you think that would be a good idea?

[sde1000]

Perhaps we could just declare the number of bits of event filter and their meanings for each instance type in a standard way? Then we could have sequences for setting and reading the event filter of an arbitrary instance that would issue the most efficient series of commands to do so.

Do the bits of the event filter always relate to particular event types, or is this defined separately in each part 3xx?

[sde1000]

I got the DALI USB device working with 24 bit frames. I can't make a pull request out of it at the moment because it depends on your changes, but here's a gist: https://gist.github.com/sde1000/2fcf07018610cfd148898e51832c724a (I'm still not happy with it; it still deadlocks or otherwise fails sometimes and I haven't worked out why.)

[sl-wallace]

Do the bits of the event filter always relate to particular event types, or is this defined separately in each part 3xx?

They're defined differently for each part.

This is Part 301 "Push buttons":

And this is Part 302 "Absolute input device":

Perhaps we could just declare the number of bits of event filter and their meanings for each instance type in a standard way?

Yeah perhaps each instance type implementation just needs to define an enum of bits, in a standard way, and there could be a generic sequence that would use the DeviceInstanceTypeMapper to figure out based on device address and instance number what the targeted instance type is, and could use that knowledge to encode and decode the appropriate filter messages?

Perhaps something like this:

def SetEventFiltersSequence(dev_addr, inst_num, filters_enum):
    # figure out inst_type through DeviceInstanceTypeMapper based on dev_addr, inst_num

    # check filters_enum aligns with inst_type

    # encode filters_enum into filters_bits

    # encode and send DTR0(filters_bits[0:7]), 
    # optionally send DTR1(filters_bits[8:15]) and DTR2(filters_bits[16:23])

    # send SetEventFilter()

And similarly for querying event filters, use DeviceInstanceTypeMapper to know which enum to use to query and then decode the event filters.

I'll try and get something put together to test this out.

[sde1000]

If we are invoking the set/query sequence, I would expect us to know which event filter enum to use in advance! I don't think the sequence would have to check.

I'm interested in the case where we're observing bus traffic and see one of these commands go by. Can we decode the parameters and response — i.e. if the bus traffic observer has a suitable DeviceInstanceTypeMapper, is there a standard way for it to fetch the appropriate enum?

[sl-wallace]

If we are invoking the set/query sequence, I would expect us to know which event filter enum to use in advance! I don't think the sequence would have to check.

Oh yeah, good point. So then the sequence would easily be able to set the appropriate DTR0-DTR2 commands depending on length.

I'm interested in the case where we're observing bus traffic and see one of these commands go by

I see what you mean, for instance could we observe a QueryEventFilterZeroToSeven and figure out that because it's addressed to a certain device and instance that this is actually a QueryEventFilterPushbutton, and the response will hence be a QueryEventFilterPushbuttonResponse. That should be possible, I'll keep this alias concept in the code then.

It would be harder to do it for the "set" commands, because it relies on the DTR registers, but the "alias" commands don't cover that anyway.

[sl-wallace]

I see what you mean, for instance could we observe a QueryEventFilterZeroToSeven and figure out that because it's addressed to a certain device and instance that this is actually a QueryEventFilterPushbutton, and the response will hence be a QueryEventFilterPushbuttonResponse.

I've now implemented this, and added a test for it (see test_frame_to_query_event_filter_with_type_mapping()). It works basically like we discussed, in the from_frame() method for the instance class, if it comes across any class for which there is a subclass with the alias_instance_type attribute set, then it will try to see if it can figure out based on the device address and instance number if there is a more specific command to decode to. The test I have demonstrates decoding a QueryEventFilterZeroToSeven into a QueryEventFilterPushbutton based on setting up a mapping.

[sl-wallace]

Thanks for that, I'll fix up the range(64) mistakes and reformat to 80 characters width. smile

And I think I've now got everything formatted to 80 characters wide.

[sl-wallace]

Just made a very minor addition, to include an optional attribute enabled_by to the _Event subclasses - the idea being that an event class stores a reference to which event filter is used to enable it, e.g. the ButtonReleased class has enabled_by = EventFilterPushbutton.button_released. python-dali itself doesn't make use of this, but the higher-level abstraction library I'm working on would benefit from this so that it can figure out which filters to enable on an instance to start receiving event messages of a certain type.

I think it's ready to go now! Or at least ready enough, I can make a new pull request if anything else comes up.

[sde1000]

enabled_by

enabled_by: a good idea! Will there always be a one-to-one mapping between event classes and bits in the event filter? If so, perhaps there's no need to define the event filter type explicitly, it can be constructed automatically from the event classes. (But see below regarding bitmap types.)

Bitmap responses

Subclasses of command.BitmapResponse — I think you have missed some of what this class does. It uses the strings in bits to add attributes to the response instance, for example:

>>> dali.gear.general.QueryStatusResponse(dali.frame.BackwardFrame(255)).lamp_failure
True

This doesn't work with (for example) dali.device.QueryDeviceStatusResponse:

>>> dali.device.general.QueryDeviceStatusResponse(dali.frame.BackwardFrame(255)).input_device_error
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/steve/python-dali/dali/command.py", line 152, in __getattr__
    raise AttributeError
AttributeError

This is because the strings from bits = list(DeviceStatus) are of the form "DeviceStatus.input_device_error" etc. and can't be mangled to valid attribute names.

Looking at it now, I'm not happy with the design of command.BitmapResponse. I should have implemented something more general. In my defense, it was written before enum and enum.IntFlag existed, and none of the dali.gear commands take bitmaps as arguments!

Enums in general

I see you've added dali.command.StrEnum, and subclasses dali.gear.general.GearEnum (unused) and dali.device.general.DeviceEnum. Are the subclasses necessary?

What was the thinking behind using a string enum instead of an int enum?

I don't think it's sensible to conflate enums for bit maps and value maps. (And bitmap enums may not be necessary anyway, see below.)

Str/int details aside, I think using enums is sensible. What you did for dali.device.general.QueryEventSchemeResponse should probably be generalised, eg. create dali.command.EnumResponse to be subclassed to specify the particular Enum type.

Instance event filter

With the instance event filter, we have a bitmap that has a variable number of bits up to 24 that can be both set and read using commands in dali.device.general. I think we need a type that can represent this, which can be used as an argument to a sequence that sets the event filter, and as a return value from a sequence that reads the event filter. No need for instance type-specific sequences like dali.device.sequences.SetPushbuttonEventFilters.

Is this the only case where a bitmap type is needed as both an argument and return value? I think it is at the moment. Are there likely to be others needed in the future?

Instance type specific commands

dali.device.pushbutton.QueryEventFilterPushbutton and the alias_instance_type attribute: I don't think this mechanism is necessary in this case. It's only being used so you can have a particular response type for the command. It works for pushbuttons because there are only 8 bits in the pushbutton event filter, but it doesn't generalise to 16 or 24 bits. I think we should be using a sequence instead, that takes the event filter type as a parameter and emits the appropriate selection of QueryEventFilter[xxx] commands, returning an event filter type instance.

However, what happens with the other instance-specific commands? dali.device.pushbutton.SetShortTimer is a _StandardInstanceCommand with an opcode of 0x00. Are other instance types going to define other _StandardInstanceCommands with an opcode of 0x00 too? If so, we need something like the alias_instance_type attribute to be able to tell them apart, rather like the devicetype mechanism for gear commands.

Feature specific commands

We don't have any support for "features" at the moment. Are they also going to have commands that share the opcode space with other features?

[sl-wallace]

enabled_by

enabled_by: a good idea! Will there always be a one-to-one mapping between event classes and bits in the event filter?

Thanks! And unfortunately not always, for example with the push button types there are the separate "button stuck" and "button free" events which are both enabled/disabled together by the "button stuck/free event enabled" filter. I've peeked ahead and had a look at what the other instance types use (which I intend to implement at some point), it seems like the occupancy sensor types also don't use a 1:1 mapping, so I think an explicit reference is needed.

---

Bitmap responses and enums in general

This is because the strings from bits = list(DeviceStatus) are of the form "DeviceStatus.input_device_error" etc. and can't be mangled to valid attribute names

Ah OK, I didn't realise that was how it worked. I'll take another look, and at how I've used enums in general, and see what I can come up with. I like the idea of a dali.command.EnumResponse, that could help neaten things up.

Are the subclasses necessary?

No, not really. I'll see about combining them, in with a general rethink of the enums.

What was the thinking behind using a string enum instead of an int enum?

I'd like to have a readable version of enum values, for e.g. better reporting errors and the like. Perhaps though having the string in the enum itself isn't the best way to do it - and one thing I've become aware of from working with Home Assistant is the need to consider translations, so perhaps the enums ought to be int enums and then there can be some translation method which can take any of the int enums and turn the value into a translated string, purely for feedback reasons. Unfortunately for that effort I will only be able to help with English translations, but I suppose it would be written in a way that it's easy for someone to contribute other languages. Ideally too this will be written in a way that the string manipulation and translation handling isn't loaded by default, so that users who don't care won't waste memory on all the strings.

What are your thoughts on this?

---

Instance event filter

I think we need a type that can represent this, which can be used as an argument to a sequence that sets the event filter, and as a return value from a sequence that reads the event filter

It works for pushbuttons because there are only 8 bits in the pushbutton event filter, but it doesn't generalise to 16 or 24 bits.

Good point. I like this idea, then e.g. the pushbutton module implements some kind of EventFilter type which can be used in a generic sequence as required. And can also be translated I suppose. Interestingly, at least in the 3xx documents I have so far for the hardware that I have, event filters never exceed 8 bits - so I don't have a way to confirm how that would actually work with real hardware. But I suppose I can implement it following the spec and then one day if anyone ever has some hardware to use it they'll just have to file a bug if it doesn't work!

---

Instance type specific commands

what happens with the other instance-specific commands? dali.device.pushbutton.SetShortTimer is a _StandardInstanceCommand with an opcode of 0x00. Are other instance types going to define other _StandardInstanceCommands with an opcode of 0x00 too?

I feared this, but fortunately not - it seems the authors of the standard took mercy on those implementing it and didn't double up, avoiding a need for the "enable device type" kind of commands of the control gear sections. In all of the 30x parts I have access to (301 to 304), different opcodes are used. e.g. "Set Short Timer" from Part 301 push buttons is opcode 0x00, through to "Query Stuck Timer" with 0x0F; then in Part 302 "Set Report Timer" is opcode 0x10 and "Query Switch" is 0x1F; Part 302 is 0x20 to 0x2F; and Part 304 is 0x30 to 0x3F (I see a pattern here!). It seems this pattern could continue up to Part 316, with 0xF0 to 0xFF, I don't know what would happen for a hypothetical Part 317 - but that doesn't seem to be on the cards.

---

Feature specific commands

We don't have any support for "features" at the moment. Are they also going to have commands that share the opcode space with other features?

I haven't got access to e.g. Part 332 and 333 for the feature types, I'm not really familiar with how they work and if they have opcodes too. I suppose the opcodes could overlap, but the "instance" byte would be set to address a feature rather than the instance (seems like bit 13 in the frame flags this). I'm very much hoping that it would continue the seemingly sensible design of Parts 301-304 and not cause overlaps, after all they have three bytes to send commands in now so really there should be enough space to have fully unique commands. If I can get some hardware which implements these "instance features" then I'll take a look at it.

---

I'll have a go at making some further changes, and then push again for more discussion. Thanks for your help!

[sde1000]

enabled_by

And unfortunately not always, for example with the push button types there are the separate "button stuck" and "button free" events which are both enabled/disabled together by the "button stuck/free event enabled" filter.

Ah, ok. So we will need to declare the event filter type explicitly and reference it in each event class: fair enough!

Enums

I'd like to have a readable version of enum values, for e.g. better reporting errors and the like.

What are your thoughts on this?

My point of view is that the audience of python-dali is developers. It should not contain any strings intended for display to users of applications that make use of python-dali. Enums are good because they provide symbolic names for constants from the standard, and help with type safety.

I'm generally against the use of the "auto" enum feature for enums that declare things from the standard: I'd prefer the values from the standard to be explicit in the library code.

An example from dali.gear.general:

class QueryDeviceTypeResponse(command.Response):
    _types = {0: "fluorescent lamp",
              1: "emergency lighting",
              2: "HID lamp",
              3: "low voltage halogen lamp",
              4: "incandescent lamp dimmer",
              5: "dc-controlled dimmer",
              6: "LED lamp",
              254: "none / end",
              255: "multiple"}

    def __str__(self):
        if self.value and self.value.as_integer in self._types:
            return self._types[self.value.as_integer]

        return "{}".format(self.value)

Good: the numbers are explicit. Bad: if I was implementing this now, I'd use an IntEnum for the device types and then make use of it in the response class. (Perhaps doing this can go on a list of things to do if I ever make a release that bumps the major version number!)

In dali.device.general I think EventScheme should be an enum.IntEnum and QueryEventSchemeResponse should be a subclass of dali.command.EnumResponse, and I should eventually work out how to change QueryDeviceTypeResponse and QueryLightSourceTypeResponse in dali.gear.general to also be subclasses of dali.command.EnumResponse.

Bitmap responses

Although it isn't perfect, I'd just make use of dali.command.BitmapResponse as originally intended for now. If we ever change the way that it works, it should be possible to do so in a way that doesn't break applications making use of the attributes on the response that correspond to the bits.

Instance event filter

Can the instance event filter type be a subclass of enum.IntFlag? Possibly with an extra attribute that allows the number of bits in it to be discovered easily? (So create a dali.device.general.InstanceEventFilter class that's a subclass of enum.IntFlag and adds this attribute, and then dali.device.{instancetype}.InstanceEventFilter for each supported instance type.)

Instance type specific commands and features

Ah, good. Ok, let's keep it simple and remove all the alias_instance_type mechanism for now, and only revisit this if the standard eventually defines commands that would otherwise clash.

dali.device.general.Event class

I think the Event class should inherit from command.Command directly and not be a subclass of _DeviceCommand. Firstly because it is not "A command addressed to a control device" as the _DeviceCommand docstring states, and secondly so _DeviceCommand.from_frame() can check bit 16 and exit immediately if it is clear.

I think UnknownEvent should be a subclass of Event, too, so it can inherit the various properties for things that have already been decoded. You'll just need to special-case it in _register_subclass().

[sl-wallace]

I think I've fixed things up based on what we've discussed here, but I'll check over things again tomorrow before marking the PR as "ready" :smile:

[sde1000]

Looks good on my first read-through!

The only potential issue I've spotted is that we're going to end up with a circular import once support for devices is added to the existing sequences in dali.sequences. (dali.sequences imports dali.device.general which imports dali.sequences)

Is there another reasonable location for the DeviceInstanceTypeMapper implementation to avoid this?

I suppose it could live in dali.device.sequences but that feels a bit odd. Perhaps in its own module?

[sl-wallace]

Is there another reasonable location for the DeviceInstanceTypeMapper implementation to avoid this?

Ah yes, good pick up. I might make something like dali.helpers? I also had another little function, check_bad_rsp(), which I ended up copy-pasting from DeviceInstanceTypeMapper to use in sequences but couldn't import because of the circular dependency. I might also have a look through and see what else might be reasonable to put into helpers, I think there will be other little things which I defined inside the module I needed them in but actually could be reused elsewhere.

[sl-wallace]

OK, I refactored to create a dali.helpers module, which for now just has the DeviceInstanceTypeMapper class and the check_bad_rsp function. I can imagine there might be more in the future that could go into the module as well.

I didn't try implementing it yet, but it seems like the sequences module could now properly use both control gear and control devices, e.g. for querying groups or setting addresses.

I also made a slight tweak to the QueryEventFilters sequence, it can now take either an InstanceEventFilter type, or a bare module - the code then checks if that module has an InstanceEventFilter definition. This means that the sequence can be run with an argument like filter_type=dali.device.pushbutton as well as filter_type=dali.device.pushbutton.InstanceEventFilter. I think the former type is more readable for code that directly knows the type being queried, whereas the latter is more useful when perhaps iterating over a collection of known devices in some higher-level code - it could be a use for the enabled_by attribute, something like filter_type=some_event_instance.enabled_by.__class__.

[sl-wallace]

Just added a few more examples to async-serial-luba.py, to show things like setting the timers for pushbutton instances. I think it's good to go now, let me know what you think :smile:

[sde1000]

Oh, I hadn't thought about putting it in a top-level module, rather than one under dali.device!

I think it makes sense — DeviceInstanceTypeMapper isn't directly part of the standard, it's just something that's useful to have as a consequence of how the standard is written. (Arguably dali.sequences is as well. Hmm. Although DeviceInstanceTypeMapper isn't just a sequence.)

I could imagine putting code that calculates fade times, etc. in dali.helpers as well — useful to have, and not just a sequence. Let's stick with it.

Could you document the intended behaviour of check_bad_rsp() and add tests for it? The helpers module should be mentioned in the README too. After that, I think it's all ready to merge! Thank you for all your work on this, it's a major addition to the project and long-overdue.

[sl-wallace]

I hadn't thought about putting it in a top-level module, rather than one under dali.device!

Maybe for now I could put it under dali.device.helpers? Since, at least for now, it doesn't have anything to do with control gear at all. Perhaps if there was a need for it, e.g. fade time helpers, there could be a similar dali.gear.helpers?

Thinking about it some more, the check_bad_rsp() function probably could fit into dali.device.sequences, since so far it's only used in either the sequences themselves, or in the autodiscover sequence of DeviceInstanceTypeMapper. I suppose it could be useful to import it in higher-level applications, in which case helpers might make more sense than sequences, but it's not really like it matters much.

---

Could you document the intended behaviour of check_bad_rsp() and add tests for it? The helpers module should be mentioned in the README too.

Yep sure thing, I'll get that sorted out.

[sde1000]

If you would prefer to reorganise as you described, you could make check_bad_rsp() just be an internal function, stick a "_" in front of it, and call it a day... I'm happy either way. We only need to nail down its behaviour with documentation and tests if it's expected to be used by higher level applications.

(Testing for such a trivial function: obviously it works as you expect at the moment, it's really just to point out if any of the assumptions change in the future, eg. if someone decided to add a __bool__ method to Frame.)

[sl-wallace]

Thinking about it, I reckon that check_bad_rsp() could be useful to higher level applications, so I've added tests and docstrings for it, as well as mentioning helpers in the README. I also moved helpers to dali.device instead of top-level, just to avoid possible confusion about DeviceInstanceTypeMapper, since it's not at all relevant for control gear and I'm guessing the majority of users of python-dali won't be interested in control devices.