fix endianess issue with 8002 frames

[badzz]

No description provided.

[doudz]

Looks good but could you provide a test ?

[badzz]

hi
No I do not have any test. checking other accepted PR, there was no test in them.
Moreover this is not a new functionality but a bug correction as the type send back was a zigpy_zigate type not (as it should) a ziigpy type.
However we tested it within the https://github.com/zigbeefordomoticz/Domoticz-Zigbee plugin quite extensively.
Thx
thx

[doudz]

Ok, could you provide a link to the issue ?

I'm just not sure this is the right place for that fix :
Maybe it should be here : https://github.com/zigpy/zigpy-zigate/blob/dev/zigpy_zigate/types.py#L208-L209
or maybe here : https://github.com/zigpy/zigpy-zigate/blob/dev/zigpy_zigate/types.py#L136-L141

[doudz]

I made a test and I just don't see the issue :

>>> from zigpy import types as zt
>>> zt.NWK(0x1234)
0x1234
>>> from zigpy_zigate import types as t
>>> t.NWK(0x1234)
0x1234
>>> zt.NWK(0x1234) == t.NWK(0x1234)
True

[badzz]

We are calling serialize to convert into an another format: we need to convert to string "1234"

from zigpy import types as zt
zt.NWK(0x1234).serialize()
b'4\x12'
from zigpy_zigate import types as t
t.NWK(0x1234).serialize()
b'\x124'
t.NWK(0x1234).serialize() == zt.NWK(0x1234).serialize()
False

[doudz]

Thanks for the explanation
And it shows the issue is not fixed by this PR
If the problem is a wrong serialization, it should be fixed in types.py
And I think you're using t.EUI64 in the wrong way, you should call deserialize()

>>> from zigpy import types as zt
>>> ieee=0x123456789
>>> zt.EUI64.deserialize(zt.uint64_t(ieee).serialize())
(00:00:00:01:23:45:67:89, b'')

>>> from zigpy_zigate import types as t
>>> ieee=0x123456789
>>> t.EUI64.deserialize(t.uint64_t(ieee).serialize())
(00:00:00:01:23:45:67:89, b'')

there's no difference

[puddly]

I think @pipiche38's intent is to keep radio library specific data types (zigpy_zigate.types.EUI64 vs zigpy.types.EUI64) out of zigpy. They behave differently even though they're both called EUI64 and have serialize and deserialize methods:

In [1]: import zigpy_zigate.types

In [2]: import zigpy.types

In [3]: zigpy_zigate.types.EUI64(range(8)).serialize()
Out[3]: b'\x07\x06\x05\x04\x03\x02\x01\x00'

In [4]: zigpy.types.EUI64(range(8)).serialize()
Out[4]: b'\x00\x01\x02\x03\x04\x05\x06\x07'

Right now, ControllerApplication.handle_join is passing zigpy-zigate data types to zigpy. Zigpy doesn't do serialization with these objects directly (at the moment) but Domoticz's zigbee plugin does and I think this is a bug.

ieee = zigpy.types.EUI64(response[7][3:11]) is already being done so I don't see the issue with adding nwk = zigpy.types.NWK(response[5].address).

[doudz]

When I started zigpy_zigate I was using zigpy types and there was endian issues that's why I create specific types for zigate

[doudz]

zigpy/zigpy#207 (comment)

[puddly]

The cause of that problem is the same as this one: zigpy-zigate objects are leaving zigpy-zigate's internal code and are being passed into zigpy.

# File: zigpy/application.py
import zigpy.types as t

class ControllerApplication:
    ...

    def handle_join(self, nwk: t.NWK, ieee: t.EUI64, parent_nwk: t.NWK) -> None:
        ...               ^^^^^^^^^^

handle_join's nwk argument should be a zigpy.types.NWK but zigpy-zigate is passing zigpy_zigate.types.NWK. They're both int subclasses and behave similarly but are not the same type (zigpy.types.NWK has .bits(), _hex_repr, etc.).

Other radio libraries are most likely doing the same and should also be fixed.

[pipiche38]

I think @pipiche38's intent is to keep radio library specific data types (zigpy_zigate.types.EUI64 vs zigpy.types.EUI64) out of zigpy.

This is not my what we are doing. I think it was challenging for us to understand , but on the domotciz side we rely only on zigpy and zigpy.types in order to able to work with all raduio libraries.

In any means, you have probably the right point on the handle_message(), but what we have found is that zigpy-zigate doesn't behave as zigpy-znp and this what the PR is about and getting znp working as zigate. Now this is always what we said we are not in position to test with ZHA.

You don't like it. fair. We have our fork and will continue with that. We are talking here on 2 lines of code .!

We had much more and much more important to push (like handling the Zigate protocol, handling correctly the ACK/NACK , group message, broadcast and more ).

[doudz]

You don't like it. fair. We have our fork and will continue with that. We are talking here on 2 lines of code .!

I just want to be sure it will not break zha, that's all

[pipiche38]

In between the library is not performing as it should be there are bug, there are lack of alignment with the firmware, groups commands are not possible, broadcast either.
Due to the endianess issue, the Zigate Coordinator IEEE which is store in the Network Information is wrong, the Extended PanID is the same.
I can fully understand that you don't have the bandwith to maintain, but in taht case you need leave others doing it, and for me this PR is pushed on a dev branch where the main purpose is to get also test from users.

The only issue for me, is that the HA community using Zigate is just impacted by that.

[Hedda]

You don't like it. fair. We have our fork and will continue with that. We are talking here on 2 lines of code .!

I just want to be sure it will not break zha, that's all

I can fully understand that you don't have the bandwith to maintain, but in taht case you need leave others doing it, and for me this PR is pushed on a dev branch where the main purpose is to get also test from users.

The only issue for me, is that the HA community using Zigate is just impacted by that.

FYI, there are two very much related topic discussions about those subjects here (seperate from this specific bug and pull request):

#116

zigpy/zigpy#880

(No idea how to proceed other than trying to get new ZHA testers and/or developers to help with maintenance of zigpy-zigate).

[doudz]

In between the library is not performing as it should be there are bug, there are lack of alignment with the firmware, groups commands are not possible, broadcast either.
Due to the endianess issue, the Zigate Coordinator IEEE which is store in the Network Information is wrong, the Extended PanID is the same.

Ok so many thanks for fixing that !

I can fully understand that you don't have the bandwith to maintain, but in taht case you need leave others doing it, and for me this PR is pushed on a dev branch where the main purpose is to get also test from users.

No problem, you could be a maintainer too, zigpy_zigate is not mine I'm not preventing anyone to work on it.

The only issue for me, is that the HA community using Zigate is just impacted by that.

What do you mean ? impacted by the bug ? or impacted by this PR ?

Just to be clear, if merging this PR break zigate support for HA => it's a problem and we need to provide informations to fix the problem. If it doesn't break anything and just fix a bug, everything is ok.

[doudz]

Merged, feel free to test and contribute