The neovi interface `use_system_timestamp` feature has compatibility issues

[grant-allan-ctct]

Describe the bug

I am a beginner with python-can, and am up to learning about the asyncio feature. After I adapted the asyncio sample code to use the neovi interface, I encountered exceptions being thrown in the code which was logging an asc file. The code in asc.py tries to perform a split on the decimal point in this line, and this was causing an exception to be thrown due to my timestamps being an integral number of milliseconds. I realized that this was related to the use_system_timestamp feature that is specific to the neovi interface. It turns out that if this Boolean is set to True when creating the Bus instance, timestamps from the Bus are emitted as DWORD values rather than floats, and the emitted timestamps are in terms of milliseconds rather than seconds.

To Reproduce

After getting the asyncio example code running successfully as is, change the constructor arguments passed to can.Bus (this line) to instead specify interface of "neovi" and appropriate device serial number, channel, baudrate..., and be sure to include use_system_timestamp=True as well. Run up the example to see exceptions being thrown by the ASC logger.

Expected behavior

I expected no exceptions 😎 . (You can get to the expected behavior by changing use_system_timestamp=False or removing that constructor argument altogether.)

Additional context

OS and version: Windows10
Python version: 3.11.3
python-can version: 4.3.0
python-can interface/s (if applicable): neovi

I think that the goal of the use_system_timestamp feature (mentioned here) is worthwhile, but I suggest that there should be mention added to the documentation of it being non-standard (by which I mean peculiar to "neovi"), and warnings given regarding the incompatibilities that it can cause with the rest of the python-can ecosystem. I imagine that the trouble which the milliseconds timestamps caused for the ASC logger could be just one of multiple areas where python-can code might get tripped up by the timestamp values being 1000 times larger than "normal" timestamps. Another example might be this code which uses timestamps to determine whether two messages are equal.

Because of the potential usefulness of having it available, maybe there is a different design that could be used to affix OS-compatible timestamps to the incoming/outgoing messages, something universal across all interfaces, and done in a way that doesn't "overload" the traditional timestamping mechanics with new meaning/measurement units? Just a thought...

Traceback and logs

def func():
    return "hello, world!"

[lumagi]

Hi,
thanks for doing such a thorough analysis of the bug, I think it is spot-on. At the moment, I see the following problems that would need addressing:

• The neovi bus with use_system_timestamp=True should return the timestamp as a float number in seconds instead of milliseconds to have a properly normalized timestamp.
• The asc serializer has a rather unfortunate way of handling milliseconds in the initial header timestamp. There shouldn't be any need for string split operations.

@zariiii9003 What do you think?

[zariiii9003]

The docstring of the use_system_timestamp parameter should warn, that this is incompatible with the IO-parts of python-can.
The string splitting in ASCWriter is weird indeed, something like this should be more robust: f"{self.last_timestamp * 1000 % 1000:.0f}". But there's probably a better way 😄

[lumagi]

@zariiii9003 but you would keep the data type as int in milliseconds?

[zariiii9003]

@zariiii9003 but you would keep the data type as int in milliseconds?

Tbh, i don't know.
Is the default msg.timestamp of a neovi bus not comparable to time.time()??
I assumed, use_system_timestamp makes use of some raw hardware timestamp, but it doesn't...

[grant-allan-ctct]

My thought is that at minimum we should warn about the dangers.

I think that it would be even better (a lot better) to not have this two different concepts of "timestamp" that are controlled by the value of a Boolean argument (and isolated to this one neovi interface). But the flip side of making such a change is it that someone's working usage might stop working.

Final thought: if we decide that we are OK with a path forward that alters the current behavior, perhaps we can just remove it altogether and provide it differently and universally, i.e. for all of the supported hardware devices. I do like the idea of being able to "timestamp" the messages according to a system clock rather than using the hardware timestamps. I can see that it could be very useful to people, e.g. a bloke who wants to have two devices plugged in at once and have their CAN traffic both timestamped against a common "clock". Maybe there is a standard way that we can build this ability into the python-can library itself, so that the neovi interface would not need to do it in a special way.

[zariiii9003]

ping @pierreluctg

[lumagi]

@zariiii9003 I have been looking into the ASC writer. Do you know if the localization of the date string is mandatory? It strikes me as quite difficult to test due to the unintended side effects.

[zariiii9003]

I don't know tbh. But maybe this helps.