Open w0lle opened 2 years ago
w0lle
commented
2 years ago w0lle
commented
2 years ago @mgreg89 Hi thanks for developing this extension. I was glad to find this one so that I would not have to write it from scratch. I used it to control my new siro devices. Controlling was just fine, but I could not get the status messages, so I looked into it.
I found 2 things which I already mentioned in the initial comment of the PR. Maybe you can have a look and see whether it makes sense or not.
Because according to my tests, I am not getting the same msgID from the multicast that I used to send the message. Maybe it just on my side and for you it is working.
mgreg89
commented
2 years ago Hello @w0lle,
thank you so much for using and contributing to my node-red-contrib! :) Very glad to know, that it is useful for other users out there.
Indeed, the status command is working for me. If I connect for example a debug node to the end of siro node I always get a response. I have seen, that you removed the check for the nodeID at the end of a received msgID. The intention behind that mechanism was to differentiate between different instances of the siro node. Before I implemented this, every instance received the message regardless if it was originally triggering the command. This would be an issue if you use this contrib multiple times across your node-red smart home project and you depend on the output of this nodes. So, I assume that other commands are working fine for you and triggering the correct node output (of the right siro node instance), aren't they? Or is it a general issue, where no msgID has the nodeID appended to it? I'm thinking, if this could be due to a new version of the Siro bridge. My is only 1 year old, but you never know :) If we know, what is the cause, we can go on with finding another approach for addressing the right node for a response.
Regarding the fix for not binding the an event handler every time the flow updates: You are absolutely right. Thank for seeing this! :) We should definitely keep that in the pull request.
Regarding the multicast to unicast change: I'm unsure, if this is really possible here as this is the way how this API works (Manual). I think the intention here is to register in the group of the bridge and devices itself.
w0lle
commented
2 years ago Hi @mgreg89, very interesting. Yes I got what you wanted to do with the msgID. But the msgID I am sending and which I get back are not the same. But since you mentioned that you siro is already 1 year old, maybe they changed something at the api implementation. I will try to find out whether I can contact the developers there. Yes triggering is not problem, it was just the receiving of the message.
Yes let us keep it.
Yeah, maybe this one is not really important. Was just an idea to prevent receiving the messages on all nodes. With receiving I just mean it is triggered. Since the device ID would be not the same, they would not process it.
Also I changed something else from my side. At the moment you are overwriting the whole message object. I would suggest, that we just overwriting the payload of the message object and keep the other fields. I can add it to the PR, so that you see what I mean.
mgreg89
commented
2 years ago Hi @w0lle,
that would be great! If you need an address, I got a response from the developers via info(at)shadeconnector.com.
Maybe the unicast approach could be a solution to your problem and make this contrib more generally working for everyone (even for users with newer Siro shades :)). Feel free to implement and test it, since I'm not quite sure, how this would work in detail. One addition from my side regarding the msgID: As you mentioned the deviceID, I am thinking that you mean the mac field of the message. And yes, you can check for the mac to be equal to the device configured in the node. But my problem is, that i have multiple instances of the nodes with even with the same device im my flows. And sure, a simple workaround here would be to have only one instance in the whole NodeRed project and accessing this one in a subflow or via link, but I feel that this would be an unintuitive limitation to the contrib, which has to be documented and will still lead to usage issues for users. So, I'm curious now, what you will find out with the developers and if the unicast approach can help us out here. Otherwise I think we should go for your first commit (dropping the nodeID check) and combining it with the (documented) limitation of only having one instance per device in a project, as the primary goal for me is, that the contrib is compatible with even newer versions of the API and can be used by every Siro user.
Yes, that is a good idea. It is a way cleaner approach then overwriting the whole message :) Would love to see you adding it to the PR :) Thank you 👍