Output of busylight supported not accurate

[henryruhs]

Software Versions:

• Operating System: MacOs
• Python version: 3.8
• BusyLight version: 0.22.2

General Type of Problem

• [ ] Installation
• [x] Command-line
• [ ] Integrating into another project
• [ ] Web API
• [ ] other

Describe the Problem busylight supported will output

Agile Innovative
  - BlinkStick
Compulab
  - fit-statUSB
Embrava
  - Blynclight
  - Blynclight Mini
  - Blynclight Plus
Kuando
  - Busylight Alpha
  - Busylight Omega
Luxafor
  - Orb
MuteMe
  - MuteMe Mini
  - MuteMe Original
  - MuteMe Original (prototype)
MuteSync
  - MuteSync Button
Plantronics
  - Status Indicator
ThingM
  - Blink(1)

Expected Behavior MuteMe seems to be duplicated and Luxafor is missing Flag and Orb.?

[henryruhs]

I figured out the issue, you seem to utilize the supported_device_ids() to generate the supported list, but Luxafor does not have a base class that contains all devices. There is one class for Flag, Orb and Mute - this does not follow internal project convention as the Kuando Busylight base class does this different.

For developer that integrate your lib, the base class approach is more pleasant. Otherwise there is the need to implement each sub class to cover all devices.

Suggested code:

class Luxafor(HIDLight):
    @staticmethod
    def supported_devices() -> Dict[Tuple[int, int], str]:
        return {
            (0x4D8, 0xF372): "Flag",
            (0x4D8, 0xF372): "Mute",
            (0x4D8, 0xF372): "Orb",
        }

I would like to recommend to rename it to supported_devices (this is more like a collection of product_id and vendor_id rather than a device_id) in all classes. Why are the product ids equal? This looks like an typo or did Luxafor screw it up? Can you combine the Mute only methods into the base class like you did this within the muteme base class?

[JnyJny]

I'm sorry, but your proposed solution will not work and the way support for the Luxafor devices is written is intentional.

As you identified, all three devices have the same vendor and product identifier, so the dictionary you propose will devolve from three initial entries to just one:

>>> {
...             (0x4D8, 0xF372): "Flag",
...             (0x4D8, 0xF372): "Mute",
...             (0x4D8, 0xF372): "Orb",
...         }
{(1240, 62322): 'Orb'}

There is a bug here to be sure. I found it in the Light.supported_lights class method, which is fixed with commit d977d7d275ca52539718ca445677b2b96b284c10. I need to sort out some of the dependabot PRs before publishing a release to PyPI.

[henryruhs]

Thanks for the explanation.

Why don't we change the dict to this?

(0, 0x4D8, 0xF372): "Flag",
(1, 0x4D8, 0xF372): "Mute",
(2, 0x4D8, 0xF372): "Orb",

My guess is, that you had troubles to identify what type of Luxafor is plugged in and therefore are forced to control it by user input -> use different classes!? Correct?

[JnyJny]

The claims class method is the key here. For most Light subclasses, it is enough to discover the vendor/product identifier tuple in the supported_device_ids dictionary. The Luxafor family of devices have a slightly different implementation of claims which allows all the subclasses to claim the correct light based on it's known product_string value. The API remains consistent without having to modify the data structures.

[henryruhs]

Could feature detection be an option to identify devices? I saw this guy looking for the mute state.

It would be great to have one base class that returns all types via all_lights(), otherwise I need to implement 3 different consumer in my project or introduce some ugly try catch.

from busylight.lights import NoLightsFound
from busylight.lights.luxafor import Flag, Orb, Mute
# we need that
# from busylight.lights.luxafor import Luxafor

try:
    Flag.first_light().is_pluggedin()
    api = Flag
try:
    Flag.first_light().is_pluggedin()
    api = Orb
try:
    Flag.first_light().is_pluggedin()
    api = Mute
except NoLightsFound:
    logger.error(wording.get('connection_not_found').format('luxafor_light'))
    sys.exit()
return api

[JnyJny]

Your code example seems to be actively ignoring the niceties I specifically engineered in to the Light class and its subclasses. Are you just supporting Luxafor lights in your application?

The available_lights() class method can tell you what lights are available without having to know a priori what lights might be attached.

from busylight.lights import Light
from busylight.lights import NoLightsFound

if not Light.available_lights():
    """ no lights are available """

first_light = Light.first_light() # first Light subclass discovered

lights = Light.all_lights() # a list of Light subclasses discovered, might be empty

for light in lights:
     assert light.is_pluggedin

Aliasing Light subclasses to api seems counterproductive to me.

[henryruhs]

It was not my intention to ignore this API, but I need to call individual lights by it's vendor or let's say series of products. However, I appreciate the time you spend to answer all my questions. This project is really amazing!

Closing the issue, sorry for the off-topic to everyone scrolling down to this point.

[JnyJny]

I experimented with adding a vendor-specific Light subclass, which could be used like:

from busylight.lights.luxafor import Luxafor

all_luxafor_lights = Luxafor.all_lights()

Adding this to the Light subclass hierarchy is relatively simple, however it would restrict each vendor to a specific light implementation HIDLight or SerialLight. While none of the current crop of vendors offer lights with different I/O implementations, it's not impossible for that to happen in the future.

The same behavior could be duplicated by filtering the results of all_lights by vendor and would be more future proof:

from busylight.lights import Light

all_luxafor_lights = [light for light in Lights.all_lights() if light.vendor() == 'Luxafor']

[henryruhs]

Our discussion revealed a couple of structural issue that happend to me in Chroma Feedback aswell. Therefore I have made the decision to resort my code by vendor directories as you did, let's analyze the current directories and files:

For Kuando Busylight we have VENDOR/PRODUCT-LINE.py:

vendor: Kuando
product line: Busylight
product: Busylight Alpha

For Luxafor Flag we have VENDOR/PRODUCT.py:

vendor: Luxafor
product line: (None)
product: Flag

As you might agree, the API should always follow a convention and here we have some kind of a mix as the Busylight class represents a product line (collection of products) and the Flag class a single product.

Don't you think it makes sense to provide a VENDOR/VENDOR.py that represents a collection of products and make PRODUCT_LINE.py and PRODUCT.py private?

[JnyJny]

The "structure" of the current Light class hierarchy is based first on the hardware hierarchy (HID or serial) and then by device capability. The vendor/product family aspects of each light is a descriptor but not part of the class hierarchy. I admit that there are inconsistencies in the current structure, driven mostly by the differences in each vendor's offerings. This is something I'll have to think about more before acting.

[JnyJny]

The harmonize branch has a re-working of the Light subclass hierarchy that does not take into account vendor or product line. Instead of busylight.lights.embrava.Blynclight providing support for three different lights, there is now:

• busylight.lights.embrava.Blynclight
• busylight.lights.embrava.Blynclight_Mini
• busylight.lights.embrava.Blynclight_Plus

A similar treatment was given to the Kuando and MuteMe devices.

While I'm pretty sure this doesn't address your use case, it does regularize the class structure (the Blinkstick classes need more work, which is forthcoming).

[henryruhs]

To be honest, this went into the opposite direction that was intended from my side. Still I like to see that you normalize the codebase, this will help my integration to be more straight forward. I merged the normalized rework to Chroma Feedback's master and cannot wait for you to get your harmonize branch finished.

[JnyJny]

I've merged the harmonize branch to master and pushed a new minor release. I appreciate your feedback.

[henryruhs]

I check it out tomorrow, thanks.