Polling optimisation
Opened this issue · 6 comments
It seems that some consolidation of polling is needed to avoid a) polling the same attribute multiple times, and b) to request multiple attributes in the same request. This is needed to optimise polling - especially on larger systems.
I would propose to add a new method in the Converter to return all the attributes that need to be refreshed during polling. This Set<ZclAttribute> getRefreshAttributes() cold be set in the converter initialisation, and then the ThingHandler can get the list when it performs polling.
A new getEndpoint() method is also required in the converter - from this, it should be easy to build a list of clusters that need to be polled, along with the attributes in each cluster.
If I look at a color bulb, we request the following -:
clusterOnOff.getOnOff(0);
clusterLevelControl.getCurrentLevel(0);
clusterColorControl.getColorTemperature(0);
and
clusterColorControl.getCurrentHue(0);
clusterColorControl.getCurrentSaturation(0);
or
clusterColorControl.getCurrentX(0);
clusterColorControl.getCurrentY(0);
I also think we probably should be reading -:
clusterColorControl.getColorMode(0);
in both the color and colorTemp converters, which adds 2 more requests.
Currently, and assuming we should be refreshing the mode, this is a total of 7 packets (or 5 without the mode). With the above approach, this would be reduced to 3 packets (one for each cluster). If there are 30 bulbs in the system, that's a reduction of 120 frames (or 60 without the mode!), which is 1 or 2 per second at 60 second polling (on average!).
@hsudbrock this is the problem we discussed briefly this morning - WDYT (I'm open to better suggestions if you have any ;) ).
Just FTR...
On a system with 32 lamps, in a 4.5 minute log, there are 104 polling events, resulting in 455 ReadAttribute commands. Note that this is before the ColorMode is added to the converters, which would add another 208 requests under the current system bringing the total to 763 frames in 290 seconds!!
With the proposed system, this would be 312 in either case - still not insignificant, but 50% of the load seen here.
I started to code this up as a test of performance on this system as I think all devices are color bulbs, but there is one piece missing in the puzzle, and that is if the attribute is from an input or output cluster. Either this information needs to be added to the ZclAttribute in the Z-Smart Systems core, or the implementation in OH needs to be different than I propose here. It could possibly be as simple also returning the cluster as well as the attribute (eg it could return Map<ZclCluster, Set<ZclAttribute>> instead of just Set<ZclAttribute>).
On the above system, removing polling completely provides good performance (apparently faster than with the Hue bridge). I should note that this is using Ember NCP with SW flow control (so 57k6).
Another thought... We should ensure that only 1 active poll request per thing is outstanding at any point in time. This would ensure that if the stack is busy, and someone sets their polling rate quite high, that we don't fill up the queues with multiple polls from the same device.
Chris, thanks for creating this issue!
Do I understand the idea correctly that the ThingHandler would no longer call the converter.handleRefresh() method in the pollingRunnable, but instead get the to-be-polled attributes from the converter, and then query those attributes in an optimized way (e.g., using multi-attribute requests)?
I wonder whether such an optimization should be done whenever handleRefresh is called. E.g., in doNodeInitialization, instead of calling handler.handleRefresh(), the thing handler could do a single multi-attribute request as well (instead of the multiple requests that are currently triggered by some handleRefresh calls). This would probably mean that we could get rid of the handleRefresh method in the converters.
Yes, my proposal would be to deprecate the handleRefresh() method, and instead have a getPolledAttributes method. This is called each time there is a polling event to get the attributes. The attributes themselves are set in a map (or whatever) during the initialisation method - as you mention above.
This requires ZSS 1.3.0