Mr-Markus / ZigbeeNet

A .NET Standard library for working with ZigBee
Eclipse Public License 1.0
130 stars 46 forks source link

ZigBeeNode refactor / json-mongodb stores fix #155

Closed oblaise closed 3 years ago

oblaise commented 3 years ago

This PR introduces several changes:

These changes are to some extends breaking changes for library consumers:

spudwebb commented 3 years ago
  • Fix Json store : before the previous changes [des]serialization wasn't working as expected specially on non-public fields and list of enums

What wasn't working exactly? I don't use JsonNetworkDataStore directly but I have my own implementation of a IZigBeeNetworkDataStore which (de)serialize ZigBeeNodeDao as json files. If some properties are going to be serialized differently this is going to be a problem for my use of this library. Can you tell me the exact differences?

oblaise commented 3 years ago

All the fields with private setters weren't [de]serialized correctly. For example each time you were restarting the application the NodeDescriptor.LogicalNodeType field wasn't restored correctly.

The following fields have been changed (in ZigBeeNode and thus in ZigBeeNodeDao):

i.e. ServerCapabilities was using List. In this proposed PR it is now ServerCapabilitiesType. From a Json perspective this means that it was previously serialized as a table of integer and is now serialized as an integer - when Newtonsoft.Json uses its default enum serialization format.

The enum in the NodeDescriptor and PowerDescriptor members have also their assigned value that possibly changed to be directly mapped to their corresponding Zigbee network representation. Meaning that value in the json format before the change and after can be different (i.e. PowerLevelType.MEDIUM was 2 and is now 0x8).

spudwebb commented 3 years ago

@oblaise , sorry I forgot about this PR and I should have commented before the merge:

For IeeeAddress here is how it was serialized before the change:

  "IeeeAddress": {
    "Value": 3781220788176933
  }

so it was already a ulong in json, is it still serialized exactly like that? Also have you checked that deserialization still works? I haven't tested but I wonder how the _address backing field can be populated since now the Value property has no setter?

regarding the other changes to NodeDescriptor: ServerCapabilities, FrequencyBands, MacCapabilities and PowerDescriptor: AvailablePowerSources, those are breaking changes that are going to be very difficult to deal with for my use of this library. Moreover I think those changes add very little benefit, so in my opinion such breaking changes should be discussed a lot more before being introduced.

Mr-Markus commented 3 years ago

@oblaise there were no answer since 3 weeks

But, yes if changes live this that have no functional effect, but breaking changes it should be avoided in the future.

Should we rollback it to List<> ? For me it would be ok

oblaise commented 3 years ago

@Mr-Markus as owner you should decide if that kind of change is acceptable or not. The change has an impact on two things:

@spudwebb as you said you are doing the serialization using your own code, everything can also be controlled at that level. And there is always the possibility to maintain intact the original format.

Mr-Markus commented 3 years ago

Sorry @oblaise I wanted to Menthol @spudwebb 😬

Store implementations like MongoDB are just an example in this repo. So it should be possible to handle it, aren't they?

For me most changes were ok in this PR, but yes I should focus if they are necessary or would improve performance.

I also think about better versioning or backward compatibility. Any Ideas how we can use this changes as an example?

spudwebb commented 3 years ago

@spudwebb as you said you are doing the serialization using your own code, everything can also be controlled at that level. And there is always the possibility to maintain intact the original format.

right, but my serialization code uses the DAO classes from ZigbeeNet, so now I have to implement some json custom converters to be able to deserialize from old and new format.

Could you answer my question about the changes to IeeeAddress?

oblaise commented 3 years ago

Could you answer my question about the changes to IeeeAddress?

You are right. And it must also be resolved using a custom JsonConverter. I've done this for MongoDB. I missed it for JsonNetworkDataStore and currently working on it to fix it.

spudwebb commented 3 years ago

You are right. And it must also be resolved using a custom JsonConverter. I've done this for MongoDB. I missed it for JsonNetworkDataStore and currently working on it to fix it.

Can't we just get rid of the _address field, and just add a private setter to the Value property ?

oblaise commented 3 years ago

This is also a solution. I can do it as soon as @Mr-Markus say that he will not reverse the PR I can push a PR for this.

oblaise commented 3 years ago

156 PR fix the issue