[rfxcom] unknown subtypes and exceptions

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.

Any thoughts?

This discussion started in this PR: https://github.com/openhab/openhab2-addons/pull/1189

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.

Okay I can work with that, what remains than is that we have other things like the Commands, do we want to stretch it all the way to that?

The benefit is indeed than unsupported commands/states/etc. are reported as exception instead of type unknown which can also not be handled.

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…

A first attempt:


I would use a new exception, exceptions are cheap to create and make it easier when using the code to determine exactly what the problem was.

What things will break if we set it to null, or don’t initialise it?

I was just having a go at the code myself as well, to see if I could find any other problems.

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.

Have put up a new version, see if that matches what you were thinking.

If we like this version, then I’ll fix up the boundary tests as well.

Have added an invalid message type test that expects to receive the new exception type as well.

Some questions / remarks:

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 :slight_smile:

And my ‘solution’ for the test could look like this:

public void basicBoundaryCheck() throws RFXComException {
     RFXComCurtain1Message message = (RFXComCurtain1Message) RFXComMessageFactory.createMessage(PacketType.CURTAIN1);

        message.command = OPEN;
        message.subType = HARRISON;

        RFXComTestHelper.basicBoundaryCheck(PacketType.CURTAIN1, message);

I like the version as its looking now, does it work :wink:

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.

The only downside of the solution which just popped up in my head is the following.

If we do not support a certain subType for message but we do recognize its command the current code will allow people to use it (at least) for input.

The new scenario will throw an exception and will not allow usage of that device.

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.

I took your changes and made some improvements of my own as well:

I will test this version later on.

Running some tests, works great :slight_smile:

Download: http://mtin.nl/exceptions/org.openhab.binding.rfxcom-2.0.0-SNAPSHOT.jar