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.

Sign up for GitHub

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

Jump to bottom

[openwebnet] Add a time stamp when an alarm zone event occurs #14743

Closed
wants to merge 6 commits into from
Closed

[openwebnet] Add a time stamp when an alarm zone event occurs #14743

wants to merge 6 commits into from

Conversation

fabgio
Copy link
Contributor

@fabgio fabgio commented Apr 3, 2023 •
edited
Loading

This PR allows to show a time stamp when an alarm zone event occurs on Bticino/Legrand Alarm systems such as an Intrusion or a Tampering
This PR closes #14535

Description

This PR allows to show a time stamp when an alarm zone event occurs, i.e. MM/DD/YY,HH:MM INTRUSION

Test

Tested on BTicino/Legrand bulgrar-alarm unit 3486

@fabgio fabgio requested a review from mvalla as a code owner April 3, 2023 19:35
@jlaur jlaur changed the title [openwebnet] Add a time stamp when an alarm zone event occurs #14535 [openwebnet] Add a time stamp when an alarm zone event occurs Apr 4, 2023
@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Apr 4, 2023
@mvalla mvalla added awaiting feedback Awaiting feedback from the pull request author additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Apr 5, 2023
@mvalla
Copy link
Contributor

mvalla commented Apr 5, 2023

@fabgio I do not see in this PR all the changes requested already in this comment

@fabgio
Copy link
Contributor Author

fabgio commented Apr 5, 2023 •
edited
Loading

@mvalla I did a commit which followed your comment but as you see I reverted it as integrations' tests had regressions. So I am proposing this solution that has been tested successfully on my plant.
The channel ALARM_ZONE_ALARM correctly shows the time stamp and the associaciated event as well. Moreover, It correctly resets to None when the system is re-armed.
LocalDateTime class is thread safe (Javadoc 17) to ensure that no race conditions heppen.

fabgio added 2 commits April 5, 2023 16:36
@fabgio
[openwebnet] alarm: update README
77f55e6 
Signed-off-by: Giovanni Fabiani <[email protected]>
@fabgio
[openwebnet] alarm: update README
f252326 
Signed-off-by: Giovanni Fabiani <[email protected]>
@fabgio
Copy link
Contributor Author

fabgio commented Apr 6, 2023 •
edited
Loading

@mvalla How should I act for the binding to be tested? I pubblished an announcement on market place but I have been told that only one contributor per binding is authorized to pubblish. and to contact you to coordinate us

mvalla
mvalla requested changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@mvalla mvalla left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@fabgio see my review.
I think current solution has a wrong design since you are putting together two information in the same channel. This solution also generates an unwanted breaking change because you are re-defining the alarm channel.
Other comments:

  • README.md will need to be updated with the new timestamp channel documentation
  • the new timestamp channel should be added to the .xml definitions (channels.xml and BusAlarmZone.xml)
  • you also have to generate the default English localization, see: https://github.com/openhab/openhab-addons#translations

bundles/org.openhab.binding.openwebnet/README.md
| `alarm` | `bus_alarm_zone` | String | Current alarm for the zone (`SILENT`, `INTRUSION`, `TAMPERING`, `ANTI_PANIC`) | R |
| Channel Type ID (channel ID) | Applies to Thing Type IDs | Item Type | Description | Read/Write |
|------------------------------|----------------------------------------|-------------|----------------------------------------------------------------------------------------------------------------------------------------------|:-----------:|
| `state` | `bus_alarm_system`, `bus_alarm_zone` | Switch | Alarm system or zone is active (`ON`) or inactive (`OFF`) | R |
Copy link
Contributor

Choose a reason for hiding this comment

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

do not modify other lines if they are not related to the PR

bundles/org.openhab.binding.openwebnet/README.md
| `network` | `bus_alarm_system` | Switch | Alarm system network state (`ON` = network ok, `OFF` = no network) | R |
| `battery` | `bus_alarm_system` | String | Alarm system battery state (`OK`, `FAULT`, `UNLOADED`) | R |
| `armed` | `bus_alarm_system` | Switch | Alarm system is armed (`ON`) or disarmed (`OFF`) | R |
| `alarm` | `bus_alarm_zone` | String | Current alarm for the zone (`SILENT`, `INTRUSION`, `TAMPERING`, `ANTI_PANIC`) as well as the date and time of the event (YY/MM/DD hh:mm:ss) | R |
Copy link
Contributor

Choose a reason for hiding this comment

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

this channel should remain as it was before: it's not a good idea to put together two differnet information: the type of alarm that was triggered an its timestamp. This is not a good desing at all, as it makes difficult for example for scripts to detect which type of alarm has been triggered (you would need to parse the string).
As already suggested, have a look to the Verisure Alarm binding, where there is one channel for the alarm type (of type String) and one for the timestamp (of type DateTime)

...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
switch (w) {
case ZONE_ALARM_INTRUSION:
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_INTRUSION));
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_INTRUSION + " " + now.format(format)));
Copy link
Contributor

Choose a reason for hiding this comment

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

timestamp should not be concatenated to the alarm channel, but returned in a new timestamp channel. This is not a good design. See explanation in previous comment

...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
break;
case ZONE_ALARM_TAMPERING:
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_TAMPERING));
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_TAMPERING + " " + now.format(format)));
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment

...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
break;
case ZONE_ALARM_ANTI_PANIC:
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_ANTI_PANIC));
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_ANTI_PANIC + " " + now.format(format)));
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment

...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
break;
case ZONE_ALARM_SILENT:
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_SILENT));
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_SILENT + " " + now.format(format)));
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment

...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
break;
case ZONE_ALARM_TECHNICAL:
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_TECHNICAL));
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_TECHNICAL + " " + now.format(format)));
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment

...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
break;
case ZONE_ALARM_TECHNICAL_RESET:
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_TECHNICAL_RESET));
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_TECHNICAL_RESET + " " + now.format(format)));
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment

...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
break;
default:
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_NONE));
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_NONE + " " + now.format(format)));
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment

Copy link
Contributor Author

@fabgio fabgio Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see, ok I will keep working on the basis of what you suggested. Do not consider this PR!

@mvalla mvalla added discussion and removed awaiting feedback Awaiting feedback from the pull request author additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Apr 7, 2023
@fabgio fabgio closed this Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvalla mvalla mvalla requested changes

Assignees
No one assigned
Labels
discussion enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[openwebnet] Alarm: add a time stamp when a alarm zone event occurs
3 participants
@fabgio @mvalla @jlaur