Skip to content

Conversation

@SwoopX
Copy link
Collaborator

@SwoopX SwoopX commented Jun 1, 2021

With introduction of isSameAddress(), a consistent address comparison became availble. Currently, quite a few functions to select alight/sensor have a similar code implemented which varies in some details.

With this PR, the address comparision gets streamlined for the functions getLightNodeForAddress(), getSensorNodeForAddress(), getSensorNodeForAddressAndEndpoint() and getSensorNodeForAddressAndEndpoint() while sparing a few lines of code

return &(*i);
}
}
if (sensor.deletedState() != Sensor::StateNormal || !sensor.node()) { continue; }
Copy link
Member

Choose a reason for hiding this comment

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

The !sensor.node() check is good intention but this would break at least ZGP devices and Sensors where the deCONZ::Node hasn't been assigned yet. Albeit I'd prefer to have this check here the current code base is more structured to have the node() check outside this function.

Comment on lines 9877 to 9878
if (!isSameAddress(sensor.address(), addr)) { continue; }
if (sensor.type() != type) { continue; }
Copy link
Member

Choose a reason for hiding this comment

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

Considering this function is called often I'd suggest to switch these two lines to get a bit more performance.

@manup
Copy link
Member

manup commented Jun 28, 2021

Just checked the ZGP code, in the button handler the getSensorNodeForAddress() is called with sensor.node() == nullptr in

Sensor *sensor = getSensorNodeForAddress(ind.gpdSrcId());
ResourceItem *item = sensor ? sensor->item(RStateButtonEvent) : nullptr;
if (!sensor || !item || sensor->deletedState() == Sensor::StateDeleted)

If the sensor.node() check is removed in that function the PR should be fine with the small tweak mentioned in review comments.

@manup manup merged commit b3c4467 into dresden-elektronik:master Jun 28, 2021
@manup
Copy link
Member

manup commented Jun 28, 2021

Tested with Friends of Hue switch, all good 👍

@manup manup added this to the v2.12.2-beta milestone Jun 28, 2021
@SwoopX SwoopX deleted the resourceselection branch July 17, 2021 11:48
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.

2 participants