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

ForceInstances enabled by default in Dev branch #1450

Open
Zkral opened this Issue Feb 21, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@Zkral

Zkral commented Feb 21, 2018

Hi,

I'm using the Dev branch and I have some issues with a Fibaro Motion Sensor, most Values are not updated.
It turns out that the associations I added with the controller as target were set with an instance of 1 instead of 0. The code responsible for this change is in the MultiChannelAssociation::Set function:

        /* for Qubino devices, we should always set a Instance if its the ControllerNode, so MultChannelEncap works.  - See Bug #857 */
        if ( ( m_alwaysSetInstance  == true )
                        && ( _instance == 0 )
                        && ( GetDriver()->GetControllerNodeId() == _targetNodeId ) )
        {
                _instance = 0x01;
        }

The problem is that m_alwaysSetInstance = true in my situation, but it should be false, as the ForceInstances setting in the XML file of my node is not set. However the constructor of the MultiChannelAssociation sets it to true by default, and ReadXML sets it to false only if ForceInstances != "false" (which seems weird).

Apparently the logic of this setting was completely reversed by this commit: a5305f0#diff-26e3ab0b3f23a933a6a8580802da0a34
Since then, it is impossible to add an association with instance 0 on most nodes, and I guess what is sent to instance 1 of my controller (Aeotec Z-Stick Gen5) is dropped.

If I revert the changes to the MultiChannelAssociation.cpp file, I can fix the associations on my Fibaro Motion Sensor and then all values are correctly updated.

This change seems obviously wrong to me, and I wonder why it was introduced in this commit about the CentralScene CC.
Note: in the master branch it is still the old code (and so this issue only concerns the Dev branch).

@1mckenna

This comment has been minimized.

1mckenna commented Feb 23, 2018

Wondering if this is what broke the motion detection of the Zooz ZSE40 sensors i have that worked with the non-dev branch.

Zkral do you have a modified build that i can build and see if that resolves my issues as well?

@Zkral

This comment has been minimized.

Zkral commented Feb 23, 2018

Sure, see https://github.com/Zkral/open-zwave/tree/Dev (fix in commit Zkral@2170d0b)

Remember that you will need to change the associations manually.

@1mckenna

This comment has been minimized.

1mckenna commented Feb 23, 2018

What do you mean by manually changing the associations?

I have removed the devices from my ZWave network already since it wasnt working, I assume with this fix it'll associate correctly when i re-add it?

@Zkral

This comment has been minimized.

Zkral commented Feb 23, 2018

Yes

@1mckenna

This comment has been minimized.

1mckenna commented Feb 23, 2018

I'll give it a try tonight/this weekend and report back.

@1mckenna

This comment has been minimized.

1mckenna commented Feb 24, 2018

Looks like my issue with the sensors not reporting motion must be related to something else as that didnt fix my issue

@Fishwaldo

This comment has been minimized.

Member

Fishwaldo commented Mar 7, 2018

The change was made as Instance Based Associations should be the default moving forward. Hence why its "opposite" from the master branch. (but a breaking change, so not ported there).

We probably need to update the config files where forceInstances is enabled to remove that line (and only use forceInstances="false" on devices that don't support instance based associations)

If I revert the changes to the MultiChannelAssociation.cpp file, I can fix the associations on my Fibaro Motion Sensor and then all values are correctly updated.

If you revert the changes - Is its instance based association, or the old Node Based Association?

@Fishwaldo Fishwaldo self-assigned this Mar 7, 2018

@Zkral

This comment has been minimized.

Zkral commented Mar 7, 2018

I don't see what the benefits of forcing the instance by default are, but If you want to disable it by setting ForceInstances="false", this line in MultiChannelAssociation::ReadXML is incorrect:
m_alwaysSetInstance = !strcmp( str, "false");
if str = "false", then strcmp(str, "false") == 0, so m_alwaysSetInstance = 1;

I'm not sure I understand your question. I haven't changed the code in MultiChannelAssociation::Set, and so with _instance set to 0, this is what is sent:
Msg* msg = new Msg( "MultiChannelAssociationCmd_Set", GetNodeId(), REQUEST, FUNC_ID_ZW_SEND_DATA, true );
msg->Append( GetNodeId() );
msg->Append( 4 );
msg->Append( GetCommandClassId() );
msg->Append( MultiChannelAssociationCmd_Set );
msg->Append( _groupIdx );
msg->Append( _targetNodeId );
msg->Append( GetDriver()->GetTransmitOptions() );
GetDriver()->SendMsg( msg, Driver::MsgQueue_Send );

The instance doesn't appear in the message sent, and considering SDS13782 Z-Wave Management Command Class Specification, I would say it is an old non multi-channel association.

Looking back, the behavior of my FGMS001-ZW5 Motion Sensor (manufacturer id 0x010f / product id 0x1001) was actually kind of weird when the associations were set with instance = 1. If I remember correctly, I did receive the motion sensors events, but not the temperature reports (I don't think those are supposed to be influenced by association groups). I will have to test to confirm this.

@no-response

This comment has been minimized.

no-response bot commented Nov 14, 2018

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

@no-response no-response bot closed this Nov 14, 2018

@Fishwaldo Fishwaldo reopened this Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment