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

Deprecation of specific attribute methods in cluster classes #591

Closed
cdjackson opened this issue Apr 13, 2019 · 4 comments
Closed

Deprecation of specific attribute methods in cluster classes #591

cdjackson opened this issue Apr 13, 2019 · 4 comments
Labels
breaking change enhancement in progress In progress, or complete but not merged to master
Milestone

Comments

@cdjackson
Copy link
Member

cdjackson commented Apr 13, 2019

I propose the deprecate the specific attribute methods in the autocoded cluster classes as of version 1.2.0 and these methods will be removed completely in 1.3.0.

Specifically, the methods to be removed are the following (example from ZclOnOffCluster) -:

    public Future<CommandResult> getOnOffAsync() {
    public Boolean getOnOff(final long refreshPeriod) {
    public Future<CommandResult> setOnOffReporting(final int minInterval, final int maxInterval) {

These methods are currently provided for all attributes that are considered mandatory, however there are two issues with this.

  1. Some clusters have a LOT of attributes - especially the SmartEnergy clusters where there are hundreds of attributes. This severely bloats the class.
  2. Some clusters have different sets of attributes depending on if they are a client or server. Adding support for client and server clusters is a trade - we can double the number of clusters and have (eg) ZclOnOffClientCluster and ZclOnOffServerCluster (which leads to a lot of new classes), or, we can handle this within the current cluster with two attribute tables. Given the cluster knows already if it is a server or client, then is best, but then these attribute specific methods have to either be removed, or we have to add them for both sets of attributes (adding to class size again).

Given that most clusters have a large number of attributes that are not used in most application, removing them will significantly reduce the framework size.

Instead, users will need to directly call new methods which are fundamentally the same, but with the attribute ID as a parameter (eg) -:

       readAttribute(attributeId, refreshPeriod);
       writeAttribute(attributeId, value);
       setReporting(attributeId, minInterval, maxInterval);
       setReporting(attributeId, minInterval, maxInterval, reportableChange);

Additionally, the methods that take ZclAttribute will also be deprecated -:

    public Future<CommandResult> read(final ZclAttribute attribute)
    public Future<CommandResult> write(final ZclAttribute attribute, final Object value)

Instead these methods will be added to the ZclAttribute class. Thus, instead of -:

ZclAttribute attribute = cluster.getAttribute(id);
cluster.write(attribute, value);

the user can use -:

ZclAttribute attribute = cluster.getAttribute(id);
attribute.write(value);

This again reduces method bloat in the ZclCluster class and makes the API more object oriented. The ZclCluster will continue to provide the methods that allow reading/writing/reporting of attributes given their ID (so is available for non-standard attributes), but if the user wants to interact with attributes defined in the ZigBee standard, then this should be done through the ZclAttribute class.

This is a breaking change and will therefore be wrapped into dev-1.2.0, although the real breaking change will occur in 1.3.0 when the existing methods will be removed. Until then, both methods will be available.

If anyone has other thoughts on this, please feel free to contribute them.

@hsudbrock and @mlriess this is what we discussed a few weeks ago.

@cdjackson cdjackson added this to the 1.2.0 milestone Apr 13, 2019
@cdjackson cdjackson added the in progress In progress, or complete but not merged to master label Apr 13, 2019
@cdjackson
Copy link
Member Author

@triller-telekom I'd welcome your thoughts and also happy to discuss on the phone if you prefer (I forget if you were on the call when myself and Henning discussed this).

@triller-telekom
Copy link
Contributor

No i was not in the call when you and Henning discussed it. But Henning told me about it.

I think it is a good change, because having hundred of methods generated in a class which are mostly unused doesn't make much sense to me.

Your suggestions would simplify the API by getting rid of these ugly named methods like setOnOffReporting(final int minInterval, final int maxInterval), which I wondered why they existed anyway if a general setReporting(attributeId, minInterval, maxInterval) exists.

So from my side feel free to mark them as deprecated and remove them in 1.3.

@cdjackson
Copy link
Member Author

Closed by #527. Leaving this open for now for visibility.

@cdjackson
Copy link
Member Author

Now that 1.2.0 is merged, this is closed. #630 is opened to remind that we should remove the deprecated methods in 1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement in progress In progress, or complete but not merged to master
Projects
None yet
Development

No branches or pull requests

2 participants