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

prepend value labels with a instance name #695

Closed
Fishwaldo opened this issue Nov 16, 2015 · 20 comments
Closed

prepend value labels with a instance name #695

Fishwaldo opened this issue Nov 16, 2015 · 20 comments
Assignees
Milestone

Comments

@Fishwaldo
Copy link
Member

Allow us to specify instance names in the config files, and then prepend value labels with the instance name, so you can have things like:
"Outlet Level" and "LED Level" on things like the Aeotec Smart Dimmer 6 (instance 1 is the outlet, instance 2 is the LED ring)

@Fishwaldo Fishwaldo self-assigned this Nov 16, 2015
@nechry
Copy link
Member

nechry commented Dec 1, 2015

nice feature I recently include a Aeotec Smart Dimmer 6 and yes it's cant be nice to have this information. Is also the reason we see on Aeotec Smart Switch 6 a SwitchMultiLevel CC, at beginning I'm thinking about a wrong interview but it's also the LED ring.

@Fishwaldo Fishwaldo added this to the Version 1.6 milestone May 30, 2017
@vincemic
Copy link
Contributor

I'd like to take on the challenge of adding the enhancement. I would like to possibly add a new XML element under Product in device_configuration.xsd. Not sure what it should be called. It would have an id, name, and a description of the "subdevice" or "endpoint". Then each value could have a subdeviceid reference. This would be helpful for power strips that have multiple binary switches and meters. In the case of the aeotec power strip, it would have 5 subdevices. The first subdevice name would be "Main Power", the others would be "Socket 1", "Socket 2".

Figuring out the XSD is the first part. Then how would you want the API to change to retrieve the additional data. The simple way may be to augment the value to bring back the whole subdevice object if it has a reference. This would make the change backwards compatible since it could be ignored.

Let me know if you already had a solution in mind.

@Fishwaldo
Copy link
Member Author

Hi,
Firstly - Please base this against the Dev Branch.

For the XSD - Way I see it is options contained in a <CommandClass id="96"> block ( The MultiInstance CommandClass Entry). Formatted something like what you described:

<Instance id="1" label="Switch 1">
    <label lang="fr">commutateur 1</lang>
</instance>

Not sure about a description field.

Expose a Manager Method to get (and set?) the Instance Name for each ValueID (something like):

Manager::GetValueInstanceName(ValueID const& _id, int32 _pos = 1);
Manager::SetValueInstanceName(ValueID const& _id, string const& name, int32 _pos = 1);

Lets store the Instance Names variables in the MultiInstance CC

Also - Add defaults for each instance (upto 256 instances from memory) so if no Instance Name is present in the config file, we still can output something other than blanks :)

Finally - I'm 50/50 on this option, but thinking that we should make a option to update each Value Label with the instance name... basically concat the two together. so a "Switch" label becomes "Instance 1 Switch" (that will ease the transition for apps). So maybe a option.xml flag?

The Dev Branch has localization. When updating the Labels, please update them during the Manager::GetValueLabel* calls (or as close as possible to that call chain), and do not save the instance name to the actual ValueID (by updating the label etc).
And you need to add the Default "English" names to Localization.xml file. (The English versions are only there to help translators, but lets be complete).

As to "how to translate" the Instance Names, the Value Labels or Value Helps code should be a good example.

(remember to save the Instance Names in the MultiInstance::WriteXML as well, so our cache has them as well...)

Sounds like a lot, but should be straight forward.

Thanks for your help on this.

@vincemic
Copy link
Contributor

Ok, makes sense. There is one problem though. I would like to associate more than one command instance to a "subdevice". For the Aeotech dimmer socket, there is a night light that has at least two command class instances associated to it. Multilevel Switch and Color. Both would affect the night light only and if I change the name "Night Light" to something else, I would want both command instances to share the change. To complicate things, The multilevel switch for the night light is instance 2 and the color command for the night light is instance 1.

If we had an object that was "subdeivce" or some better name, we could name it "Night Light" and assign it one or more command instances.

When presenting these objects in a UI or in voice commands, I would have the application layer show two virtual devices showing as lights or light switches. One would be "Aeotec Dimmer" the other would be "Night Light". You can rename either independently.

@Fishwaldo
Copy link
Member Author

I think you can still do that with two <instance> blocks. Nothing says we can't have duplicate names for each instance.

<Instance id="2" label="Night Light">
    <label lang="fr">veilleuse</lang>
</instance>
<Instance id="3" label="Night Light">
    <label lang="fr">veilleuse</lang>
</instance>

@vincemic
Copy link
Contributor

Ok, would the API try to preserve the instance names that are the same on save? Or would that be up to the application?

@Fishwaldo
Copy link
Member Author

I think its ok to save them...

@vincemic
Copy link
Contributor

So this may be a bug, not sure. OZW knows with the MultiChannelEndPointReport that the endpoints are not the same for the Aeotec Dimmer 6. Not sure if instances and endpoints are meant to be the same thing. Is OZW stacking command instances and giving them an index in arrival order. If instance id was the same as endpoint id then it might be possible for the Color command class to have a instance 2 but no instance 1. Still learning the code and terminology.

@vincemic
Copy link
Contributor

vincemic commented Jan 30, 2019

If the class instance's ids where mapped correctly by endpoint then the solution for instance labels would work perfectly. This issue is down to HandleMultiChannelEncap. Looking further.
OZW_Log.txt

@vincemic
Copy link
Contributor

vincemic commented Jan 30, 2019

Adding log with additional log around endpoint and instance mapping. Watch the color command class
OZW_Log.txt

@vincemic
Copy link
Contributor

vincemic commented Jan 31, 2019

After looking through the code, I see that it was specifically designed to allow the id of an instance to stray from the associated endpoint id. I think that changing the way instances work will possibly break bindings end users have already setup.

I would like to offer this solution. I would like to expose endpoints as a first class citizen. In configuration you would see the following

<endpoint id="1" label="Socket">
    <label lang="fr">prise</lang>
</endpoint>
<endpoint id="2" label="Night Light">
    <label lang="fr">veilleuse</lang>
</endpoint>

But in order to present things correctly we will need to expose endpoints through the manager. First I would like to augment the ValueID to include endpoint id. Endpoint id shouldn't change the m_id packing or the m_id2 packing. Then add the methods for endpoint.

Manager::GetEndpointName(uint32 const _homeId. uint8 const _nodeId, uint8 const _endpointId, int32 _pos = 1);
Manager::SetEndpointName(uint32 const _homeId. uint8 const _nodeId, uint8 const _endpointId,  string const& name, int32 _pos = 1);

This would allow applications that are endpoint aware to associate ValueID to endpoints and give those endpoints a descriptive name.

@Fishwaldo
Copy link
Member Author

So this may be a bug, not sure. OZW knows with the MultiChannelEndPointReport that the endpoints are not the same for the Aeotec Dimmer 6. Not sure if instances and endpoints are meant to be the same thing. Is OZW stacking command instances and giving them an index in arrival order. If instance id was the same as endpoint id then it might be possible for the Color command class to have a instance 2 but no instance 1. Still learning the code and terminology.

instances and endpoints are essentially representing the same thing, except, instance 1 is always the root node, and endpoint 1 is the first node under the root.

The instances are "stored" against valueID's and not command classes. So one CommandClass on a Node can work with multiple instances/endpoints.

So in your case, you would have a (few) ValueID that are of the User Genre for the ColorCC on Instance 2, but not on Instance 1 (if you do have instance 1, thats a bug).

@Fishwaldo
Copy link
Member Author

Adding log with additional log around endpoint and instance mapping. Watch the color command class
OZW_Log.txt

Just a FYI - I'd do your development with no security. This is due to WIP on #1639 which is related to MultiInstance issues.

@Fishwaldo
Copy link
Member Author

After looking through the code, I see that it was specifically designed to allow the id of an instance to stray from the associated endpoint id. I think that changing the way instances work will possibly break bindings end users have already setup.

I would like to offer this solution. I would like to expose endpoints as a first class citizen. In configuration you would see the following

<endpoint id="1" label="Socket">
    <label lang="fr">prise</lang>
</endpoint>
<endpoint id="2" label="Night Light">
    <label lang="fr">veilleuse</lang>
</endpoint>

But in order to present things correctly we will need to expose endpoints through the manager. First I would like to augment the ValueID to include endpoint id. Endpoint id shouldn't change the m_id packing or the m_id2 packing. Then add the methods for endpoint.

Manager::GetEndpointName(uint32 const _homeId. uint8 const _nodeId, uint8 const _endpointId, int32 _pos = 1);
Manager::SetEndpointName(uint32 const _homeId. uint8 const _nodeId, uint8 const _endpointId,  string const& name, int32 _pos = 1);

This would allow applications that are endpoint aware to associate ValueID to endpoints and give those endpoints a descriptive name.

How do they stray? as mentioned above, instances are the same as endpoints, its just 0 based versus 1 based counting thats makes them different.

as Instances = Endpoints, I don't see how this helps you. Your still going to have dynamic endpoints with different CC's.

I'm lost as far as what you are trying to show in the logs. Can you expand on what you are seeing?

@Fishwaldo
Copy link
Member Author

Ok. I can see where you might be confused with the Endpoint->Instance Mapping code in the CommandClass class. Thats pretty much dead redundant code though from OZW's inception to handle a few old buggy devices.

Maybe one day I'll just go through that code and drop the whole notion of endpoint->instance maps and just do "instance = endpoint +1"

@vincemic
Copy link
Contributor

vincemic commented Feb 1, 2019

Sorry for the wall of text,

Things I have found so far....

  • Instances start at 1

  • There is a new instance for each CC instance found

  • Each CC can have a different number of instances and they are not related to each other

  • Endpoints start at 1

  • There are as many endpoints as the node reports

  • Endpoints can have totally different CCs and amount of CCs

For Devices that support MultiChannelCapabilityReport that have endpoints that have the same CCs, everything is good

-<CommandClass version="1" name="COMMAND_CLASS_BASIC" id="32" request_flags="5" mapping="38">
<Instance index="1" endpoint="1"/>
<Instance index="2" endpoint="2"/>
</CommandClass>
-<CommandClass version="2" name="COMMAND_CLASS_SWITCH_MULTILEVEL" id="38" request_flags="1" issecured="true">
<Instance index="1" endpoint="1"/>
<Instance index="2" endpoint="2"/>

But when the endpoints are not matching and the second endpoint has additional CCs

-<CommandClass version="1" name="COMMAND_CLASS_COLOR" id="51" request_flags="3" issecured="true" colorchannels="28">
<Instance index="1" endpoint="2"/>

COMMAND_CLASS_SWITCH_MULTILEVEL instance 2 and COMMAND_CLASS_COLOR instance 1 are the same endpoint on the same device. If I wanted give endpoint 2 the same label across all the CCs, I would have to do it by hand and copy the label and its language entries across all the supported CC instances.

A better way would be to add a endpoint XML element in the device config to allow for the labeling of endpoints. Then add manager code and node code to access that element. Then the endpoint label, description, help and its language entries would be all in one spot.

Not to confuse things, but I did modify the code to lock instance and endpoint together for MultiChannelCapabilityReport and got this

<CommandClass id="38" name="COMMAND_CLASS_SWITCH_MULTILEVEL" version="2" request_flags="1" issecured="true">
				<Instance index="1" endpoint="1" />
				<Instance index="2" endpoint="2" />
<CommandClass id="51" name="COMMAND_CLASS_COLOR" version="1" request_flags="3" issecured="true" colorchannels="28">
				<Instance index="2" endpoint="2" />

The change above doesn't help me label endpoints efficiently and it may break bindings people are depending on. I also had to remove a bunch of code in MultiInstance::HandleMultiChannelCapabilityReport in order to lock the instance and the endpoint. I am sure its in there to support low level devices, so taking it out may be a bad idea. But... it does help one thing. Endpoint information is not exposed anywhere that I can find. ValueID does not have endpoint information. Value does not have endpoint information. But if I lock instance and endpoint together, then instance is endpoint and ValueID and Value classes would expose endpoint as Instance.

May question is this now.
Will you accept a PR that has a new optional XML element for Endpoint that has a label and supports language.

The PR will include one of the following
Option 1: Lock instance number with endpoint number // easy
Option 2: Add endpoint information to ValueID and Value classes to be expose through the manager. // Harder

PS: I do think that this is strongly related to #1639

@Fishwaldo
Copy link
Member Author

Hi,
Thanks for sticking with this...

So firstly - I dont like option 2. Instances and Endpoints are meant to represent the same thing. We should not expose them when their purpose is the same. (and confuse users etc).

Non-identical Endpoint Devices are few and far between, so your one of the lucky ones that gets to deal with this.. All the memories of WTF are starting to flood back to me now!

What your describing for non-identical Endpoints is a bug. All CommandClasses on the same endpoint should be in the same instance as far as OZW is concerned.

As far as the

<instance>

tag in the config file. I'm dubious of that, and it needs auditing.. because:

Instance 1 in OZW is for CommandClasses on the root Node. Ie, non-encapsulated.
Instance 2 should equal endpoint 1 (meaning its encapsulated)

As for the code that maps between endpoints and instances - I remember now, its because there are some devices that send the endpoint as a bitmask in the Capabilities Report, and some that send it as a integer. At the time when the MultiInstance CC was written, the Z-Wave spec wasn't open, so we were guessing how it was encoded.

What this means - Devices that conform to the spec, send endpoints as 1, 2, 3, 4 etc. Non-Conformant Devices send 1, 2, 4, 8 (eg, bitmask).

Unfortunately they still exist. I just did a quick grep over the logs uploaded to the website in the last 30 days, and I still see "bitmask" devices. sigh
Storing the map of instance->endpoint, we avoid having to create a config file option for buggy devices, and can handle them natively.

(whats annoying is the spec allows bitmasks for sending messages in a MultiChannelEncap message. So one on type of message, its a integer, on another its a choice for the developer to use integer or bitmask!. Nice consistency eh?)

So Locking the instance->endpoint values (with a +1) wont work for these devices, and we have to do a config option for them.. no thanks :)

btw, we use that mapping here:

m_endPoint = _cc->GetEndPoint( _instance );

So - This code for the identical endpoints is dubious -

for( uint8 i = 1; i <= len; i++ )

And clearly this code for non-identical endpoints is wrong:

for( i = 1; i <= 127; i++ )

I remember what I was attempting to do, and that is find the next available free instance number so we were linear regardless if a endpoint was a bitmask or a integer. I can't remember if having Linear instance numbers is a requirement/constraint somewhere though...

what is the model number of the Aeotec device you have? I'll dig mine up and try to fix this bug up as part of the #1639 issue. (or your free to have a go at fixing it!)

@vincemic
Copy link
Contributor

vincemic commented Feb 3, 2019

Aeotec ZW099-A02.

Thank you, I have a much better understanding of the intent of the code. I will look through it and try a bunch of multi-endpoint devices I have. The fix should be straight forward. I'll give it a try in the next few days.

@stiansoevik
Copy link

Regarding bit masks, I read through an app note while working on another issue, which briefly mentioned the rationale for allowing bitmasks. It makes sense to be able to e.g. control all switches simultaneously.

https://www.silabs.com/documents/login/application-notes/APL12955-Z-Wave-Multi-Channel-Basics.pdf

Fishwaldo added a commit that referenced this issue Apr 24, 2019
@Fishwaldo
Copy link
Member Author

So I've done this, and hopefully in a way that addresses your challenge with the endpoint mappings.

If you Specify Instance Labels for the MultiInstance CC (in the config file) they are applied to all Instances/CommandClasses (ie, global). But you may also add Instance Labels to individual CommandClasses as well, to give them different names that the Global Ones.

If No Instances Labels are specified in the Config File, it defaults to "Instance x"

ValueLabels are also prepended with the Instance Label so you can easily identify what Control relates to what instance without having to display the actual instance Number

This may be disabled by Setting "IncludeInstanceLabel" as false in the Options.xml or class.

A example (Global) instance config file: a64edfb#diff-7693ba9a9c6991d55e25c52dbf0df288

(Just add another commandclass block if you want different instance labels for different commandclasses).

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

No branches or pull requests

4 participants