Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Timestamps in Diagnostic Logs cluster do not match spec #24265

Closed
bzbarsky-apple opened this issue Jan 4, 2023 · 1 comment · Fixed by #24830
Closed

Timestamps in Diagnostic Logs cluster do not match spec #24265

bzbarsky-apple opened this issue Jan 4, 2023 · 1 comment · Fixed by #24830
Labels
spec Mismatch between spec and implementation v1.1

Comments

@bzbarsky-apple
Copy link
Contributor

Per spec, in RetrieveLogsResponse the UTCTimeStamp should be epoch-us (64-bit microseconds) and the TimeSinceBoot should be systime-us (64-bit microseconds).

In the SDK right now UTCTimeStamp is an epoch-s in the XML, but the actual value the cluster stores in there is a millisecond monotonic timestamp (which is neither seconds, nor an epoch value).

In the SDK right now TimeSinceBoot is an INT32U in the XML. It's never actually sent on the wire by the current cluster implementation.
@bzbarsky-apple bzbarsky-apple added spec Mismatch between spec and implementation v1.1 labels Jan 4, 2023
@bzbarsky-apple
Copy link
Contributor Author

so can't this be made an optional field?

Both timestamp fields are already optional.

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Feb 2, 2023
@bzbarsky-apple
Fix timestamps in Diagnostic Logs cluster to follow the spec.
fef3749 
There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes project-chip#24265
@bzbarsky-apple bzbarsky-apple mentioned this issue Feb 2, 2023
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Feb 2, 2023
@bzbarsky-apple
Fix timestamps in Diagnostic Logs cluster to follow the spec.
b7b1af2 
There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes project-chip#24265
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Feb 2, 2023
@bzbarsky-apple
Fix timestamps in Diagnostic Logs cluster to follow the spec.
a9717a8 
There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes project-chip#24265
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Feb 14, 2023
@bzbarsky-apple
Fix timestamps in Diagnostic Logs cluster to follow the spec.
42e4c6a 
There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes project-chip#24265
bzbarsky-apple added a commit that referenced this issue Feb 14, 2023
@bzbarsky-apple
Fix timestamps in Diagnostic Logs cluster to follow the spec. (#24830)
1d6a958 
* Fix timestamps in Diagnostic Logs cluster to follow the spec.

There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes #24265

* Address review comment.
kkasperczyk-no pushed a commit to kkasperczyk-no/sdk-connectedhomeip that referenced this issue Mar 15, 2023
@bzbarsky-apple @kkasperczyk-no
Fix timestamps in Diagnostic Logs cluster to follow the spec. (#24830)
9bccd16 
* Fix timestamps in Diagnostic Logs cluster to follow the spec.

There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes project-chip/connectedhomeip#24265

* Address review comment.
kkasperczyk-no pushed a commit to kkasperczyk-no/sdk-connectedhomeip that referenced this issue Mar 15, 2023
@bzbarsky-apple @kkasperczyk-no
Fix timestamps in Diagnostic Logs cluster to follow the spec. (#24830)
4b290ba 
* Fix timestamps in Diagnostic Logs cluster to follow the spec.

There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes project-chip/connectedhomeip#24265

* Address review comment.
lecndav pushed a commit to lecndav/connectedhomeip that referenced this issue Mar 22, 2023
@bzbarsky-apple
Fix timestamps in Diagnostic Logs cluster to follow the spec. (projec…
ca38aac 
…t-chip#24830)

* Fix timestamps in Diagnostic Logs cluster to follow the spec.

There were a few issues here:

1. We were using a monotonic timestamp, but setting the UTCTimeStamp in the
response payload.

2. We were using a millisecond timestamp, whereas the spec has microsecond ones.

Switch to using "time since server init" for the timestamp, put it in the
TimeSinceBoot field, and make sure it has the right units.

Also fixes the timestamps to actually be optional, per spec.

Fixes project-chip#24265

* Address review comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
spec Mismatch between spec and implementation v1.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix timestamps in Diagnostic Logs cluster to follow the spec. bzbarsky-apple/connectedhomeip
1 participant
@bzbarsky-apple