[avmfritz] Fixed NPE if temperature, powermeter or switch model is null #1887

cweitkamp · 2017-02-12T21:07:24Z

Added some more debug loggings
Added additional xml-element tags for creating objects by JAXBContext
Refactoring the use of JAXBContext as singleton instance to prevent memory leaks

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

mention-bot · 2017-02-12T21:07:25Z

@cweitkamp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kaikreuzer and @robbyb67 to be potential reviewers.

sjsf

apart from some really minor comments the only thing which I can see prevents this PR to be merged is that it needs a rebase.

sjsf · 2017-04-05T14:10:03Z

....openhab.binding.avmfritz/src/main/java/org/openhab/binding/avmfritz/handler/BoxHandler.java

@@ -109,6 +110,7 @@ public void initialize() {
 public void dispose() {
 logger.debug("Handler disposed.");
 if (pollingJob != null && !pollingJob.isCancelled()) {
+ logger.debug("stop polling job");


here something with the indentation went wrong.

sjsf · 2017-04-05T14:10:41Z

.../main/java/org/openhab/binding/avmfritz/internal/hardware/callbacks/FritzAhaXMLCallback.java

+import org.openhab.binding.avmfritz.internal.hardware.FritzahaWebInterface;
+
+/**
+ * 


some words here would be nice

sjsf · 2017-04-05T14:10:51Z

.../main/java/org/openhab/binding/avmfritz/internal/hardware/callbacks/FritzAhaXMLCallback.java

+ try {
+ jaxbc = JAXBContext.newInstance(DevicelistModel.class);
+ } catch (JAXBException e) {
+ // TODO


I want to add a debug output here, but the method is static and I have no clue how to access the logger from a static methods.

Oh, I see. That's actually not much different than usual, just use the LoggerFactory:

LoggerFactory.getLogger(FritzAhaXMLCallback.class).debug(/*...*/);

@SJKA I assume we are not going to fetch the logger each time but just have a static logger in this case:

As we are in a dynamic OSGi environment, loggers should be non-static, when ever possible and have the name logger.

cweitkamp · 2017-04-06T11:05:57Z

@SJKA Thanks for your review. I applied your changes.

sjsf · 2017-04-11T13:17:06Z

Thanks, looks good to me.
@robbyb67 do you want to have a second look since you wrote the binding?

martinvw

Thanks for your changes I added some remarks/questions.

martinvw · 2017-05-24T07:14:26Z

.../main/java/org/openhab/binding/avmfritz/internal/hardware/callbacks/FritzAhaXMLCallback.java

+ try {
+ jaxbc = JAXBContext.newInstance(DevicelistModel.class);
+ } catch (JAXBException e) {
+ LoggerFactory.getLogger(FritzAhaXMLCallback.class).error("{}", e.getMessage(), e);


Better fetch the logger only once and add a meaningful message before quoting the exception its message.

I cannot fetch the logger only once here. It is used in a static method. This is the way to go in this case.

No for this case a static logger is allowed and accepted :-)

Sry to ask again: Are you sure? Have a look at this comment. @SJKA recommended me to do so. And I followed this discussion yesterday - ok, it was in ESH repo, but maybe it ist true for this repo.

No problem yes I am sure :-)

See also eclipse-archived/smarthome#3521 (comment)

martinvw · 2017-05-24T07:14:56Z

.../main/java/org/openhab/binding/avmfritz/internal/hardware/callbacks/FritzAhaXMLCallback.java

+ */
+ public static final JAXBContext jaxbContext = initContext();
+
+ /**


If you are docblock give this amount of information better leave them out.

martinvw · 2017-05-24T07:16:59Z

.../main/java/org/openhab/binding/avmfritz/internal/hardware/callbacks/FritzAhaXMLCallback.java

+ * @author Christoph Weitkamp
+ * 
+ */
+public class FritzAhaXMLCallback extends FritzAhaReauthCallback {


What actually changes by adding this class in the hierarchy, it only has a static field which should/could also be called explicitly without extending etc.

If you have a look at our last conversation here I hope you will understand why I added this new class. ;-)

Yes but still you are actually only referring to some static field you don't need to make it part of the class hierarchy in that case?

Ok. What should I do? Remove it and add the static field to the extended class FritzAhaReauthCallback?

Okay so your goal is to have a place to store the static final JAXB_CONTEXT if there is no proper location I'm okay with creating a dedicated class for it (most likely better than putting it in FritzAhaReauthCallback ) but just make it a static util class and do not put as a parent class to make static field accessible.

Please note that it is also not a callback, so make it live in another package.

Edit: added missing not which does change the message ;-)

martinvw · 2017-05-24T07:17:56Z

...java/org/openhab/binding/avmfritz/internal/hardware/callbacks/FritzAhaDiscoveryCallback.java

-public class FritzAhaDiscoveryCallback extends FritzAhaReauthCallback {
+public class FritzAhaDiscoveryCallback extends FritzAhaXMLCallback {
+ /**
+ * logger


I'm no fan of comments which add no value, everyone could have guessed this is a logger :-P

martinvw · 2017-05-24T07:18:57Z

.../main/java/org/openhab/binding/avmfritz/internal/hardware/callbacks/FritzAhaXMLCallback.java

+ /**
+ * JAXBContext
+ */
+ public static final JAXBContext jaxbContext = initContext();


This is a constant, please follow the naming conventions and uppercase it.

martinvw · 2017-05-26T21:05:48Z

@cweitkamp is it clear now what changes I expect?

Please do not forget to mention us if you force push, because we will not be notified by GitHub in that case

cweitkamp · 2017-05-27T05:12:25Z

@martinwv Yes, very clear. I will need a couple of days to apply those changes. Thanks for your support.

martinvw · 2017-05-27T08:55:34Z

Np you are welcome

- Added some more debug loggings - Added additional xml-element tags for creating objects by JAXBContext - Refactoring the use of JAXBContext as singleton instance to prevent memory leaks Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cweitkamp · 2017-05-29T19:41:17Z

@martinvw I did it. Do you want to have a look?

kaikreuzer · 2017-05-29T20:59:47Z

Thanks everyone!

…#1887) - Added some more debug loggings - Added additional xml-element tags for creating objects by JAXBContext - Refactoring the use of JAXBContext as singleton instance to prevent memory leaks Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

* master: (23 commits) Fixes for setting brightness and colour temperature for V3 white bulbs. (openhab#2338) [RFXCOM] Add new protocols (openhab#2331) [RFXCOM] Add support for UV sensors (openhab#2330) Removed the unsupported MACROMAN encoding (openhab#2332) Create Issue and PR template files (openhab#2285) Corrected the discovery of new devices (openhab#2326) Fixes ./, decimal point issue in volume (openhab#2297) Fix table layout feed binding (openhab#2318) When trying to send a message while the bridge was not connected an NPE occurred (openhab#2299) D-Link Smart Home device binding (openhab#2035) Updated README.md, Changed definition of 'battery_low' channel to sytemwide channel type 'system.low-battery' (openhab#2262) Tesla : Move Event Stream to a separate Thread to speed up processing + discard events that are too old to be relevant + clean-up constants (openhab#2307) Fixed NPE if temperature, powermeter or switch model is null (openhab#1887) [DSCAlarm] Various Fixes and Enhancements (openhab#2313) [globalcache] Implement bi-directional support for serial devices (openhab#2311) Keba : Fix kecontact.xml in ESH-INF (openhab#2314) Keba : Fixes openhab#2237 openhab#2236 openhab#2234 openhab#2038 (openhab#2270) Fixed typo in configuration of channel-type-id (openhab#2310) Update native AllJoyn library for linux x86 and x64 (openhab#2308) [kodi] Added channels for opening PVR TV or Radio streams (openhab#2084) ...

…#1887) - Added some more debug loggings - Added additional xml-element tags for creating objects by JAXBContext - Refactoring the use of JAXBContext as singleton instance to prevent memory leaks Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cweitkamp changed the title ~~Fixed NPE if temperature, powermeter or switch model is null (see issue #1885)~~ [avmfritz] Fixed NPE if temperature, powermeter or switch model is null (see issue #1885) Feb 14, 2017

cweitkamp mentioned this pull request Feb 15, 2017

[avmfritz] 2 FRITZ!DECT 200 NullPointerException because PowerMeterModel missing #1885

Closed

sjsf requested changes Apr 5, 2017

View reviewed changes

cweitkamp force-pushed the bugfix-1885 branch from e203cce to ee78bb5 Compare May 13, 2017 07:07

martinvw reviewed May 24, 2017

View reviewed changes

cweitkamp force-pushed the bugfix-1885 branch from ee78bb5 to a8ddc72 Compare May 24, 2017 09:18

cweitkamp force-pushed the bugfix-1885 branch from a8ddc72 to ce7f369 Compare May 29, 2017 19:38

martinvw approved these changes May 29, 2017

View reviewed changes

martinvw assigned kaikreuzer May 29, 2017

martinvw requested a review from kaikreuzer May 29, 2017 19:53

kaikreuzer merged commit 3fc0307 into openhab:master May 29, 2017

cweitkamp deleted the bugfix-1885 branch May 29, 2017 21:04

martinvw changed the title ~~[avmfritz] Fixed NPE if temperature, powermeter or switch model is null (see issue #1885)~~ [avmfritz] Fixed NPE if temperature, powermeter or switch model is null Jun 25, 2017

martinvw added this to the 2.1 milestone Jun 25, 2017

martinvw added the bug An unexpected problem or unintended behavior of an add-on label Jun 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[avmfritz] Fixed NPE if temperature, powermeter or switch model is null #1887

[avmfritz] Fixed NPE if temperature, powermeter or switch model is null #1887

cweitkamp commented Feb 12, 2017 •

edited

Loading

mention-bot commented Feb 12, 2017

sjsf left a comment

sjsf Apr 5, 2017

sjsf Apr 5, 2017

sjsf Apr 5, 2017

cweitkamp Apr 5, 2017 •

edited

Loading

sjsf Apr 5, 2017

martinvw May 24, 2017

cweitkamp commented Apr 6, 2017

sjsf commented Apr 11, 2017

martinvw left a comment

martinvw May 24, 2017

cweitkamp May 24, 2017

martinvw May 24, 2017

cweitkamp May 24, 2017

martinvw May 24, 2017

martinvw May 26, 2017

martinvw May 24, 2017

martinvw May 24, 2017

cweitkamp May 24, 2017

martinvw May 24, 2017

cweitkamp May 24, 2017

martinvw May 24, 2017 •

edited

Loading

martinvw May 24, 2017

martinvw May 24, 2017

martinvw commented May 26, 2017

cweitkamp commented May 27, 2017

martinvw commented May 27, 2017

cweitkamp commented May 29, 2017

kaikreuzer commented May 29, 2017

[avmfritz] Fixed NPE if temperature, powermeter or switch model is null #1887

[avmfritz] Fixed NPE if temperature, powermeter or switch model is null #1887

Conversation

cweitkamp commented Feb 12, 2017 • edited Loading

mention-bot commented Feb 12, 2017

sjsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweitkamp Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweitkamp commented Apr 6, 2017

sjsf commented Apr 11, 2017

martinvw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw commented May 26, 2017

cweitkamp commented May 27, 2017

martinvw commented May 27, 2017

cweitkamp commented May 29, 2017

kaikreuzer commented May 29, 2017

cweitkamp commented Feb 12, 2017 •

edited

Loading

cweitkamp Apr 5, 2017 •

edited

Loading

martinvw May 24, 2017 •

edited

Loading