Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial support for LifeControl enviroment sensor #1964

Merged
merged 5 commits into from
Oct 22, 2019
Merged

Add initial support for LifeControl enviroment sensor #1964

merged 5 commits into from
Oct 22, 2019

Conversation

bastshoes
Copy link
Contributor

This PR adds initial support for LifeControl enviroment sensor.

@manup, @ebaauw I would like to ask you to take a look and guide me as I'm not sure that I'm moving in right direction.
Sensor stores all meaningful data in TEMPERATURE MEASUREMENT CLUSTER. You can see on picture below
image

End point 0x0000 is temperature and 0x0001 is humidity.

@manup
Copy link
Member

manup commented Oct 14, 2019

HI thanks for the PR. I don't understand the changes to updateSensorNode() shouldn't the existing implementation for Temperature and Humidity cluster already work?

@bastshoes
Copy link
Contributor Author

bastshoes commented Oct 15, 2019

Hello Manuel.

Usefull data stored in temperature cluster endpoint 0x0000 and 0x0001. If I understand code correct, on update only 0x0000 will be read. I'm wrong?

Copy link
Member

@manup manup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok thanks, I do understand now. I've put some suggestions in the PR code to prevent code duplication and only express special handling for the humidty attribute. Can you please check if thats correct?

Comment on lines 3872 to 3882
if (modelId.startsWith(QLatin1String("VOC_Sensor"))
{
if (i->endpoint() == 0x00)
{
fpTemperatureSensor.inClusters.push_back(ci->id());
}
else if (i->endpoint() == 0x01)
{
fpHumiditySensor.inClusters.push_back(ci->id());
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a simpler variant in order to only express that the humidity sensor values are kept in the temperature cluster

Something like:

if (modelId.startsWith(QLatin1String("VOC_Sensor") && i->endpoint() == 0x01)
{
    // humidity sensor values are transferred via temperature cluster 0x0001 attribute
    // see: https://github.com/dresden-elektronik/deconz-rest-plugin/pull/1964
    fpHumiditySensor.inClusters.push_back(ci->id());
}
else
{
    fpTemperatureSensor.inClusters.push_back(ci->id());
}

@@ -5718,34 +5747,99 @@ void DeRestPluginPrivate::updateSensorNode(const deCONZ::NodeEvent &event)
{
for (;ia != enda; ++ia)
{
if (ia->id() == 0x0000) // temperature (0.01 °C)
if (i->modelId().startsWith(QLatin1String("VOC_Sensor")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same suggestion as above, currently the code of the temperature attribute 0x0000 is duplicated.
I think this can be simplified by just adding the special humidity attribute 0x0001 handling code in an else if (), so that the VOC_Sensor can use the common temperature cluster 0x0000 attribute code.

if (ia->id() == 0x0000) // temperature
{
   // ...
}
else if (i->modelId().startsWith(QLatin1String("VOC_Sensor")) && ia->id() == 0x0001)
{
    // humidity sensor values are transferred via temperature cluster 0x0001 attribute
    // see: https://github.com/dresden-elektronik/deconz-rest-plugin/pull/1964
   
   // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @manup!
I've changed code according to your suggestions.
Could you please review it.

@bastshoes bastshoes changed the title WIP: Add initial support for LifeControl enviroment sensor Add initial support for LifeControl enviroment sensor Oct 22, 2019
manup added a commit that referenced this pull request Oct 22, 2019
@manup manup merged commit 0436805 into dresden-elektronik:master Oct 22, 2019
@manup
Copy link
Member

manup commented Oct 22, 2019

Thanks, the PR is merged now. I've added some braces to let it compile and static_cast<> for clang datatype warning.

@bastshoes bastshoes deleted the MCLH-08 branch October 22, 2019 09:25
@bastshoes
Copy link
Contributor Author

Hello @SwoopX!
Could you please do me a favor and check my code. I can't make sensor work. It's not appear in rest api.
Endpoint details can be found here: #1540

Logs for join process posted here:
#2032

@SwoopX
Copy link
Collaborator

SwoopX commented Apr 29, 2020

@bastshoes I'll continue on #2032

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants