Currently we have the following piece of code in the message implementations of the RFXCom binding:
public Object convertSubType(String subType) throws RFXComException {
for (SubType s : SubType.values()) {
if (s.toString().equals(subType)) {
return s;
}
}
// try to find sub type by number
try {
return SubType.values()[Integer.parseInt(subType)];
} catch (Exception e) {
throw new RFXComException("Unknown sub type " + subType);
}
}
My problem with this piece of code is that catch clause catches two exception of which at least one should never be caught, ie ArrayIndexOutOfBoundsExceptoin and the other one is the NumberFormatException.
A possible suggestion could be to use the (recently added) static factory of the enum which returns unknown for unsupported values. The code could look like this then:
public Object convertSubType(String subType) throws RFXComException {
for (SubType s : SubType.values()) {
if (s.toString().equals(subType)) {
return s;
}
}
// try to find sub type by number
if (StringUtils.isNumeric(subType)){
SubType fromByte = SubType.fromByte(Integer.parseInt(subType));
if (fromByte != SubType.UNKNOWN) {
return fromByte;
}
}
throw new RFXComException("Unknown sub type " + subType);
}
We might also change the behaviour of SubType.fromByte to throw exception for unknown values or we could create two static factory methods.
My suggestion is to stop being so nice in the fromByte() method. If we can’t match the byte to a subtype, we should be throwing an exception so the caller can decide what to do about it. I would actually:
Make a new exception (RFXComUnknownSubTypeException, extending RFXComException)
Throw it from fromByte() if the sub type is unknown, instead of returning SubType.UNKNOWN
We could even remove SubType.UNKNOWN entirely, and just leave things uninitialized or null, which would also help detect errors.
Change the code here to:
// try to find sub type by number
try {
return SubType.fromByte(Integer.parseInt(subType);
catch (NumberFormatException e) {
throw new RFXComUnknownSubTypeException("Unknown sub type " + subType);
}
The danger we have here is that encodeMessage() uses fromByte(), which means instead of reporting an unknown subtype, it will throw an exception when parsing messages from RFXCom. I would say that as we’re in beta, and this behaviour is likely to help uncover cases where we are incorrectly handling unknown subtypes already, that’s actually a good thing.
Shall we throw the (existing) RFXComNotImpException from the fromByte and and make that extend the RFXComException? And throw the RFXComUnknownSubTypeException from the convertSubType.
The problem with doing this is that I just converted (upon you suggestion) to initialize everything with UNKNOWN. If we change it null some things break otherwise we have to choose some arbitrary default…
I ended up with very similar changes to you, but also went a little further and removed the unknown packet type. The only tests that broke are the basic boundary tests, I’m not sure how useful they are anyway - were they in there for a specific reason?
As for Command, Status, and other enums, I would personally do the same thing to all of them.
About the boundary test: There were some incorrect lengths implemented in the past, but we can just write better tests to fix this.
What makes me doubt the separate exceptions are for example the RFXComSecurity1Message which contains an enum Status, Contact and Motion. And some of the weather related bindings (which should be ported from OH1) even have more internal enums.
I would not prefer to give all those things a separate exception. And should you then share the exception regarding the security Status with another class which has a unrelated Status enum.
We could add a supertype and make RFXComUnknownSubTypeException and RFXComUnknownPacketTypeException extend that super type. For more specific enums the supertype can be returned and declared for the encodeMessage method.
Yes, I hadn’t realised how many different enums we had. I was starting to think along the same lines, and to use an Exception that we pass the enum into.
Should we talk about unknown value’s or unsupported?
throw new RFXComUnknownValueException(PacketType.class, String.valueOf(input))
Shouldn’t we just overload the constructor once more (with an int or an object) to overcome the duplication of calling String.valueOf on a lot of places.
I disabled the * imports in my IDE
And my ‘solution’ for the test could look like this:
I haven’t had a chance to test it with the hardware, and won’t now until the new year. I have gone in and fixed up the tests and the old docblock, however, so the tests are all green.
You’re welcome to grab my latest, test it, and submit a PR if you have the time, otherwise it can wait.
To me, that’s how it should be. If we were to respond to commands from unknown subtypes, we would really be guessing as to their meaning, it would work by accident. Imagine if the command meant something else, and our guess was wrong, then we would be broken and someone might not even know.
If the RFXCom spec can confirm that a command means the same for a certain packet type of any sub type, then we could rely on that and just produce a warning message to say “This subtype is unknown, so we are making assumptions about what this command means”. We would still need some means of recording the subtype byte that was provided, otherwise we’re losing information, which we were before when defaulting to 255.
Scrolling through the SDK documentation I see indeed that a lot of messages with multiple subtypes have commands with different meanings, however most of the time the ON/OFF are the same for all subTypes. For Temperature sensors all messages mean the same for all the 11 Temperature sensors currently supported by RFXCom.
If we just get all subtypes up-to-date for now, I’m okay with your plan.
I will maybe connect my RFXCom to my OH2 instance and do a test drive, but since my wife in the kids are also at home next week I have less time to spend.
