Fix time range being incorrectly reported to start at 0 #6
Conversation
src/ULog.ts
Outdated
| // Other message types do not have a timestamp field, so we use the current max. timestamp or | ||
| // alternatively the header timestamp which denotes when the logging started. | ||
| const timestamp = maxTimestamp ?? this._header.timestamp; | ||
| timeIndex.push([timestamp, i++, message]); |
There was a problem hiding this comment.
Why do messages without a timestamp field need to be in the timeIndex?
There was a problem hiding this comment.
Was wondering the same. The reason is, that the current readMessages generator function returns all messages in the data section. If we limit this to only return message types with timestamps, then we do not have to add messages without a timestamp to the time index.
There was a problem hiding this comment.
Are these other message types that don't have timestamps used for anything? Seems like we could either not emit them from readMessages or skip them when building the index?
There was a problem hiding this comment.
Are these other message types that don't have timestamps used for anything?
At least not by us. Not sure if there are any other users of this package (other than Lichtblick).
I think I will go ahead and not emit these kind of messages.
Edit: I kept the types of emitted messages the same and only changed the time range calculation
src/ULog.test.ts
Outdated
| @@ -301,7 +301,7 @@ describe("README.md", () => { | |||
| const ulog = new ULog(reader); | |||
| await ulog.open(); // required before any other operations | |||
| expect(ulog.messageCount()).toBe(64599); // ex: 64599 | |||
| expect(ulog.timeRange()).toEqual([0n, 181493506n]); // ex: [ 0n, 181493506n ] | |||
| expect(ulog.timeRange()).toEqual([112574307n, 181493506n]); | |||
There was a problem hiding this comment.
magical - we already had a (broken) test case 🎉
There was a problem hiding this comment.
well, turned out that there is really a data message with 0n timestamp. Change here was related to the minTimestamp assignment bug
src/ULog.ts
Outdated
| let maxTimestamp = 0n; | ||
| let logMessageCount = 0; | ||
|
|
||
| let i = 0; | ||
| let message: DataSectionMessage | undefined; | ||
| while ((message = await this.readParsedMessage())) { | ||
| if (message.type === MessageType.Data) { | ||
| minTimestamp ??= message.value.timestamp; |
There was a problem hiding this comment.
If we are never checking with <, how do we know this is the min?
There was a problem hiding this comment.
arg, good catch! I forgot that these messages are not necessarily ordered
achim-k
deleted the
achim/fg-9416-ulog-incorrect-handling-of-addlogged-timestamp-0
branch
Changelog
Fix time range being incorrectly reported to start at 0
Docs
None
Description
Fixes that the time range being incorrectly reported to start at 0 by using only data & log messages for determining the time range.