Skip to content

Conversation

@sthelen-enqs
Copy link
Contributor

When a device sends e.g. a measurementListData response containing incomplete identifiers, and then sends a second measurementListData response containing complete identifiers the data from the second response is not correctly merged with the first response and queries to measurements by ID will always return the value of the first incomplete response and will never update to later values.

Our current approach implemented in this PR is to reject (negative acknowledgement) spine updates containing incomplete identifiers because to my knowledge devices really shouldn't be sending spine messages with incomplete identifiers (and to preclude needing to write a general merge function for generic spine data with potentially incomplete identifiers).

Commit bd9e7c8 adds a (initially failing) test for this scenario to help in reproduction and I've attached trace logs of this scenario happening on a real system below:

2024-12-17 16:05:40 TRACE Recv: *ski* {"data":[{"header":[{"protocolId":"ee1.0"}]},{"payload":{"datagram":[{"header":[{"specificationVersion":"1.3.0"},{"addressSource":[{"device":"d:_i:*device_id*"},{"entity":[1,1]},{"feature":11}]},{"addressDestination":[{"device":"d:_n:*device_id*"},{"entity":[2]},{"feature":9}]},{"msgCounter":60},{"msgCounterReference":26},{"cmdClassifier":"reply"}]},{"payload":[{"cmd":[[{"measurementListData":[{"measurementData":[[{"measurementId":0}],[{"measurementId":7}],[{"measurementId":8}],[{"measurementId":9}],[{"measurementId":4}],[{"measurementId":5}],[{"measurementId":1}],[{"measurementId":2}],[{"measurementId":3}],[{"measurementId":11}],[{"measurementId":13}],[{"measurementId":15}],[{"measurementId":10}],[{"measurementId":12}],[{"measurementId":14}],[{"measurementId":6}]]}]}]]}]}]}}]}

2024-12-17 16:05:50 TRACE Recv: *ski* {"data":[{"header":[{"protocolId":"ee1.0"}]},{"payload":{"datagram":[{"header":[{"specificationVersion":"1.3.0"},{"addressSource":[{"device":"d:_i:*device_id*"},{"entity":[1,1]},{"feature":11}]},{"addressDestination":[{"device":"d:_n:*device_id*"},{"entity":[2]},{"feature":9}]},{"msgCounter":74},{"cmdClassifier":"notify"}]},{"payload":[{"cmd":[[{"function":"measurementListData"},{"filter":[[{"cmdControl":[{"partial":[]}]}]]},{"measurementListData":[{"measurementData":[[{"measurementId":4},{"valueType":"value"},{"value":[{"number":0},{"scale":0}]},{"valueSource":"measuredValue"},{"valueState":"normal"}]]}]}]]}]}]}}]}

The identifiers in this case being the measurementId and the valueType, the first message only contains the measurementId while the second message contains both measerumentId and valueType. In the merge function the checked identifier for the first message is e.g. "4" while the checked identifier for the second message would be "4|value".
End result of this is that functions (such as in the eebus-go usecases) querying data receive multiple values for the same measurementId and have no way of identifying which of those should be correct.

This PR is in Draft state because I still wanted to test with the device producing this data how it responds to receiving a negative acknowledgement on the initial write with incomplete identifiers, and in case I might be missing something.

@coveralls
Copy link

coveralls commented Jan 24, 2025

Coverage Status

coverage: 93.436% (-0.3%) from 93.689%
when pulling f90fdf5 on fix/unset-key-handling
into 5fb9ea1 on dev.

@DerAndereAndi DerAndereAndi added this to the Version 0.8 milestone Jan 28, 2025
@sthelen-enqs
Copy link
Contributor Author

We discussed this, and decided on a couple of TODOs to make this PR better:

  • work on adding additional info to some of the new debug logs to help correlate the log messages with the events causing them
  • add some tests for this case in update_test.go in addition to the integration test to catch regressions faster/more easily
  • in update.go we should check every element in newData to see if no identifiers are set, not just the first element
  • adjust the logic used to check for messages with incomplete identifiers such that if any element in the message has incomplete identifiers, the whole message is NACK'd and no changes are applied

Some work on these points has already been done and I'll update the PR with the changes soon

Specifically:

- Include type of function data in log messages when function data update is rejected because of invalid data
- Evaluate how to handle list data updates item by item (merge or copy to all existing)
- If a there is a single invalid item (some but not all identifiers set) in new data reject the whole list
- Added tests to update_test.go which test behaviour when there is an invalid item in list, or there are items with all and no identifiers mixed in one list.
sthelen-enqs and others added 2 commits February 3, 2025 09:23
The Github action should use Go v1.22 as the go.mod also requires v1.22 as a minimum.
@sthelen-enqs
Copy link
Contributor Author

sthelen-enqs commented Apr 11, 2025

So I just found a device that sends the following message:

Recv: f61d81e6f542b32a05f18c82b745d52ecba34c22 {"data":[{"header":[{"protocolId":"ee1.0"}]},{"payload":{"datagram":[{"header":[{"specificationVersion":"1.3.0"},{"addressSource":[{"device":"d:_i:<...>"},{"entity":[3]},{"feature":11}]},{"addressDestination":[{"device":"d:_n:<...>"},{"entity":[2]},{"feature":9}]},{"msgCounter":62805},{"msgCounterReference":36},{"cmdClassifier":"reply"}]},{"payload":[{"cmd":[[{"measurementListData":[{"measurementData":[[{"measurementId":0},{"valueType":"value"},{"value":[{"number":17},{"scale":0}]},{"valueSource":"measuredValue"},{"valueState":"normal"}],[{"measurementId":7}],[{"measurementId":8}],[{"measurementId":9}],[{"measurementId":4}],[{"measurementId":5}],[{"measurementId":1}],[{"measurementId":2}],[{"measurementId":3}],[{"measurementId":11}],[{"measurementId":13}],[{"measurementId":15}],[{"measurementId":10}],[{"measurementId":12}],[{"measurementId":14}],[{"measurementId":6}]]}]}]]}]}]}}]}

This would fail with this PR as not all list entries have all identifiers set (only the first measurement has both measurementId and valueType set).
I'm pretty sure this device is violating the EEBus_SPINE_TS_ResourceSpecification section 3.4.2.6 Optional identifiers without default values

  1. If the Identifier is present:
    In this case the Identifier SHALL be present in all list entries. As usual, the Identifier is then used to identify particular list entries.

I'm not entirely sure how we want to handle this, but I think we might need the ability to differentiate between mandatory identifiers and optional identifiers. I'm going to put more work into this PR in that direction, but I'm open to any comments or great ideas in this regard.

@DerAndereAndi
Copy link
Member

The SPINE message is not a partial message. So it should overwrite the complete existing data and no merge may happen.

@DerAndereAndi
Copy link
Member

With the integration of #66 this PR should be obsolete. Would you agree? If so, we could close it.

@sthelen-enqs sthelen-enqs self-assigned this Oct 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants