[Homematic] Data point enumeration potentially incorrect for HM-MOD-EM-8

maniac103 commented 6 years ago

Bug description

When having two (or more) HM-MOD-EM-8 whose switches are configured to differing button modes (e.g. some are configured to button mode, others to sensor mode), the data points of the one discovered last are enumerated incorrectly: the data points of the channels of the first instance are inherited all other instances, even though switching modes causes different data points to appear

For sensor mode, result of getParamsetDescription for VALUES is this (fetched with https://github.com/hobbyquaker/binrpc)

{
  "INSTALL_TEST": {
    "FLAGS": 3,
    "ID": "INSTALL_TEST",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 4,
    "TAB_ORDER": 0,
    "TYPE": "ACTION",
    "UNIT": ""
  },
  "LOWBAT": {
    "CONTROL": "NONE",
    "FLAGS": 1,
    "ID": "LOWBAT",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 5,
    "TAB_ORDER": 1,
    "TYPE": "BOOL",
    "UNIT": ""
  },
  "STATE": {
    "CONTROL": "DOOR_SENSOR.STATE",
    "FLAGS": 1,
    "ID": "STATE",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 5,
    "TAB_ORDER": 2,
    "TYPE": "BOOL",
    "UNIT": ""
  }
}

while in button mode, it's this:

{
  "INSTALL_TEST": {
    "FLAGS": 3,
    "ID": "INSTALL_TEST",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 4,
    "TAB_ORDER": 0,
    "TYPE": "ACTION",
    "UNIT": ""
  },
  "PRESS_CONT": {
    "FLAGS": 3,
    "ID": "PRESS_CONT",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 4,
    "TAB_ORDER": 1,
    "TYPE": "ACTION",
    "UNIT": ""
  },
  "PRESS_LONG": {
    "CONTROL": "BUTTON.LONG",
    "FLAGS": 1,
    "ID": "PRESS_LONG",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 4,
    "TAB_ORDER": 2,
    "TYPE": "ACTION",
    "UNIT": ""
  },
  "PRESS_LONG_RELEASE": {
    "FLAGS": 3,
    "ID": "PRESS_LONG_RELEASE",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 4,
    "TAB_ORDER": 3,
    "TYPE": "ACTION",
    "UNIT": ""
  },
  "PRESS_SHORT": {
    "CONTROL": "BUTTON.SHORT",
    "FLAGS": 1,
    "ID": "PRESS_SHORT",
    "MAX": true,
    "MIN": false,
    "OPERATIONS": 4,
    "TAB_ORDER": 4,
    "TYPE": "ACTION",
    "UNIT": ""
  }
}

The caching done at https://github.com/openhab/openhab2-addons/blob/2.1.0/addons/binding/org.openhab.binding.homematic/src/main/java/org/openhab/binding/homematic/internal/communicator/AbstractHomematicGateway.java#L400 only takes device type, firmware version and channel number into account, which is the same for all devices.

Possible Solution

Not sure whether there's a proper way to detect such channels with 'dynamic' data points. If not, caching should be disabled for HM-MOD-EM-8 devices.

Your Environment

In my case, I have one device where 4 channels are configured to sensor mode and another one where all 8 channels are configured to button mode: in that scenario, the data points of the former device are also used for the latter device, which means I can't use the button events of the first 4 channels of the latter device.

maniac103 commented 6 years ago

Damn, with #2932 the wrong caching is now fixed, but using multiple HM-MOD-EM-8 in differing configurations still doesn't work because the datapoints-to-thing-channels conversion is cached via the ThingType interface as well [1]. The referenced thingTypeUID only includes the device name, thus doesn't account for the different data point sets. From my log:

2017-12-23 21:34:54.131 [DEBUG] [ommunicator.AbstractHomematicGateway] - Loaded device 'MEQ0783017' (HM-MOD-EM-8) with 87 datapoints
[...]
2017-12-23 21:34:54.161 [DEBUG] [rnal.type.HomematicTypeGeneratorImpl] - Generating ThingType for device 'HM-MOD-EM-8' with 87 datapoints
[...]
2017-12-23 21:34:54.434 [DEBUG] [ommunicator.AbstractHomematicGateway] - Loaded device 'MEQ0783008' (HM-MOD-EM-8) with 100 datapoints

The logic in [2] should probably account for the CHANNEL_FUNCTION data point being present, but I'm not sure how exactly ... add the serial number to the UID? Add the values of the CHANNEL_FUNCTION data points? @gerrieg Any ideas/comments?

[1] https://github.com/openhab/openhab2-addons/blob/24b805c7a8216ac14b64e2ec61cbc21ff0855f3d/addons/binding/org.openhab.binding.homematic/src/main/java/org/openhab/binding/homematic/internal/type/HomematicTypeGeneratorImpl.java#L127 [2] https://github.com/openhab/openhab2-addons/blob/24b805c7a8216ac14b64e2ec61cbc21ff0855f3d/addons/binding/org.openhab.binding.homematic/src/main/java/org/openhab/binding/homematic/internal/type/UidUtils.java#L40

gerrieg commented 6 years ago

I already expected that. It's currently not supported to have several identical devices with different channel configurations. But I have already thought about it and i will implement a thingTypeKey in the thing configuration for such devices. Then it should work.

maniac103 commented 6 years ago

That's great, thanks. I should probably have tested it properly, but only had remote access to my server (and thus no access to the actual HM device) when I tested my patch :-/

maniac103 commented 6 years ago

@gerrieg I looked at how one would implement your idea, but I'm not yet sure I fully understand it yet. Could you please tell me what I'm missing? My understanding is this:

discovery is started (HomematicDeviceDiscoveryService::loadDevices)
this causes metadata to be fetched (AbstractHomematicGateway::loadAllDeviceMetadata), which causes devices and channels to be initialized and their datapoints to be fetched (but those don't have their values initialized for now)
the same method calls HomematicBridgeHandler::onDeviceLoaded, which in turn calls HomematicTypeGeneratorImpl::generate
at this point, the ThingTypeUID is generated and used for ThingType lookup: if the thing type for the given UID is created already, use that for the device, else create a new one
HomematicBridgeHandler::onDeviceLoaded calls HomematicDeviceDiscoveryService::deviceDiscovered, which generates a ThingUID from ThingTypeUID, bridge thing UID and device serial number, ultimately leading to the device being listed in inbox
when adding the discovered device via inbox, a new Thing is created, using the previously determined ThingTypeUID and ThingUID
at some later point in time (scheduled via scheduler), ThingManager creates the HomematicThingHandler for the newly generated item
when those are initialized (HomematicThingHandler::initialize), they schedule a runnable that fetches the datapoint values and initializes the thing configuration for each thing

Is that understanding correct? If yes, it's not clear to me how adding something to the thing configuration (in the last step) helps in determining the ThingType (which in turn determines the available channels), as that happens much earlier. What am I missing there?

(NB: https://gist.github.com/maniac103/f3718a950167b32a4a280af86b21d20b will probably work, but it feels somewhat hacky :-/ - so if you could give a short outline of your idea, I'm happy to implement it)

gerrieg commented 6 years ago

Is that understanding correct

Yes, that's correct

You are right, my first idea to use a Thing configuration for the ThingTypeUID generation does not work. I don't think openhab is designed for something like this, that one device type can have several different channel configurations. But maybe i'm wrong, we have to ask the core developers whether there is a possibility.

maniac103 commented 6 years ago

@kaikreuzer Do you have any comments/ideas about the problem outlined above? TL,DR version: Is there an officially blessed way of dealing with devices that, depending on their configuration, can have varying sets of ChannelDefinitions?

kaikreuzer commented 6 years ago

@maniac103 Do points (3) and (5) of the FAQs answer your question or is it something slightly different?

maniac103 commented 6 years ago

@kaikreuzer It kinda does. As I read it, the solution for our problem is to filter the channel definitions of the channels that can appear/disappear depending on configuration and only keep those channels in the ThingType that are known to be static. Correct?

@gerrieg As per the above, we'd need a way to find out what data points depend on the configuration. I guess this will be hard, right? However, item 4 from the FAQ above matches our scenario pretty well, but seems to imply that one can cover different sets of channels with one ThingType. Maybe omitting the channel definitions from the ThingType for devices with CHANNEL_CONFIGURATION data points and adding them via ThingHelper::addChannelsToThing() in HomematicThingHandler::initialize() would be a working approach?

EDIT: I guess it would be, if ThingHelper was API available for bindings ... @kaikreuzer Are you aware of any example that implements item 4 of the FAQ?

kaikreuzer commented 6 years ago

Correct?

Yes, only define the static ones in the type and add the rest dynamically when appropriate.

Are you aware of any example that implements item 4 of the FAQ?

Actually no. But I came across this question myself recently when working on the Bluetooth binding and solved it this way. We could possibly check whether some more straight-forward API could be offered for such a situation.

maniac103 commented 6 years ago

But I came across this question myself recently when working on the Bluetooth binding and solved it this way.

Thanks for that pointer, a similar solution should work here as well -> #3140

kaikreuzer commented 6 years ago

FTR, I have created https://github.com/eclipse/smarthome/issues/4950 to have a simplified API for this task in future.

openhab / openhab-addons