-
-
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
[innogysmarthome] Fix Battery Warnings (Bug #7956
Conversation
Travis tests were successfulHey @mmans, |
1 similar comment
Travis tests were successfulHey @mmans, |
I assume that "DeviceLinkList" is still used for other types of messages so shouldn't you use both then? |
Good question. It is only used in a "Message". And the only function that uses the getDeviceLinkList is the LowBattery message. So I thought it could do no harm to change it. If other messages are implemented in the future it could change. |
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 change is correct. It looks like with the change to the new innogy api 1.1 some information got lost in the Message class. While some data structures changed in the new api, the changes lost some information. Partly because the json parsing was also changed. So places that used @Key
. didn't get a @SerializedName
annotation. In this case it had the @Key("Devices")
. But besides it being renamed to the lower case devices
in the new api, the @SerializedName
annotation was not added. I see there are also to other annotations missing:
- Field
isRead
should get the annotation@SerializedName("read")
- Field
messageClass
should get the annotation@SerializedName("class")
.
The change here of the field name to devices is ok. But please do also update the javadoc and method parameter in the Message.java
class. And while you change that can you also add the 2 other annotations.
So the use of deviceLinkList is not something that will be present in json and this change should not be a problem.
Signed-off-by: Marco Mans <[email protected]>
Hi @Hilbrand Thanks for your review. I changed the things you mentioned. Regards, |
Signed-off-by: Marco Mans <[email protected]>
Travis tests were successfulHey @mmans, |
1 similar comment
Travis tests were successfulHey @mmans, |
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
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]> Signed-off-by: CSchlipp <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]> Signed-off-by: MPH80 <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]> Signed-off-by: Daan Meijer <[email protected]>
* Renamed DeviceLinkList to Devices * Updates Javadocs + Added 2 Annotations Closes openhab#6932 Signed-off-by: Marco Mans <[email protected]>
This PR fixes low battery messages.
Closes: #6932
Root cause: Json field "DeviceLinkList" of a "Message" is renamed (by Innogy) to "Devices".