Simplify attribute record types in foundation

[puddly]

The only one left is AttributeReportingConfig, which has a field whose data type is determined by a previous field. This isn't supported by the struct module right now without a few changes.

One ambiguous class that I saw is ConfigureReportingResponseRecord. The spec (2.5.10.1) says that:

Note that attribute status records are not included for successfully configured attributes, in order to save bandwidth. In the case of successful configuration of all attributes, only a single attribute status record SHALL be included in the command, with the status field set to SUCCESS and the direction and attribute identifier fields omitted.

It doesn't say "SHALL not", only "are not". Does this mean that status records for successfully configured attributes can possibly be present? If so, can they have direction and attrid fields?

I'd like for serialization and deserialization to be inverses, so T.deserialize(T.serialize()) == (T, b"") should always be true.

This necessitated the behavior of the class and an associated unit test to be changed by making ConfigureReportingResponseRecord(Status.SUCCESS, 0x00, 0xAABB).serialize() be equal to b"\x00\x00\xbb\xaa" instead of b"\x00".

[coveralls]

Coverage decreased (-0.02%) to 99.98% when pulling c3612fe on puddly:improvement/simplify-foundation-structs into 14579e5 on zigpy:dev.

[Adminiuga]

It doesn't say "SHALL not", only "are not". Does this mean that status records for successfully configured attributes can possibly be present? If so, can they have direction and attrid fields?

This is exactly the problem I was experiencing with Zen-01 thermostat and why deserialization was changed.

[puddly]

I'd like to add an explicit unit test for that. Like this?

In [1]: from zigpy.zcl import foundation as f

In [2]: f.ConfigureReportingResponse.deserialize(
  ...     f.ConfigureReportingResponseRecord(
  ...         status=f.Status.SUCCESS,
  ...         direction=f.ReportingDirection.SendReports,
  ...         attrid=0x0001,
  ...     ).serialize()  # b'\x00\x00\x01\x00'
  ...     + f.ConfigureReportingResponseRecord(
  ...         status=f.Status.SUCCESS,
  ...         direction=f.ReportingDirection.SendReports,
  ...         attrid=0x0002,
  ...     ).serialize()  # b'\x00\x00\x02\x00'
  ...     + f.ConfigureReportingResponseRecord(
  ...         status=f.Status.UNSUPPORTED_ATTRIBUTE,
  ...         direction=f.ReportingDirection.SendReports,
  ...         attrid=0x0003,
  ...     ).serialize()  # b'\x86\x00\x03\x00'
  ... )
Out[2]:
([ConfigureReportingResponseRecord(status=<Status.SUCCESS: 0>, direction=<ReportingDirection.SendReports: 0>, attrid=1),
  ConfigureReportingResponseRecord(status=<Status.SUCCESS: 0>, direction=<ReportingDirection.SendReports: 0>, attrid=2),
  ConfigureReportingResponseRecord(status=<Status.UNSUPPORTED_ATTRIBUTE: 134>, direction=<ReportingDirection.SendReports: 0>, attrid=3)],
 b'')

Or were they using multiple "short" SUCCESS responses (with only the status field)?

[Adminiuga]

I frankly don't remember all the details. i'll need to re-pair it and collect the logs.

[pipiche38]

Shouldn't ReadAttributeResponse similar to ConfigureAttributeresponse ? from my knowledge and from what I have seen it is the case

[Adminiuga]

Shouldn't ReadAttributeResponse similar to ConfigureAttributeresponse

one contains the attribute value, the other one does not. Check ZCL specs for more details

[puddly]

I've reverted the changes to ConfigureReportingResponse and ConfigureReportingResponseRecord for now.