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

Add PIN code support for ideAlarm #168

Closed
wants to merge 4 commits into from
Closed

Add PIN code support for ideAlarm #168

wants to merge 4 commits into from

Conversation

fastlorenzo
Copy link

@fastlorenzo fastlorenzo commented Jun 21, 2019

Fixes #74

@5iver
Copy link
Member

5iver commented Jun 21, 2019

I don't have ideAlarm setup to test. @besynnerlig, can you please review and test?

We should also add something to the documentation.

@fastlorenzo
Copy link
Author

Changes made. I think we should update the doc as well. The armed home and armed away items needs to be Number and not Switch types.

@besynnerlig
Copy link
Contributor

I don't have ideAlarm setup to test. @besynnerlig, can you please review and test?

Sure, I'll do that :)

@besynnerlig
Copy link
Contributor

Thank you for your contribution to the repo.

Fixes #47

That must be wrong. I cant see what this to do with #47

Except for the title there is not much of a pull request description. The purpose of this Pull Request is quite obvious but can you provide a brief description or summary of the new feature?

I'll have a look at the code within a few days.

@fastlorenzo
Copy link
Author

Summary of the new feature: a PIN code can be used to disarm the alarm when armed home/away.
The PIN code needs to be set in the configuration.py file and must be a digit.
If you do not wish to use the PIN code feature, the pinCode property can be set to None.

When using the pinCode feature, the zone armAwayToggleSwitch and armHomeToggleSwitch needs to be set to a Number and not a Switch type.

Copy link
Contributor

@besynnerlig besynnerlig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a user (a home owner with ideAlarm) enters a PIN code, instead of storing the entered code into a separate OH Item, you have chosen in the proposed PR to store the entered code into the OH switch items that are currently used to toggling the alarm zone between armed and disarmed. To be able to do that you have changed the OH Item types for the 2 toggle switches belonging to each alarm zone from Switches to Numeric. Then you are reading the entered PIN code as the toggleSwitch new numeric value. Doing that is a breaking change for all users, that is they can not upgrade to this new version without making changes in their OH Items file and most likely quite extensive changes to their custom functions. (Not to mention all documentation changes that need to be done) Furthermore if they have persisted the state of the toggleSwitches they would need to manually delete the time series stored in the database for those items.

You haven't added a separate alarm event that can be triggered whenever a faulty PIN code has been entered hence what will follow when that happens is hard coded (a log entry). Maybe some users would prefer to have a light to turn on, a message be sent etc.

Arming and disarming ideAlarm in various ways is already very easy using a script and using a PIN code device doing that isn't complicated. Why haven't you simply taken the standard approach, made a script to take care of it all like in the following example (untested)

from core.rules import rule
from core.triggers import when
from configuration import idealarm_configuration

wrongCodeCount = 0

@rule("PIN Entered", description="This rule fire when a PIN code has been entered")
@when("Item Pin_Code changed")
def pin_entered(event):
    pin_entered.log.info('A new PIN code was entered [{}]'.format(event.itemState))
    if event.itemState == idealarm_configuration['PIN'] # You will have to add this in the configuration file
        events.sendCommand('Toggle_Z1_Armed_Away', 'ON') 
        wrongCodeCount = 0
    else:
        wrongCodeCount = wrongCodeCount + 1
        if wrongCodeCount > 2:
            pin_entered.log.info('Wrong PIN code was entered multiple times')
            # Do something more here, maybe send an SMS or turn on a light

Wouldn't the approach that I suggest above not only be much easier but also more flexible do you think? It won't require any breaking changes to ideAlarm at all.

@fastlorenzo
Copy link
Author

fastlorenzo commented Jun 22, 2019

Wouldn't the approach that I suggest above not only be much easier but also more flexible do you think? It won't require any breaking changes to ideAlarm at all.

This could indeed work but I took the approach of changing the toggle switches for security reasons.
When you use the pinCode, you cannot disarm the alarm without knowing the pin code which is hard coded in the python script, which means even if you have access to the event bus of OH you cannot disarm the alarm.
Regarding the breaking change, I agree this is one if the pinCode is set to something else than None, but if it is set to None (which is the case by default), the script works as previously (using Switch items).

You haven't added a separate alarm event that can be triggered whenever a faulty PIN code has been entered hence what will follow when that happens is hard coded (a log entry). Maybe some users would prefer to have a light to turn on, a message be sent etc.

True, I think this could be a good addition for ideAlarm to have an item which contains an error message. A specific item with invalid pin code might work as well.

@fastlorenzo
Copy link
Author

I wanted to update the documentation, but don't know how to generate the html from the rst.txt files. Any help?

@CrazyIvan359
Copy link
Contributor

@fastlorenzo you should be adding or editing documentation in the .rst files in the /Sphinx directory. I believe Scott is pushing the changes to the output files separately so that PRs are more readable since rebuilding the pages can sometimes modify up to 100 or more files.

If you want to build the pages so you can see them for proofing purposes, there are instructions here, just don't commit any changes to files in the /docs directory.

@besynnerlig
Copy link
Contributor

besynnerlig commented Jun 22, 2019

Wouldn't the approach that I suggest above not only be much easier but also more flexible do you think? It won't require any breaking changes to ideAlarm at all.

This could indeed work but I took the approach of changing the toggle switches for security reasons.
When you use the pinCode, you cannot disarm the alarm without knowing the pin code which is hard coded in the python script, which means even if you have access to the event bus of OH you cannot disarm the alarm.

You could. Frankly, if an intruder can access the Event Bus, e.g use a script to send a given command to the specified Item there are numerous ways to break the functionality of ideAlarm.
for example sendCommand("Z1_Arming_Mode", 0)

Regarding the breaking change, I agree this is one if the pinCode is set to something else than None, but if it is set to None (which is the case by default), the script works as previously (using Switch items).

No it won't.
For example trying to get the integer value of a switch like in your statement pinCode = getItemValue(itemName, 0) will throw AttributeError: 'org.eclipse.smarthome.core.library.types.OnOffType' object has no attribute 'intValue'

Even if you adapt your code in some way you must remember that the users have custom scripts that expect the arming mode toggle switches to be of the type switch. If we shall change the item type for the arming mode toggling switches we need to gain something quite valuable to do so. We also need to rename the items to not hold the word "switch" in their names because it's confusing. Unfortunately I can't see that we gain security or anything else by doing this.

You haven't added a separate alarm event that can be triggered whenever a faulty PIN code has been entered hence what will follow when that happens is hard coded (a log entry). Maybe some users would prefer to have a light to turn on, a message be sent etc.

True, I think this could be a good addition for ideAlarm to have an item which contains an error message. A specific item with invalid pin code might work as well.

It's not what I meant. ideAlarm has Event Helpers for specific events. That's the correct place to create your own error messages. If your PR would be approved we'd need another event helper: onPinEntered(zone, pincode) . Just letting ideAlarm log an entry when a faulty PIN is entered is not enough. Having an event helper would also allow a user to define and handle multiple PIN codes. But all this and more is easier to implement using the simple script that I suggested above.

I still can't see any benefits using the suggested PR over using the simple script example that I suggested above. If you don't agree with me, please convince me by giving me more details in how the security is improved and please let me know how a user can customize what will happen when a PIN code (correct or faulty) has been entered.

@fastlorenzo
Copy link
Author

I'll try your approach and see if it can work, it can indeed be easier to implement than all the breaking changes I wanted to introduce.

You could. Frankly, if an intruder can access the Event Bus, e.g use a script to send a given command to the specified Item there are numerous ways to break the functionality of ideAlarm.
for example sendCommand("Z1_Arming_Mode", 0)

Good point, it might be interesting to find a solution so only the ideAlarm script can change the state of those items. I'm not sure this is something feasible in OH at the moment.

@besynnerlig
Copy link
Contributor

I'll try your approach and see if it can work, it can indeed be easier to implement than all the breaking changes I wanted to introduce.

Please do that and please don't hesitate to ask if there's something that you can't solve by using the example script.

If you after trying think that the script is working well, we could add the example script to the documentation.

@besynnerlig
Copy link
Contributor

I'll try your approach and see if it can work, it can indeed be easier to implement than all the breaking changes I wanted to introduce.

@fastlorenzo , have you had the time yet to check it out?

@5iver
Copy link
Member

5iver commented Jul 23, 2019

Pinging @fastlorenzo... shall we close this?

@5iver
Copy link
Member

5iver commented Aug 17, 2019

Closing due to lack of response.

@5iver 5iver closed this Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add PIN code support for ideAlarm
4 participants