-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[openwebnet] Add a time stamp when an alarm zone event occurs #14819
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabgio see required changes.
You also have to generate the default English localization, see: https://github.com/openhab/openhab-addons#translations
This is a second PR after #14743
PS
Do not change again the PR, beacuse each time a new review is required.
| `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`) | R | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you modifiy only the lines related to adding the new channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I did'nt change this lines I can't figure out why they appear as changed. I cannot spot any difference.. Can you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe spaces? Copy paste the original lines from original version
...webnet/src/main/java/org/openhab/binding/openwebnet/internal/OpenWebNetBindingConstants.java
Outdated
Show resolved
Hide resolved
@@ -232,31 +240,44 @@ private void updateZoneAlarm(WhatAlarm w) { | |||
switch (w) { | |||
case ZONE_ALARM_INTRUSION: | |||
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_INTRUSION)); | |||
updateZoneAlarmTimeStamp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have to choose when updateZoneAlarmTimeStamp is called. This way it's called 2 times: in the switch inside updateZoneAlarm and then again in the updateZone switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I choose to call it in updateZone() to invoke it only once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
break; | ||
case ZONE_ALARM_TAMPERING: | ||
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_TAMPERING)); | ||
updateZoneAlarmTimeStamp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
break; | ||
case ZONE_ALARM_ANTI_PANIC: | ||
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_ANTI_PANIC)); | ||
updateZoneAlarmTimeStamp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -218,6 +225,7 @@ private void updateZone(Alarm msg) { | |||
case ZONE_ALARM_TECHNICAL: | |||
case ZONE_ALARM_TECHNICAL_RESET: | |||
updateZoneAlarm(w); | |||
updateZoneAlarmTimeStamp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have to choose when updateZoneAlarmTimeStamp is called. This way it's called 2 times: in the switch inside updateZoneAlarm and then again in the updateZone switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I will call it in updateZone() only
break; | ||
default: | ||
updateState(CHANNEL_ALARM_ZONE_ALARM, new StringType(ALARM_NONE)); | ||
logger.warn("Alarm.updateZoneAlarm() Ignoring unsupported WHAT {} for zone {}", w, this.deviceWhere); | ||
} | ||
} | ||
|
||
private void updateZoneAlarmTimeStamp() { | ||
ZonedDateTime now = ZonedDateTime.now(); | ||
ChannelUID cuid = new ChannelUID(getThing().getUID(), CHANNEL_ALARM_ZONE_TIMESTAMP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is useless. Why don't you do the same like in initialize():
updateState(CHANNEL_ALARM_ZONE_TIMESTAMP, ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -17,6 +17,7 @@ | |||
<!-- read only --> | |||
<channel id="state" typeId="alarmZoneState"/> | |||
<channel id="alarm" typeId="zoneAlarm"/> | |||
<channel id="timestamp" typeId="zoneAlarmTimeStamp"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<channel id="timestamp" typeId="zoneAlarmTimeStamp"/> | |
<channel id="timestamp" typeId="zoneAlarmTimestamp"/> |
Timestamp and not TimeStamp (it's one word)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -461,6 +461,14 @@ | |||
</state> | |||
</channel-type> | |||
|
|||
<channel-type id="zoneAlarmTimeStamp"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<channel-type id="zoneAlarmTimeStamp"> | |
<channel-type id="zoneAlarmTimestamp"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -461,6 +461,14 @@ | |||
</state> | |||
</channel-type> | |||
|
|||
<channel-type id="zoneAlarmTimeStamp"> | |||
<item-type>DateTime</item-type> | |||
<label>Zone TimeStamp</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<label>Zone TimeStamp</label> | |
<label>Zone Alarm Timestamp</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -285,6 +285,8 @@ channel-type.openwebnet.zoneAlarm.state.option.SILENT = Silent | |||
channel-type.openwebnet.zoneAlarm.state.option.TECHNICAL = Technical | |||
channel-type.openwebnet.zoneAlarm.state.option.TECHNICAL_RESET = Technical Reset | |||
channel-type.openwebnet.zoneAlarm.state.option.NONE = None | |||
channel-type.openwebnet.zoneAlarmTimestamp.label = Zone Alarm TimeStamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
channel-type.openwebnet.zoneAlarmTimestamp.label = Zone Alarm TimeStamp | |
channel-type.openwebnet.zoneAlarmTimestamp.label = Zone Alarm Timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
<item-type>DateTime</item-type> | ||
<label>Zone TimeStamp</label> | ||
<label>Zone Alarm TimeStamp</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<label>Zone Alarm TimeStamp</label> | |
<label>Zone Alarm Timestamp</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -270,14 +264,13 @@ private void updateZoneAlarm(WhatAlarm w) { | |||
|
|||
private void updateZoneAlarmTimeStamp() { | |||
ZonedDateTime now = ZonedDateTime.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ZonedDateTime now = ZonedDateTime.now(); |
@@ -270,14 +264,13 @@ private void updateZoneAlarm(WhatAlarm w) { | |||
|
|||
private void updateZoneAlarmTimeStamp() { | |||
ZonedDateTime now = ZonedDateTime.now(); | |||
ChannelUID cuid = new ChannelUID(getThing().getUID(), CHANNEL_ALARM_ZONE_TIMESTAMP); | |||
updateState(cuid, new DateTimeType(now.truncatedTo(ChronoUnit.SECONDS))); | |||
updateState(CHANNEL_ALARM_ZONE_ALARM_TIMESTAMP, new DateTimeType(now.truncatedTo(ChronoUnit.SECONDS))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateState(CHANNEL_ALARM_ZONE_ALARM_TIMESTAMP, new DateTimeType(now.truncatedTo(ChronoUnit.SECONDS))); | |
updateState(CHANNEL_ALARM_ZONE_ALARM_TIMESTAMP, new DateTimeType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 266 is unuseful at this point so I would remove it and as a consequence would be updateZoneTimeStamp() implementation still convenient? I could simply invoke updateState(CHANNEL_ALARM_ZONE_ALARM_TIMESTAMP, new DateTimeType()) directly in updateZone()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done as per previous comment
| `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`) | R | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe spaces? Copy paste the original lines from original version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you so much! |
Please add migration instructions for the new channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never used such migration feature, but I think that:
- directory is wrong
- < label > should be the new channel label, not its description
- according to instructions this is also needed that "In addition to the update instructions, the thing-type definition needs to add a property thingTypeVersion to prevent newly created things from being modified"
@mvalla I see, I will check in accordance with your tips.
So do I, that is completely new to me |
@mvalla I fulfilled all of the three points you mentioned above. Moreover I validated update.xml for correctness |
bundles/org.openhab.binding.openwebnet/src/main/resources/OH-INF/thing/channels.xml
Show resolved
Hide resolved
...et/src/main/java/org/openhab/binding/openwebnet/internal/handler/OpenWebNetAlarmHandler.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.openwebnet/src/main/resources/OH-INF/update/update.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.openwebnet/src/main/resources/OH.INF/update/update.xml
Outdated
Show resolved
Hide resolved
Do you mean I should remove that line,right? I removed it and validate update.xml with no errors |
You have update.xml twice in 2 different folders: OH-INF/update and OH.INF/update. Only the first is needed. |
You are right My local branch does not show OH.INF update whereas the origin does. lets try push it |
I am striving to get rid of the wrong OH.INF dir from my origin even by forcing push... I will handle this problem |
ok OH.INF dir has gone now!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you
DCO KO. |
This reverts commit 3807c80. Signed-off-by: Giovanni Fabiani <[email protected]>
DTO is ok now.. tommorow I will do further integration-tests, waiting for @mvalla review |
@lolodomo I appreciated your support! Thanks a lot |
Do you mean the PR is not yet ready for a merge? |
Yes, Exactly I prefer to perform a couple of integration tests tomorrow so if everything works as expected and @mvalla agrees, on saturday the PR will be ready for merge! |
Current code looks good to me. |
@lolodomo, i did tests. Good feedbacks! To me the PR is ready for merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you
Thanks again See you soon! |
Hi lolodomo! After the merging of my PR I am not able to compile the binding anymore. Even Cloning from Main repo! This is the message
[WARNING] Could not transfer metadata org.openhab:openhab-super-pom/maven-metadata.xml from/to github (https://maven.pkg.github.com/fabgio/14535_openwebnet_4.0.0_publish): authentication failed for https://maven.pkg.github.com/fabgio/14535_openwebnet_4.0.0_publish/org/openhab/openhab-super-pom/maven-metadata.xml, status: 401 Unauthorized
It looks that maven looks for an unwanted dependency… I don’t know where it comes from I need help.. I am terrified!!!
Please help!!
From: lolodomo ***@***.***>
Sent: giovedì 4 maggio 2023 20:34
To: openhab/openhab-addons ***@***.***>
Cc: Giovanni Fabiani ***@***.***>; Mention ***@***.***>
Subject: Re: [openhab/openhab-addons] [openwebnet] Add a time stamp when an alarm zone event occurs (PR #14819)
tommorow I will do further integration-tests, waiting for @mvalla <https://github.com/mvalla> review
Do you mean the PR is not yet ready for a merge?
—
Reply to this email directly, view it on GitHub <#14819 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZHU6DATRC2KFU6T4RXBDLXEPZCFANCNFSM6AAAAAAW7QR7QU> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/ACZHU6AVJSQVA55QKYECUELXEPZCFA5CNFSM6AAAAAAW7QR7QWWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTS3QG6KA.gif> Message ID: ***@***.*** ***@***.***> >
|
This reverts commit 3807c80. Signed-off-by: Giovanni Fabiani <[email protected]> Signed-off-by: Thomas Burri <[email protected]>
This reverts commit 3807c80. Signed-off-by: Giovanni Fabiani <[email protected]> Signed-off-by: Matt Myers <[email protected]>
This reverts commit 3807c80. Signed-off-by: Giovanni Fabiani <[email protected]> Signed-off-by: Jørgen Austvik <[email protected]>
Description:
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
Test
Tested on BTicino/Legrand bulgrar-alarm unit 3486