HomeSeer / node-red-contrib-homeseer

HomeSeer Nodes for Node-RED
GNU Affero General Public License v3.0
5 stars 4 forks source link

Added support for updating device string #5

Closed AZweimiller closed 4 years ago

spudwebb commented 4 years ago

Sorry for the delay and thanks @AZweimiller for the pull request. However I would like to change a little what you did to make it more future proof and closer to what we have in the Plugin SDK. Right now the current node red implementation only allows to "control" a device either by value or by string, what you added is the ability to "update" a device value or device string. I would like to make this difference of action obvious by adding it to the topic property.

so to update a device value the msg would be:

{
    topic: 'update',
    payload: {
        value: 1234
    }
}

and to control it using a control value

{
    topic: 'control',
    payload: {
        value: 1234
    }
}

to make this backward compatible, 'control' would be the default topic i.e if there is no topic set in the msg, it would try to control.

same thing for string/status: to update a device status:

{
    topic: 'update',
    payload: {
        status: 'my status'
    }
}

and to control a device using a control text

{
    topic: 'control',
    payload: {
        status: 'my control'
    }
}

does that make sense?

AZweimiller commented 4 years ago

Yes, it makes sense. I am a bit green in this area so I will see what I can do. I am new to Git as well and had intended for this pull to only be for my first commit. I now think I should have created a branch for this feature. I was not entirely happy with using msg.payload.statusvalue in my second commit was brainstorming better ways before submitting another pull.

Your solution of using "topic" solves this. I will see if I can make it work and submit a third commit.

Thanks, Adam

spudwebb commented 4 years ago

no worries, just let me know if you don't have time or don't know how to do it, and I will pull your branch and start from there

AZweimiller commented 4 years ago

@spudwebb I've made the requested changes and it passed all of my attempts to break it. The topic is case insensitive and defaults to "control" for backwards compatibility.

Let me know if you have any questions.

Thanks, Adam

spudwebb commented 4 years ago

@AZweimiller, I removed the help part you changed and merged this PR because I need to make some other changes that would have caused some potential merge conflicts. When you have the help part ready just issue a new PR.

AZweimiller commented 4 years ago

Thanks, hopefully it's a good starting point.

KSumwalt commented 4 years ago

@spudwebb I've made the requested changes and it passed all of my attempts to break it. The topic is case insensitive and defaults to "control" for backwards compatibility.

Let me know if you have any questions.

Thanks, Adam

While the topic is defaulting if not set, when I use a change node from an MQTT node, the topic is set to the MQTT topic. So older flows are broken. See my screen captures in the HomeSeer forum sticky topic where @spudwebb posted the notice on the update.

Basics: From an MQTT Node the message is:

{
   "topic":"Test/run",
   "payload":"Basement Cabinets On",
   "qos":0,
   "retain":false,
   "_msgid":"137092e3.e2f6cd"
}

So in a Change node I had to Set the topic:


   "topic":"control",
   "qos":0,
   "retain":false,
   "_msgid":"137092e3.e2f6cd",
   "oldpaylod":"Basement Cabinets On",
   "payload":{
      "value":"255"
   }
}```

Is it possible, for better backward compatibility, to default to control if not one of the other HomeSeer toics? 

Karl S