andrei-tatar / nora-service

NORA backend service
https://node-red-google-home.herokuapp.com
34 stars 30 forks source link

Locks #10

Open rgerrans opened 4 years ago

rgerrans commented 4 years ago

@andrei-tatar I saw you merged in the lock changes to the service. Has that been deployed? I'm getting an "Invalid object for sync" error and don't see any obvious disconnects in naming between the node and services files. I'll keep looking in the meantime.

Thanks

andrei-tatar commented 4 years ago

Yes, you can see the changes here: https://github.com/andrei-tatar/nora-service/pull/9 I also added isJammed in the state, so the node-red plugin needs to provide it.

export interface LockUnlockState extends State {
    isLocked: boolean;
    isJammed: boolean;
}

Also, please don't open new issues if there's already one for that topic (https://github.com/andrei-tatar/nora-service/issues/7). Just reopen closed ones or add comments to them.

rgerrans commented 4 years ago

Thanks @andrei-tatar Will do going forward on reopening. I had originally put in the isJammed but wasn't sure the best way to do it in the node. Wasn't sure if should just make it a different payload for jammed/unjammed or set it up as a msg.payload.jammed (along the lines of what you did in the light node). Any thoughts on what would be a cleaner / more consistent approach with the other nodes?

rgerrans commented 4 years ago

@andrei-tatar I'm still trying to get comfortable with the node code base. I'm slowly thinking I'm understanding it (I haven't written real code in 20 years so am trying to understand it as I go), but somehow I've done something or have a typo I"m not seeing that is creating the following error

src/nodes/nora-lock.ts:53:49 - error TS2345: Argument of type 'LockState' is not assignable to parameter of type 'boolean'.
53                 tap(([_, state]) => notifyState(state)),
                                                   ~~~~~

Here's the file - https://github.com/rgerrans/node-red-contrib-nora/blob/master/src/nodes/nora-lock.ts

Any suggestions or guidance would be greatly appreciated (I'm just trying to get a base node to work then will play with the payload structure).

andrei-tatar commented 4 years ago

@rgerrans notifyState is the function used to take the state of the node and update the node status string.

You defined it as:

function notifyState(isLocked: boolean) {
  stateString$.next(`(${isLocked ? 'locked' : 'unlocked'})`);
}

you could define it as:

function notifyState(state: LockState) {
  stateString$.next(`(${state.isLocked ? 'locked' : 'unlocked'}:${state.isJammed?'jammed':'-'})`);
}

and just call it on the state object. Instead of:

const lvalue = state.isLocked;
const jvalue = state.isJammed;
notifyState(state.isLocked);
notifyState(state.isJammed);

you can just: notifyState(state);

Your code actually calls it with a state object but the function is defined to accept a boolean. That's why it fails.

rgerrans commented 4 years ago

Ahhhh, that makes sense. I wasn't looking all the way to the bottom to see that function and close the loop on the issue. Thanks!!!

andrei-tatar commented 4 years ago

Vs Code helps a lot to navigate through the code. Just ctrl + click :)

rgerrans commented 4 years ago

Thanks. It's more foundational than that, where I've never worked that much in javascript so don't always know when I'm looking at a defined function vs. a standard call ;-) I've been trying to step through everything to make sure I understand but didn't make it to the end to realize that was a function.

rgerrans commented 4 years ago

@andrei-tatar I think I have it working correctly on the Node-Red side receiving commands from Google but it's having an issue when I send commands to Google. I get a invalid object for update error when I try to push a change to the node.

Edit: Deleted the reference to Execute since I realized that I was probably looking in the wrong file since the command from Google comes through fine. Any suggestions on where to look to try to troubleshoot? Thanks.

andrei-tatar commented 4 years ago

@rgerrans I think you need to send the whole state object. Not just isLocked, but also isJammed.

rgerrans commented 4 years ago

@andrei-tatar Thanks. I actually had a typo that I finally found.... Will test a bit before sending a pull request.

andrei-tatar commented 4 years ago

@rgerrans wouldn't be cleaner and easier to treat isJammed differently? I assume most of the people won't use it anyway. So have input/output payload only for isLocked. isJammed would be set when a message with topic "jammed" arrives or something similar.

rgerrans commented 4 years ago

@andrei-tatar I know, I was definitely going back and forth on taking a different approach to be able to keep locked in the main payload. I had toyed with the idea of doing it along the lines of what you did with lights and brightness. I hadn't thought about using the topic, how would you then treat the outbound msg from the node?

Also, Google didn't treat isJammed: true like I thought it would. It still changed lock status on request even if isJammed: true was set? I saw that there was an error code that should be thrown in that situation but couldn't quite understand how to use your error code pull to check that in order to block the lock/unlock action?

Edit: Realized I wouldn't need an outbound "isJammed" message from the Node. I'll go with the Topic approach for inbound messages. I'll also try to put in a blocker/error message if the isJammed: True Probably take me a few days with the holidays to get back to this.

rgerrans commented 4 years ago

@andrei-tatar So I implemented the "Jammed" topic approach. I also put in error messages if they use Google and the lock reflects Jammed status. Google still responds with the requested action, but the node won't send any commands and sends an error message instead. Let me know any other thoughts on changes. Otherwise the pull should be ready to go

rgerrans commented 4 years ago

@andrei-tatar I realized that two of my Lock nodes never deployed to Google. I was just trying to recreate them to see if it was just something that happened during testing and ran into a weird issue. If I change the Unlock value from binary | false to number "0" it doesn't retain when I save the config change. But I can change it to string "0", deploy that and then change it to number "0". Not sure if the bug is on my end or on Node-Red's? It works fine for the Lock value if I go straight from binary|true to Number|255 or Number|0? I don't see any difference in the code between the two. Any thoughts on what I might be missing?

andrei-tatar commented 4 years ago

@rgerrans probably invalid state. If the state is invalid (does not pass schema check), new devices won't get synced.

rgerrans commented 4 years ago

@andrei-tatar That makes sense. When I deleted and recreated they created the Google end points.

Let me know any thoughts on that bug in setting the UnlockValue? Otherwise everything seems to be testing out fine.