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

Security Arming Model - Proposal #9

Closed
boc-tothefuture opened this issue Jun 6, 2017 · 30 comments
Closed

Security Arming Model - Proposal #9

boc-tothefuture opened this issue Jun 6, 2017 · 30 comments

Comments

@boc-tothefuture
Copy link

boc-tothefuture commented Jun 6, 2017

I think we should modify the arming model we are using for the Omni because I am not sure it appropriately maps right now. This issue also exists in the V1 driver.

Right now, we have as user selectable states:
Off, Day, Night, Away, Vacation, Day Instance, Night Delay.
The issue is that Off is the only of these states that you can actually directly move an Omni into.

When you select, for example 'Away', the Omni really moves into the 'Arming Away' state and after the exit delay, then actually moves into the 'Away' state. This is an important distinction because there are rules you may want to trigger in both sets of states. For example, in the 'Away' state, I want to lock all my doors. I want that to trigger after the exit delay - not during the 'Arming Away' state because the deadbolt comes out while I am trying to close the door..

I think we will need to change the actual state representation in binding, such that the user selectable states are the arming states (besides off) and then switch over to the real state after arming.

The protocol has the appropriate support for these modes:

For HAI Omni series controllers, the security mode for an area is as follows:
0 Off
1 Day
2 Night
3 Away
4 Vacation
5 Day instant
6 Night delayed

Bit 3 of the security mode byte will be set during the arming exit delay, resulting in the following
additional security modes:
9 Arming day
10 Arming night
11 Arming away
12 Arming vacation
13 Arming day instant
14 Arming night delayed 

This has an impact to the driver, but I think its important to get this right before its released because I don't see a work around.

@boc-tothefuture
Copy link
Author

Adding @digitaldan to this discussion.

@craigham
Copy link

craigham commented Jun 6, 2017

Having those distinctions available in the rules sounds like a great idea.

When you say impact to driver, do you mean the binding code, or do you think changes need to be made to the jomnilink lib as well?

@boc-tothefuture
Copy link
Author

I think the binding will certainly be impacted - looking at the driver code, the mode support appears to be in place already
However, in the log when I arm the system in night mode at the panel the binding logs that its in area mode 2 instead of area mode 10. I will take a look at the jomnilink side.

From the binding perspective, we have to decide if the user selectable modes of (Night, Away, Vacation, etc) go away and are replaced with (Arming Away, Arming Vacation)? I am not sure what is right there.

I think if we keep the user selectable modes of "Night", "Away", etc it will be a problem because if the user changes the mode to "Night" it will instantly fire those rules even if the actual mode is "Arming Night".

@digitaldan
Copy link

HI, sorry to be late to the party :-) I had to go back and read that code a few times....I think we have a SecurityMode channel, it can accept the same string modes as the 1.x version does, but that does not restrict what the state value is for the channel.

So for example the supported Commands: of the channel would be:
{ "Off", "Day", "Night", "Away", "Vacation", "Day-Instant","Night-Delayed" }'

But the channel state could be:

{ "Off", "Day", "Arming Day","Night","Arming Night", "Away","Arming Away" ,"Vacation"," Arming Vacation", "Day-Instant","Arming Day-Instant","Arming Night-Delayed" }'

Furthermore, would would have a different channel-type for lumina and omni so that we can define the channel commands in the XML file which would be much nicer for users as UI's like the paperUI could automatically populate the arming dropdown menu with the correct command value.

@boc-tothefuture is this what you are thinking?

@craigham
Copy link

craigham commented Jun 7, 2017

Dan,

Let's assume panel security state is 'Off'

So by selecting a command 'Night', the state channel would transition to 'Arming Night', and then after the arming delay to 'Night' ? How does the state avoid that initial change to 'Night'? I haven't seen that mechanism before within openhab.

@boc-tothefuture
Copy link
Author

What is @craigham is pointing out is my big concern. Because if it openhab fires the 'Night' or 'Away' item changed when the user changes it those rules are going to trigger early, then again after the exit delay.

I am also concerned, because when I look at the jominlink code, it appears it should be picking up those additional security modes. But when I go to the panel and do a night arming it logs mode 2, not mode 10.

@digitaldan
Copy link

What is @craigham is pointing out is my big concern

Yes I see, I had incorrectly assumed this was implemented eclipse-archived/smarthome#595 , but it looks like it has stalled out.

I am also concerned, because when I look at the jominlink code, it appears it should be picking up those additional security modes. But when I go to the panel and do a night arming it logs mode 2, not mode 10.

Well, i wrote that code so long ago that I'm embarrassed when I look at it now :-) If the code is not working right we (I) can fix it. At a minimum I need to at lest run it through a code formatter!

So in terms of ESH mechanics, are you proposing having a command channel and and current state channel (so 2 channels). Can you describe how your proposal would look in the ESH model? Sorry If I am not understanding your proposal.

@craigham
Copy link

craigham commented Jun 7, 2017

I think we could solve it pretty easily within sitemaps by just using a mapping. Paper is more difficult because it is driven by the xml, etc. This would result in only the one channel. But yes a command channel would probably work as well.

imho expecting users to get a nice polished experience out of paper is not really realistic. If they want polish and wife acceptance they'll have to use basic or classic.

@craigham
Copy link

craigham commented Jun 7, 2017

I guess to support the 2 channel solution, we could have a command channel for the area. also have a read-only state channel, the value of which is driven entirely by the omni. Then the area handler sends commands to the omni, but reads the status.

@digitaldan
Copy link

imho expecting users to get a nice polished experience out of paper is not really realistic. If they want polish and wife acceptance they'll have to use basic or classic.

Yes, but I think its important to support these features as other UI's (like habpanel) could start to utilize them more. This future proofs our implementation and I think helps the overall project. But I agree, the paperUI is not what I would be building for.

@boc-tothefuture
Copy link
Author

I don't know enough about the XML vs. mapping debate here to weigh in so I will abstain.

The only path I see (but I am no expert) given the current constraints of openhab autoupdate functionality would be to have the two channels. If we have the two channels, we can provide the normal names for the intentions while having the state accurately reflected.

Will that be confusing to the users? I am not sure how common that setup is.
Do we have the 'intention' channel updated when the omni state changes?

@digitaldan After a cursory look at the code, I am not sure where the defect could be, its simply reading a byte (I don't see any code stripping off the third bit or anything). I am starting to wonder if I am reading the protocol spec wrong.

@digitaldan
Copy link

you can add autoupdate="false" when binding a channel to a item, this will disable a command from updating the state, but we will still be able to update it from the binding side. While I would prefer to make this more concrete (and the default option), I think this gets us where we want to be, and as long as we document it correctly with an example, I think would be ok. Any reason this would not work?

https://github.com/eclipse/smarthome/blob/master/bundles/model/org.eclipse.smarthome.model.item/src/org/eclipse/smarthome/model/item/AutoUpdateGenericBindingConfigProvider.java

@craigham
Copy link

craigham commented Jun 7, 2017

Dan,

Can you specify autoupdate="false" from the paper ui, or any ui? Or does this mean items will have to be specified within files?

@digitaldan
Copy link

Unfortunately that is correct, until we have channel options for not auto updating.

@boc-tothefuture
Copy link
Author

I may just be misreading this, but the protocol doc is not lining up with what I am seeing when look at what we are getting over the wire. It appears we will have to do the conversion between the modes in the binding:

When I arm the system in Night mode, this comes across the wire:
0x21 0x09 0x3b 0x05 0x06 0x00 0x01 0x02 0x00 0x00 0x3c 0x6d 0x48 0x00 0x00 0x00
Breaking that down into the protocol parts:

0x21  start char
0x09  length
0x3b  type (0x23?)
0x05  0x05
0x06  area number MSB
0x00  area number LSB
0x01  Mode
0x02  Alarms
0x00  entry timer
0x00  exit timer
0x3c  second area MSB
0x6d  second area LSB
0x48  Mode for second area
0x00  area alarms for second area
0x00  entry timer for second area
0x00  exit timer for second area

60 seconds later when 'Arming Completes'
0x21 0x09 0x3b 0x05 0x06 0x00 0x01 0x02 0x00 0x00 0x00 0x6d 0x59 0x00 0x00 0x00

This maps to:

0x21 start char
0x09 length
0x3b type (0x23?)
0x05 0x05
0x06 area number MSB
0x00 area number LSB
0x01 Mode
0x02 Alarms
0x00 entry timer
0x00 exit timer
0x00 second area MSB
0x6d second area LSB
0x59 Mode for second area
0x00 Area alarms for second area
0x00 Entry timer for second area
0x00 Exit timer for second area

So despite, what I see in the protocol documentation, the mode doesn't actually indicate a difference between arming and being in the actual state.
I added some more debugging into the jomnilink driver and had this output during an arming event:

Area 0
Area Update
Number 1
Mode 2
Alarms 0
Entry Timer 0
Exit Timer 60

and after 60 seconds:

Area 0
Area Update
Number 1
Mode 2
Alarms 0
Entry Timer 0
Exit Timer 0

So, I think the logic is that if you have an exit timer value > 0, then you are in an arming mode. Otherwise you are in the actual mode. I don't know if this change belongs in jomnilink or the binding.

@digitaldan
Copy link

When I arm the system in Night mode

The response says the mode is 1 which is Day mode, thats not correct either?

So, I think the logic is that if you have an exit timer value > 0, then you are in an arming mode. Otherwise you are in the actual mode.

So the panel I ordered was defective and I had to send it back, i should have the new one tomorrow, so I can test as well hopefully this weekend. Since I have not touched this stuff in 3+ years it may take me a little bit to get back up to speed.

@boc-tothefuture
Copy link
Author

I am actually not sure what the output is of the RX line from jomnilink, if it is doing some sort of conversion? Because the mode is actually correct when its read as an unsignedByte from jominlink. For example:

eadBytesEncrypted2: Omni message Length 9
readBytesEncrypted2: Additional bytes to read 0
RX: 0x21 0x09 0x3b 0x05 0x06 0x00 0x01 0x02 0x00 0x00 0x3c 0x6d 0x48 0x00 0x00 0x00 
readBytesEncrypted2: Data still available after read 0
Area 0
Area Update
Number 1
Mode 2
Alarms 0
Entry Timer 0
Exit Timer 60

@digitaldan
Copy link

I'll have to play with it on Friday, but lets assume the docs are correct and I have a bug somewhere in the omnilink driver.

@boc-tothefuture
Copy link
Author

I am looking forward to @digitaldan getting his omni panel. I spent about an hour looking at this and made no progress. I feel like I am on crazy pills. jomnilink is clearly working but what I am seeing over the wire isn't matching what I see in the protocol spec. Its almost like I am looking at the wrong spec.

We will see what @digitaldan uncovers when he looks. But, based on my testing across a variety of modes, we can use that exit timer setting (anything > 0) to determine if we are in arming mode or in the armed state of the desired mode.

@craigham
Copy link

craigham commented Jun 8, 2017

Brian, how have you hooked in the jomnilink source debugging into eclipse? I tried adding the jomnilink project to the other projects folder, and then attaching that source to the jar, but couldn't seem to set up breakpoints and stuff.

@boc-tothefuture
Copy link
Author

@craigham I wasn't running it in the eclipse debugger. I was just editing the code in atom, compiling with ant, then running it from the command line. The main method of the example jar has debug mode on and spits out all that info.

@craigham
Copy link

craigham commented Jun 8, 2017

fwiw I am seeing the same byte patterns on the wire as you are Brian.

I ended up just compiling my own jomnilink with the debug flag turned on inside the connection class, and debug info into the Javac for the ant target, and I use that in my lib folder.

@digitaldan
Copy link

Either the spec is wrong OR they have updated it and not published that publicly!

You are both correct they do not send the arming modes, in fact I think those are now obsolete. I also see the exit timer value when the arming starts and again when it completes, so if I do a arm day I first get:

mode:1 entryTimer:0 exitTimer:60 alarms:0

and then 60 seconds later I get

mode:1 entryTimer:0 exitTimer:0 alarms:0

So, I think the logic is that if you have an exit timer value > 0, then you are in an arming mode. Otherwise you are in the actual mode.

Correct, question is, how do we handle this? Do we keep the delayed mode that we have and do the logic in the binding (my vote)? Or do we keep this exactly to the spec/wire and let users do something with rules to tie the exit timers to mode?

I don't know if this change belongs in jomnilink or the binding.

The library has no concept of this, it simply reports the structures, so unless we build a new abstraction into the library, it needs to be in the binding (which is the abstraction).

I ended up just compiling my own jomnilink with the debug flag turned on inside the connection

Yes, I went back and cleaned up all the super ugly system.out's, you can now use logging within OH like:
log:set TRACE com.digitaldan.jomnilinkII

Note that I also moved from ant to maven, the README has build instructions now.

@craigham
Copy link

I have no problem with anything you said.

In particular I also agree about handling these changes in the binding, and not adding more to the lib. Nice changes to the jomnilink lib too.

@digitaldan
Copy link

digitaldan commented Jun 12, 2017

Ok, so this is now working like it should according to the docs.

Next question! The arm/disarm command requires the User code number, which is not actually the the value of the code, but the codes object number (1st code, 2nd code, 3rd code ...).

Right now I have it hardcoded to code 1 for testing, but thats probably not good. i think the desire should be to pass in the numeric code when arming/disarming and we will lookup the code number and use it if it exists, and error if it does not.

Thats all easy, whats more difficult is how to model this as a channel in ESH. Right now we receive modes (0,1,2,3...) and when send modes (0,1,2,3).

I'm open to suggestions. We could have a send only channel per mode that takes a code (arm channel, disarm channel, day channel,....) and have the current mode channel read only. This actually makes sense since the mode channel right now has states that can not be set (delayed). I'm thinking about prototyping this unless there are objections.

@craigham
Copy link

no objections here. sorry, but no suggestions either.

@boc-tothefuture
Copy link
Author

boc-tothefuture commented Jun 12, 2017

Correct, question is, how do we handle this? Do we keep the delayed mode that we have and do the logic in the binding (my vote)? Or do we keep this exactly to the spec/wire and let users do something with rules to tie the exit timers to mode?

@digitaldan
I vote that we handle it in the binding. I don't think we can easily do it in the rules.

@boc-tothefuture
Copy link
Author

I'm open to suggestions. We could have a send only channel per mode that takes a code (arm channel, disarm channel, day channel,....) and have the current mode channel read only. This actually makes sense since the mode channel right now has states that can not be set (delayed). I'm thinking about prototyping this unless there are objections.

I also agree with this. Current mode channel is read only and the 'write' channel takes a code.

@digitaldan
Copy link

Shall we close this issue? I think we are good on our implantation .

@craigham
Copy link

Done. Nice work.

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

No branches or pull requests

3 participants