Move static EZSP version to non-static property in Zigbee Dongle EZSP

[mikomarrache]

This PR is very important when working with multiple NCPs and therefore multiple network managers. The fact that the EZSP version is stored in a static property makes it shared by all network managers and can cause various issues (like not being able to manage NCPs with pre-v8 and v8 versions).

The changes have been tested.

[lgtm-com]

This pull request fixes 1 alert when merging abf454a into b15130c - view on LGTM.com

fixed alerts:

• 1 for Dereferenced variable may be null

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

[cdjackson]

I don't think it's a good idea to only allocate a single EmberNcp that's used by multiple threads. That saves information in fields there that may be required by users of an individual NCP instance (eg the getLastStatus() method). I think this needs to change back to the original implementation so that a new instance of the EmberNcp is created for different threads/users.

[mikomarrache]

I saw that before I removed the method, I think it's a waste of ressource. The only information saved in the class was the last status (which is used in the library at one place only - energy scan). Client code that would like to access the last status could save it and access it later. Every method in EmberNcp returns the response (which includes the status) or the status. In my opinion, this class could be merged into ZigBeeDongleEzsp (at the end, every instance of EmberNcp was accessed by first getting ZigBeeDongleEzsp via the network manager).

I agree it's a breaking change but that could be merged in a next major release. In any case, let me know if you agree with the change. If not, I will revert this part.

[cdjackson]

In my opinion, this class could be merged into ZigBeeDongleEzsp

I'm not keen on that - for starters that would be quite a large change for user code. Secondly, it just makes ZigBeeDongleEzsp a huge class which means it's even less maintainable.

[cdjackson]

Every method in EmberNcp returns the response (which includes the status) or the status.

That's not correct. Many methods return the inner information or null if the response was an error. For example -:

    public EmberKeyStruct getKey(EmberKeyType keyType) {
        EzspGetKeyRequest request = new EzspGetKeyRequest();
        request.setKeyType(keyType);
        EzspTransaction transaction = protocolHandler
                .sendEzspTransaction(new EzspSingleResponseTransaction(request, EzspGetKeyResponse.class));
        EzspGetKeyResponse response = (EzspGetKeyResponse) transaction.getResponse();
        if (response == null) {
            return null;
        }
        logger.debug(response.toString());
        lastStatus = response.getStatus();
        if (lastStatus != EmberStatus.EMBER_SUCCESS) {
            return null;
        }
        return response.getKeyStruct();
    }

In this case, if you want to know why the key request failed, you need to access lastStatus.

[cdjackson]

As above, I think this really needs to return a new instance, and not reuse the same instance for all users.

[cdjackson]

I'm uncomfortable with reusing the same instance for the reasons mentioned already. It might be ok if we can be sure that they always run in the same thread, but otherwise I think there should be a new instance.

[cdjackson]

Why is this removed and replaced with a call to get the protocol handler in each method?

[mikomarrache]

The protocol handler doesn't include the EZSP version required by getVersion(). Therefore, I replaced the protocolHandler property by the zigBeeDongleEzsp property. The protocolHandler can then be accessed via the zigBeeDongleEzsp.

[cdjackson]

Why not save the protocolHandler in the constructor as well. That would allow retaining the same code in all the other methods, and reduce a set of calls to get the protocol handler every time.

[mikomarrache]

This was my first version. Then, I got an issue where the protocol handler wasn't initialized at the time EmberNcp was created. Therefore, I thought it's better not to rely on that, and access the protocol handler as late as possible (i.e. in the different methods).

[cdjackson]

Then, I got an issue where the protocol handler wasn't initialized at the time EmberNcp was created.

I've not looked at when your changes now create EmberNcp, but I guess it's created very early now as you only have a single instance that's reused? Does this issue go away if we revert to creating new instances of EmberNcp as per the discussion above? Or alternatively, is this only an issue due to where EmberNcp is instantiated, and if that was moved does that solve this issue?

[mikomarrache]

It is created only once during the initialization (initialize() method). The same instance is then reused all over the code.

Creating the EmberNcp requires some other stuffs to be initialized (like the protocol handler), therefore I found it more correct to leave this responsibility to the initialization code.

In my opinion, making the EmberNcp stateful (like before this PR) is a bad idea. What is the meaning of the last status? To which last call it refers? As I said, I still think this should be the responsibility of client code to manage this state, not the library.

[cdjackson]

What is the meaning of the last status? To which last call it refers?

It is the return value from the last call made to this NCP instance. Given that an NCP instance is created for each "user", this information is specific to a thread/class/method/whatever in which it was created. Therefore it has meaning to that process.

[cdjackson]

Why has this method been removed? This is quite a large breaking change.

[mikomarrache]

Same as comment above.

[mikomarrache]

I would also add that these changes have been heavily tested in a production environnement with dozens of Ember NCPs and thousands of ZigBee devices. In the future, we will probably have to deal with Ember NCPs of different versions and the actual library code will not allow that since we use the library in one application that manages all these NCPs.

[cdjackson]

In the future, we will probably have to deal with Ember NCPs of different versions and the actual library code will not allow that since we use the library in one application that manages all these NCPs.

I really wish I'd added a version tag to each method in the XML definition so that we could manage different versions better. However Silabs don't really provide this information in an easy format - we'd have to sift through the different versions of UG100... Still it could be done where the API has changed.

[Hedda]

@mikomarrache May I suggest that you also check out the somewhat indirectly related discussion/request here -> #1332