Skip to content

Philips-hue batched command handling support #2075

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Apr 25, 2025
Merged

Conversation

NoahCornell
Copy link
Contributor

@NoahCornell NoahCornell commented Apr 15, 2025

Check all that apply

Type of Change

  • WWST Certification Request
    • If this is your first time contributing code:
      • I have reviewed the README.md file
      • I have reviewed the CODE_OF_CONDUCT.md file
      • I have signed the CLA
    • I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
  • Bug fix
  • New feature
  • Refactor

Checklist

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have verified my changes by testing with a device or have communicated a plan for testing
  • I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

This is adding batched command handling support for the philips-hue driver by taking advantage of hue's rooms and zones. The group is interacted with via the grouped_light service which supports all of the same APIs that we use to control devices via the light service.

The batched commands are ingested into the driver by overriding the top level capability handler in the driver. This is overridden if the hub has support for the receive_batch function on the capability channel socket. Commands that are handled individually are sent to the default handler while commands that are determined to be a match for a group are sent via the new grouped light command handlers.

Commit by commit:

  1. The first commit is adding the rooms/zones management into the driver. On connect of the SSE stream, the driver will request all of the rooms and zones from the bridge and put them in a device field GROUPS on the bridge device. As CUD events flow through the SSE stream, the driver will handle these and update the bridge device groups accordingly. Groups are augmented with a list of device records and the grouped_light resource ID when ingested so that time can be saved when processing a command batch. The groups are ordered by most to least device records when added to the list of groups.
  2. The second commit is adding the command handlers for the grouped light commands. These should be similar to the normal command handlers. One difference of note, there is a concept of "auxiliary" data associated with the command. This is data that is device specific that must match each of the devices for the command to be handled in a group e.g. gamut for color control.
  3. This commit is adding some util for tracking items in a KV pair table. This was needed for processing the command batch and made things a little easier to not have to manually track all the items.
  4. This is just adding a quiet param to get_hue_bridge_for_device which was very verbose when processing the batched command.
  5. This is the actual processing of the batched command. The batch is first sorted by bridge, command name, and matching arguments/auxiliary data. Then for each matching command, the groups are iterated over to find group(s) that match those commands. Any matching groups are sent the grouped light commands and any remaining commands are sent to the default capability handler.

Summary of Completed Tests

Tested on my hue setup with 1 bridge and 10 bulbs. 3 of which are full color, 1 color temperature, and the other 6 plain dimmable white.

I have 1 room with all the bulbs, a zone for the color bulbs, a zone for the color temp (color + 1) bulbs, and a zone for the rest minus 1.

I have 2 rules for each capability to change states: on/off, level 10%/100%, color red/blue, temp warm/cold where only the relevant devices are in those rules.

I also have a rule for 9 of the 10 to run which covers the 4 color temp supported bulbs and 5 of the white bulbs to testing going to multiple zones in one batch

This is mainly what I have been using for testing but I have been trying other random permutations as well.

Copy link

github-actions bot commented Apr 15, 2025

Channel deleted.

Copy link

github-actions bot commented Apr 15, 2025

Test Results

   67 files    426 suites   0s ⏱️
2 182 tests 2 182 ✅ 0 💤 0 ❌
3 718 runs  3 718 ✅ 0 💤 0 ❌

Results for commit 90da94a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Apr 15, 2025

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 90da94a

if not grouped_light_id then
log.error(string.format("Couldn't find grouped light id for group %s",
group.id or "unknown group id"))
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we fail this call does it fall back to unicast commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not right now, but it could be added. In the case of grouped_light_id being nil, this shouldn't be possible because we clear out the device table if it isn't on the group response. This is a remnant of when I was iterating through the services in the commands and wasn't saving the service off on group intake.

@NoahCornell NoahCornell force-pushed the hue_jiffy_pop branch 2 times, most recently from 5a2e335 to 677455e Compare April 15, 2025 18:55
@@ -4,6 +4,7 @@ local HueDeviceTypes = {
BUTTON = "button",
CONTACT = "contact",
DEVICE_POWER = "device_power",
GROUPED_LIGHT = "grouped_light",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make anything go wonky during disco? I can't remember how smart the checks around this table actually are 😬

Copy link
Contributor Author

@NoahCornell NoahCornell Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is based off PrimaryDeviceTypes below on line 22 but I will take a closer look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this could be an issue with is_valid_device_type so I think I will remove it just to be safe.

Copy link
Contributor

@dljsjr dljsjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, I'll try to test this on my home setup tomorrow morning.

Copy link
Contributor

@cjswedes cjswedes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we definitely need the confidence of some manual testing.

@NoahCornell
Copy link
Contributor Author

LGTM, but we definitely need the confidence of some manual testing.

Sorry just updated description with my testing. We are also working with thinktank to get some early testing.

if queue == nil then
local tx, rx = cosock.channel.new()
-- Set timeout to 30 seconds to allow for other queued scans to come in.
rx:settimeout(30)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an odd way to achieve this from my perspective. I would probably just use https://github.ecodesamsung.com/iot-hub/scripting-engine/blob/main/lua_libs/st/driver.lua#L112 and store the timer in the GROUPS_SCAN_QUEUE field, then cancel and restart the timer if it's not nil when this is called. I don't think this approach is wrong, so you don't need to change it, I just think it's a bit counter intuitive to set a timeout and depend on it being hit on receive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. I forgot about that function to be honest. One slight benefit here is that we don't have to recreate the closure and new timer each time this is called which could add up with if we receive a lot of added events but I doubt it matters all that much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree there are some benefits of doing it this way, and given that you've built and tested it I don't see a need to change at this point.

@NoahCornell
Copy link
Contributor Author

ThinkTank testings results: https://smartthings.atlassian.net/wiki/spaces/HUBPLT/pages/3890250154/2025-04-16+Hue+Jiffy+Pop+Testing+Results

I did a few more onboarding and general testing this morning and feeling good so I am going to merge this.

@NoahCornell NoahCornell merged commit c06fcbc into main Apr 25, 2025
11 checks passed
@NoahCornell NoahCornell deleted the hue_jiffy_pop branch April 25, 2025 12:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants