Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(mqtt-ad): expose Scene Activation CC #3580

Closed
wants to merge 3 commits into from

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Feb 3, 2024

as a number

fixes #3570

Tested against openHAB and Home Assistant.

@coveralls
Copy link

coveralls commented Feb 3, 2024

Pull Request Test Coverage Report for Build 7787094463

  • -10 of 20 (50.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 22.106%

Changes Missing Coverage Covered Lines Changed/Added Lines %
api/lib/Gateway.ts 0 10 0.0%
Totals Coverage Status
Change from base Build 7784686582: 0.03%
Covered Lines: 3773
Relevant Lines: 18144

💛 - Coveralls

@ccutrer ccutrer changed the title expose Scene Activation CC to MQTT Auto Discovery fix(mqtt-ad): expose Scene Activation CC Feb 3, 2024
api/lib/Gateway.ts Outdated Show resolved Hide resolved
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

@ccutrer Couldn't you simply revert the previous change about this?

@apella12
Copy link
Contributor

apella12 commented Feb 5, 2024

FYI I can't help on the coding and not overly familiar with these CCs, but will comment that scene activation and scene control CCs both launch scenes and probably should be setup the same. The difference is that scene control is via the devices' Association groups and scene activation is via the lifeline to the controller. From the specs.

2.2.85 Scene Controller Configuration Command Class, version 1
The Scene Controller Configuration Command Class is used to configure nodes launching scenes using
their association groups
2.2.85.2 Scene Controller Configuration Set Command
This command is used to configure settings for a given physical item on the device.
Bytes:
Command Class = COMMAND_CLASS_SCENE_CONTROLLER_CONF
Command = SCENE_CONTROLLER_CONF_SET
Group ID
Scene ID
Dimming Duration
------
2.2.83 Scene Activation Command Class, version 1
The Scene Activation Command Class used for launching scenes in a number of actuator nodes e.g.
another scene-controlling unit, a multilevel switch, a binary switch etc.
2.2.83.2 Scene Activation Set Command
This command is used to activate the setting associated to the scene ID.
Bytes:
Command Class = COMMAND_CLASS_SCENE_ACTIVATION
Command = SCENE_ACTIVATION_SET
Scene ID
Dimming Duration
------

On an unrelated note, the openHab Zwave binding supports either option.

@ccutrer
Copy link
Contributor Author

ccutrer commented Feb 5, 2024

@ccutrer Couldn't you simply revert the previous change about this?

As discussed in #3570 (comment), it was broken before, so there's no point in simply reverting.

@apella12: keep in mind the CC discussed here is Scene Activation. The PR that removed Scene Activation from being exposed (for not actually working) was addressing Central Scene CC. Scene Controller Configuration is a separate CC. I'm not sure if any of my devices support that CC. They do support Scene Actuator Configuration. If Scene Controller Configuration is important to you, we could likely expose it via auto discovery easily enough. I'd just need some examples.

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM, I would like if possible if also @kpine can do a review

Copy link
Contributor

@kpine kpine left a comment

Choose a reason for hiding this comment

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

It fixes Scene Activation, but it's still a breaking change.

@@ -245,6 +246,15 @@ const configurations: Record<HassDeviceKey, HassDevice> = {
entity_category: 'config',
},
},

scene_activation: {
type: 'number',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type: 'number',
type: 'sensor',

Changing the entity type to number is still a breaking change.

@ccutrer
Copy link
Contributor Author

ccutrer commented Feb 5, 2024

I don't see how fixing something that couldn't have worked is a breaking change. It's a bug fix.

@kpine
Copy link
Contributor

kpine commented Feb 5, 2024

I don't see how fixing something that couldn't have worked is a breaking change. It's a bug fix.

In what way was it not working before? #3570 (comment)

@ccutrer
Copy link
Contributor Author

ccutrer commented Feb 5, 2024

The entire point of Scene Activation CC is to send a command to the device to trigger a scene. A sensor component is read only. You could read the last scene that was manually set on the device, but not actually send a scene command to it (at least when using Home Assistant or openHAB).

@kpine
Copy link
Contributor

kpine commented Feb 5, 2024

Devices, like portable controllers send Scene Activation Set commands to the controller (or end nodes). That is the issue here.

So Scene Activation has dual usage depending on the target.

@ccutrer
Copy link
Contributor Author

ccutrer commented Feb 5, 2024

Ahhhh, now that is a critical piece of information I was missing. My devices don't do that, and I didn't even know it was possible. My devices all send button presses to the controller with Central Scene commands, and when talking directly with each other use Basic Set or Multilevel Set commands, but don't communicate that to the controller when they do so. The only purpose for Scene Activation is to tell the switch to activate to a particular preset (and you could use a multicast to the same scene to several switches, which each have their own presets for a given scene).

So are you saying some devices will update the controller with their current Scene Activation value, and that value can be triggered locally (like say if scene 1 is dimming to 50%, when you set to 50% it will update the controller and say "I'm now at scene 1")? Or are you saying "pressing the top left button on a device with 6 buttons will send a Scene Activation value of 1 to the controller"? Or are you saying "I can configure my switch to directly control another switch, and when I use that button on the switch, on or both of those switches will notify the controller that this happened via the Scene Activation value"?

In that case, not only is the component type is a breaking change, but also the component ID is a breaking change. In order to maintain compatibility, it will still need to publish a Scene Activation CC under the ID the Central Scene component uses.

@ccutrer
Copy link
Contributor Author

ccutrer commented Feb 5, 2024

Closed in favor of a full revert: #3583

@ccutrer ccutrer closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scene Activation CC (43) support with MQTT Discovery
5 participants