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

Tint remote control by Müller Light; Refactor setting light state #2849

Merged
merged 42 commits into from Jun 15, 2020
Merged

Conversation

ebaauw
Copy link
Collaborator

@ebaauw ebaauw commented May 30, 2020

Skip the _On_ command in between _Move to Level (with On/Off)_ and _Move to Level_, see #2824.
Don't remove `/groups/0`.
Add Tint remote control by Müller Light, see #1209.
Add Tint remote control by Müller Light
Add `previousCt`.
- Handle `state.xy` (for Tint remote control by Müller Light);
- Don't attach `/groups/0` to sensors.
Add Tint remote control by Müller Light, see #1209.
{
sensorNode.addItem(DataTypeUInt16, RStateX);
sensorNode.addItem(DataTypeUInt16, RStateY);
sensorNode.addItem(DataTypeUInt16, RStateTiltAngle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be RStateAngle instead of RStateTiltAngle? From my testing so far it shows "tiltangle":null in the REST API instead of the angle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, thanks for noticing!

It's correct in database.cpp. If you restart deCONZ, you should see state.angle instead.

Max `ct` value is 0xFEFF (65279), see #2475.
Max `ct` value is 0xFEFF (65279), see #2475.
Refactor logic for PUT light state, see #2475.
@ebaauw ebaauw changed the title Tint remote control by Müller Light Tint remote control by Müller Light; Refactor setting light state Jun 1, 2020
@Thomas-Vos Thomas-Vos mentioned this pull request Jun 4, 2020
@bortim
Copy link

bortim commented Jun 6, 2020

Hello @ebaauw,

when will this branch be part of an official build?
I am using deconz and api with in a docker-container on a raspberry pi.
Because of multiple missing libs I was not able to compile it directly within the container.

Regards

ebaauw added 11 commits June 6, 2020 18:55
Use values by latest Hue bridge gen-2 firmware, to re-enable Hue app, see #2837.
- Add a global variable for the `DeRestPluginPrivate` instance, to enable calling `enqeueuEvent()` (amongs others) from `LightNode` (amongst others) methods.
- Add a global variable for the `DeRestPluginPrivate` instance, to enable calling `enqeueuEvent()` (amongs others) from `LightNode` (amongst others) methods, see #2475.
- Ignore _Enhanced Current Hue_ for Müller Lights, see #1209
- Add support for Müller lights special scene, see #1209
- Add manufacturer-specific attribute for Müller Light special scene;
- Add more _Color Control_ attributes.
- Add methods to update `ResourceItem` value, including issuing event, updating `etag`, and saving to database.
- Remove `state.alert` for _Window Covering Device_;
- Make `state.effect` a true `ResourceItem`;
- Add `setValue` methods to set `ResourceItem` value, issue `Event`, update `etag`, and save database.
- Use true `ResourceItem` for `state.effect`;
- Add `Resource::toVariant()`.
- Use true `ResourceItem` for `state.effect`;
- Add `Resource::toVariant()`.
Refactor handling PUT of `/lights` `state`, see #1209.
Add bindings for Mueller Light.
ebaauw added 10 commits June 7, 2020 12:16
- Fix name `RAttrLastAnnounced`;
- Fix format of UTC date/time attributes:
  - `null` when invalid;
  - End with `Z` when in UTC;
- Return `RStateEffect` as string.
- Add optional `forceUpdate` parameter to `LightNode::setValue()`;
- Remove `LightNode::setColorXY()`, `LightNode::colorMode()`, and `LightNode::setColorMode()` in favour of `LightNode::setValue()` and `Resource::toString()`.
- Websocket notifications for `lastseen` and `lastannounced`;
- Restore `lastseen` and `lastannounced` from database;
- Add optional `forceUpdate` parameter to `LightNode::setValue()`;
- Remove `LightNode::setColorXY()`, `LightNode::colorMode()`, and `LightNode::setColorMode()` in favour of `LightNode::setValue()` and `Resource::toString()`.
- Leverage `LightNode::setValue()`;
- Collect attributes in one web socket notification.
Leverage `LightNode::setValue()`.
Leverage `LightNode::setValue()`
Leverage `LightNode::setValue()`.
@ebaauw ebaauw requested a review from manup June 8, 2020 18:31
@ebaauw
Copy link
Collaborator Author

ebaauw commented Jun 8, 2020

@manup, could you please have a look at this PR? Running it on my production for a day and haven't encountered any issues, but it's turned into a bigger one.

- Limit #events for `lastseen` (of same resource) to 1/sec;
- Keep `lastseen` and `lastannounced` in sec resolution.
- Keep `lastseen` and `lastannounced` in sec resolution.
Support rule condition for `dx` `attr/lastannounced`, see #2590
@bortim
Copy link

bortim commented Jun 14, 2020

Hello @ebaauw ,

I have a problem after rebooting, because the Tint remote is forgotten.
I have to add a new sensor and reset the remote by the button over the batteries.

Pressing some buttons on the tint does not result in a reachable = true.
I checked this by curl-ing the API.

Regards
bortim

@manup manup merged commit 630845e into dresden-elektronik:master Jun 15, 2020
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.

Just added some minor comments, the PR runs in my setup fine.
I'm doing some more tests with color loop now.

@@ -2881,7 +2895,7 @@ void DeRestPluginPrivate::checkOldSensorGroups(Sensor *sensor)
i->removeDeviceMembership(sensor->id());
}

if (i->state() == Group::StateNormal && !i->hasDeviceMembers())
if (i->address() != 0 && i->state() == Group::StateNormal && !i->hasDeviceMembers())
Copy link
Member

Choose a reason for hiding this comment

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

i->address() != 0 may we should use some constants here to get more meaning and make it easier to search for places where group 0x0000 is used.

GROUP_0 / GROUP_ZERO or something similar.

Copy link
Collaborator Author

@ebaauw ebaauw Jun 15, 2020

Choose a reason for hiding this comment

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

There's several things going wrong with /groups/0. Events are issued on /groups/65520 instead of on /groups/0. A GET of /groups/65520 returns the same as a GET of /groups/0. The entire code handling this should be reviewed. I think we might need to decouple the REST resource ID from the group address for all groups, similar to the Hue bridge.

@@ -343,6 +345,8 @@ QString ApiRequest::apikey() const
DeRestPluginPrivate::DeRestPluginPrivate(QObject *parent) :
QObject(parent)
{
plugin = this;
Copy link
Member

Choose a reason for hiding this comment

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

In v2 we should inject this as dependency in the constructor instead a global variable. Makes writing tests easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, using a global variable definitely doesn't deserve a beauty prize. I tried to add a variable to the constructors at first, but couldn't get that to work. Got lost in wanting to put that into RestNodeBase but needing to pass it to the LightNode and SensorNode classes. I'm afraid my C++ is a bit rusty here, and Qt doesn't help.

}
}
}
else if (param == "xy" && taskRef.lightNode->item(RStateX) && taskRef.lightNode->item(RStateY) &&
taskRef.lightNode->modelId() != QLatin1String("FLS-PP"))
{
// @manup: is check for FLS-PP needed, or is this already handled by check for state.xy?
// @manup: is check for FLS-PP needed, or is this already handled by check for state.x and state.y?
Copy link
Member

Choose a reason for hiding this comment

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

Hmm good question: The check refers to the very old "FLS-PP" wireless ballast from 2012 which didn't support xy color mode there are less than 200 deployed but still active. When they are configured we'd need to strip the xy ResourceItems (the ballast wrongly claims it supports this color mode).

The newer ones with model id "FLS-PP3" support xy and don't fall into this check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You make devices of too high quality, now you never get rid of old stuff ;-)

Another argument for capabilities...

state[key] = item->toVariant();
if (rid.suffix == RStateEffect)
{
state[key] = RStateEffectValuesMueller[item->toNumber()];
Copy link
Member

Choose a reason for hiding this comment

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

Is a guard here for Müller Licht needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, but better naming is. Should probably have used RStateEffectAllValues vs RStateEffectDefaultValues. No clue how to expand this to other lights with similar capabilities, but implemented differently.

@ebaauw
Copy link
Collaborator Author

ebaauw commented Jun 15, 2020

I have a problem after rebooting, because the Tint remote is forgotten.

What are you rebooting? How do you start deCONZ after reboot? Are you sure it starts with your self-compiled REST API plugin?

What do you mean by “ forgotten”? Does it still show in the GUI? Does the /sensors resource still exist? Can the remote still control lights in the associated groups? Does the node in the GUI blinkt blue when you press a button?

I have to add a new sensor and reset the remote by the button over the batteries.

Resetting the remote would only be needed if it has lost the association to the Zigbee network. That cannot be caused by restarting deCONZ.

Pressing some buttons on the tint does not result in a reachable = true.

Is that before or after reboot? Before or after reset and re-pair? Does lastseen change?

@bortim
Copy link

bortim commented Jun 15, 2020

I have a problem after rebooting, because the Tint remote is forgotten.

What are you rebooting? How do you start deCONZ after reboot? Are you sure it starts with your self-compiled REST API plugin?

I am rebooting my "host" - a Raspberry Pi 3 B.
deCONZ is started as a service: systemctl enable deconz // systemctl start deconz
Yes I am, because I compiled the branch with the remote-support and copied it to the deconz folder.

What do you mean by “ forgotten”? Does it still show in the GUI? Does the /sensors resource still exist? Can the remote still control lights in the associated groups? Does the node in the GUI blinkt blue when you press a button?

I am using deconz headless so I can't
The ressource still exists - curl -X GET /sensors/21
[{"error":{"address":"/sensors/21","description":"resource, /sensors/21, not available","type":3}}]
Only if i repair it again, events are created on the websocket for my iobroker.
And the remote-sensor does not have any entry in the Phoscon-Browser-App - is this an issue?

I have to add a new sensor and reset the remote by the button over the batteries.
Resetting the remote would only be needed if it has lost the association to the Zigbee network. That cannot be caused by restarting deCONZ.

Hm okay - strange behavier within my network.

Pressing some buttons on the tint does not result in a reachable = true.

Is that before or after reboot? Before or after reset and re-pair? Does lastseen change?

Usually I do the following:

  1. Start Raspberry with deCONZ & iobroker
  2. Add batteries to the tint (I remove them after playing with iobroker, deconz, a led-stripe and the remote).
  3. Add Sensor with phoscon
  4. Reset the tint to get it working again
  5. Press button
  6. wait for "finished" green in phoscon
  7. play with iobroker, deconz, a led-stripe and the remote
  8. shutdown raspberry
  9. begin with 1. on the next weekend

With
sudo systemctl stop deconz and
sudo systemctl start deconz
it is reproducable for me.

@ebaauw
Copy link
Collaborator Author

ebaauw commented Jun 15, 2020

And the remote-sensor does not have any entry in the Phoscon-Browser-App - is this an issue?

For Phoscon: it doesn't (yet ?) support the remote.

I am using deconz headless so I can't

That's unfortunate. I wouldn't know how to troubleshoot without access to the GUI.

The ressource still exists - curl -X GET /sensors/21
[{"error":{"address":"/sensors/21","description":"resource, /sensors/21, not available","type":3}}]

This actually suggest that the resource no longer exists. Is it included if you do a GET of /sensors?

With
sudo systemctl stop deconz and
sudo systemctl start deconz
it is reproducable for me.

Very strange. How long after pairing the remote do you stop deCONZ? It would seem that the newly paired sensor resource hasn't been written to the database. Do you see the same for other devices? I don't see how this could be related to this PR; best open an separate issue for it.

@bortim
Copy link

bortim commented Jun 16, 2020

And the remote-sensor does not have any entry in the Phoscon-Browser-App - is this an issue?

For Phoscon: it doesn't (yet ?) support the remote.

Thank you for clearification.

The ressource still exists - curl -X GET /sensors/21
[{"error":{"address":"/sensors/21","description":"resource, /sensors/21, not available","type":3}}]

This actually suggest that the resource no longer exists. Is it included if you do a GET of /sensors?

It is not included. Thank you for that hint.
But I got a solution for that:
I have installed my ubuntu and deconz new and now it is working.

Thanks a lot!

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.

None yet

4 participants