search

fruggy83 / openocean

27 stars 12 forks source link

Add support for Multi Function Sensors (EPP family D2-14) #73

Closed nimric closed 4 years ago

nimric commented 5 years ago

Hello Daniel,

it would be nice if the binding supports Multi Function Sensors like the INSAFE+ Origin which uses EPP D2-14-30, but I am not sure how much effort it is to add a new thing and the EPPs.

fruggy83 commented 5 years ago

No big deal, I will try to implement it with #71. By the way nice device. Do you own such a thing and could you support in testing?

nimric commented 5 years ago

Nice to hear that you can do this. I own such a device and can support you.

nimric commented 5 years ago

Hi Daniel,

Since I am going to buy some more device in the future that are not supported yet, I like to contribute to this project a bit. Therefore I created a fork and started with the implementation for the Multi Function Sensors. I did the following so far:

As far as I can see I need to implement the EEPs in the new package D2_14 next. Am I right, that the following methods are the important ones:

Please let me know if there is anything else I need to implement to get this working.

fruggy83 commented 5 years ago

Hi @nimric,

thanks for contributing. You are right, these functions are the important one to implement. However as this device is a sensor, it is ok to just implement the convertToStateImpl function. The other ones do not need to be implemented and can use the standard implementation (do nothing).

Best regards Daniel

nimric commented 5 years ago

Hello Daniel, can you help me with setting up the project? I have issues to resolve the parent POM.

fruggy83 commented 5 years ago

Hi, which IDE do you use? I would suggest to use the latest commit from my repo. It contains the neccessary adjustments for the new build system. I am using the following setup

Just adjust the url to your fork. Good luck!

fruggy83 commented 5 years ago

Hi @nimric ,

how are you doing? As another user requested support for a smoke detector too, I have already implemented EEP F6-05-02. You find my implementation in branch issue71.

Best regards Daniel

nimric commented 5 years ago

Hello @fruggy83,

I am doing well, but I had no time to continue my implementations due to my work. However, your commit shows me that I did the right things so far. I have got 2 questions:

F6-05 is “defined” as Detector in the EEP while D2-14 is “defined” as Multi Function Sensor.

fruggy83 commented 5 years ago

Hi @nimric,

thats sounds good 👍

channels.xml A little background: As you can see in the EnOceanBaseThingHandler.updateChannels() the channels of a thing are created dynamically based on the choosen receiving and sending EEPs. However the properties of these dynamically created channels are merged with the properties given in the channels.xml. So for example an i18n label can be simple added to the channels.xml and I just need to specifiy the channelUID and itemtype in my EnOceanChannelDescription datastructure in moste cases. If I would not be so lazy I could completely avoid the channels.xml. However when I started to implement the binding the ChannelBuilder API was not so powerful as it is now. Now to you question: Channels which are not mentioned in channels.xml are system channels which are defined in the openhab core. Therefore I do not need to define them in my channels.xml. Just my own channels go there.

Multi Function Sensor When I started to implement the binding I asked myself the same question. Should I create the thingtypes acccordingly to the EEP family or accordingly their functionality. I choose the later way to reduce the number of thingtypes. The Multi Function Sensor is a good example for not using the EEP family as a thingtype name. This name is also used for an 4BS EEP A5-14. However in this case it is used for a window handle with additional sensors. So from user perspective it is more intuitive to add A5-14 EEP to the Window Handle thingtype. The Multi Function Sensor should contain all EEPs regarding smoke detection. So maybe we rename it better to Smoke Detector.

Best regards Daniel

nimric commented 5 years ago

Hello @fruggy83,

now I see the issue regarding the naming. A5-14 and D2-14 are both Multi Function Sensors according to the EEP. So would you suggest to add D2-14-30 to Multi Function Sensor/Smoke Detector as well as to Environmental Sensor for example?

fruggy83 commented 5 years ago

HI @nimric,

I would add D2-14-30 just to the F6-05. I like the name multi function smoke detector 👍 So just rename my multi function sensor thing to "multi function smoke detector" and add D2-14-30 as another option for receivingEEP.

Best regards Daniel

nimric commented 5 years ago

Hello @fruggy83,

I made some progress with the implementation in the last days but I have some questions regarding the "building blocks" of the telegrams. The blocks (like "TMP8" or "IAQCO") should be mapped to channels, according to my understanding. Furthermore I need to convert the content of the blocks to a state (type). While it's straight forward for blocks like TMP8, I am not sure what the right approach is for blocks like "HCI" or "SMA".

Let’s start with “HCI” since it’s the simpler one. It’s 2 bits and defined as enum with 4 different values. Should I create a new PrimitiveType like the OnOffType or should I map it to StringType?

Other blocks like “SMA” with 13 bits are way more complex and contain different information while the block itself is a logical unit. Is here a ComplexType the way to go?

fruggy83 commented 5 years ago

Hi @nimric,

The blocks (like "TMP8" or "IAQCO") should be mapped to channels

correct 👍

Let’s start with “HCI”

I would create a Number channel with a fixed number of options. Like so

<channel-type id="hci">
   <item-type>Number</item-type>
   <label>Hygrothermal Comfort Index</label>
   <state readOnly="true">
       <options>
         <option value="0">Good</option>
         <option value="1">Medium</option>
         <option value="2">Bad</option>
         <option value="3">Error</option>
      </options>
   </state>
</channel-type>

In this way you can just update the item state with the received value. In PaperUI the number value is mapped to the corresponding value as a string (like a drop down list).

Other blocks like “SMA” with 13 bits are way more complex and contain different information while the block itself is a logical unit.

It is true that this is a logical unit, however for the user it is probably the best to seperate the different information into individual (switch) channels. Like "temperature warning" => Off stands for OK, On for NOK. So whenever such a channel switches to ON (smoke, temp, humidity etc) the user can react through a simple rule to it. However this is just my humble opinion. I am open for any suggestions 😄

fruggy83 commented 4 years ago

PR in official repo created. Hence issue can be closed

fruggy83 commented 4 years ago

Hi @nimric, I finished the implementation for EEP D2-14-30 in the official binding. Would be great if you could test it and give me a feedback. Best regards Daniel

nimric commented 4 years ago

Hello @fruggy83,

Thanks for the time you put into this. Unfortunately mine was short in last couple of months. I looked over the changes you did and I spotted out some things. First of all you use the wrong factor for scaling the temperature. It should be 1/5 and not 1/50. Other than that I noticed that you don’t check for values that indicate an error or which are reserved (SMA, RPLT, TMP8, HUM and IAQTH).

Is it possible to provide me the jar of the addon with your changes for testing, since I am not able to build it on my own at the moment?

fruggy83 commented 4 years ago

Hi @nimric ,

thanks for your review. I fixed the scaling factor, added new channels and ignore reserved and error values. However in my current implementation I am not really able to return an error state. So I am hoping that in this case the sensor analysis channels (SMA) are set.

You find bellow the compiled jar for the PR.

org.openhab.binding.enocean-2.5.2-SNAPSHOT.jar.zip