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

Make ZWaveNode.endpoints property public and remove GetAllEndpoints method #42

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

spudwebb
Copy link

Because ZWaveNode.endpoints was internal, NodesCollection.ReplaceInformation() wasn't copying this property. So if you added a node to the network, then when called in the NodeReady event, the node passed as a parameter always had its endpoints property set to null.
To fix that I made it public (with an internal setter) and remove the GetAllEndpoints method which is no longer needed.

Another problem with NodesCollection.ReplaceInformation() that I did not fix, is that it calls the setter for ZWaveNode.name, ZWaveNode.location, ZWaveNode.keepAwake, which in turn sends some message backs to the server which are completely unnecessary.
The same thing also happens every time a node is deserialized as those setters are called too.
So I think it would make sense to have some SetName, SetLocation and SetKeepAwake methods and have an internal setter that only updates the local value, like with all other properties.
What do you think?

@marcus-j-davies
Copy link
Member

marcus-j-davies commented Oct 12, 2023

Hi @spudwebb

Agree on all counts! (and thank you)

I need to review some updates later, so i'll re-structure it this evening (I'll use your PR)
I do however propose some args to the name and location methods.

By default, if the target Node supports the Node Naming and Location CC - presently it will set it permanently (and will be seen by other controllers if the device moves to a new network say)

Maybe an option to not do that?

public Task<CMDResult> SetName(string, Name, bool UpdateCC = true)
public Task<CMDResult> SetLocation(string, Location, bool UpdateCC = true)

https://github.com/zwave-js/zwave-js-server/blob/621108f43e34eb081cc7b2ad8b8158de87dc5fe4/src/lib/node/message_handler.ts#L249

@spudwebb
Copy link
Author

Looks good to me, thanks!

@marcus-j-davies
Copy link
Member

@spudwebb

The above discussed should now be fixed with this PR.

Importantly, I don't update local values, unless the commands are successful.

Example:

 [Newtonsoft.Json.JsonProperty]
 public string name { get; internal set; }
 public Task<CMDResult> SetName(string Name, bool UpdateCC = true)
 {
     Guid ID = Guid.NewGuid();

     TaskCompletionSource<CMDResult> Result = new TaskCompletionSource<CMDResult>();

     Driver.Instance.Callbacks.Add(ID, (JO) =>
     {
         CMDResult Res = new CMDResult(JO);
         if (Res.Success)
         {
             this.name = Name;
         }
         Result.SetResult(Res);

     });

     Dictionary<string, object> Request = new Dictionary<string, object>();
     Request.Add("messageId", ID);
     Request.Add("command", Enums.Commands.SetName);
     Request.Add("nodeId", this.id);
     Request.Add("name", Name);
     Request.Add("updateCC", UpdateCC);

     string RequestPL = JsonConvert.SerializeObject(Request);
     Driver.Instance.ClientWebSocket.SendInstant(RequestPL);

     return Result.Task;
 }

I'll get it merged soon!

@marcus-j-davies marcus-j-davies merged commit 0d4a289 into zwave-js:4.0.0 Oct 12, 2023
1 check passed
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.

2 participants