Conversation
(cherry picked from commit bd26d99)
|
Oh no, this seems to be a problem with timestamps again ... |
Merge release 2.1.0 into develop
|
Oh wow, now the tests pass. ^^ I will still patch it to not occur again if there is still a problem ... |
|
Okay, so as I understand it, the tests above fail (and should always fail) because of a legitimate reason. The problem resides in |
There was a problem hiding this comment.
Sorry, there were too many unrelated changes so I only got half way through. Could you clean this up a little and focus on error frames?
| @@ -1,9 +1,13 @@ | |||
| #!/usr/bin/env python | |||
| # coding: utf-8 | |||
There was a problem hiding this comment.
The shebang is only useful if the file can be executed as a script. Most files here are modules so it doesn't add any value.
I don't think there are any non-ASCII characters in these files so the UTF-8 should not be needed either. Besides, UTF-8 is default in Python 3.
There was a problem hiding this comment.
These changes originate from a different PR that this one was rebased on, since it required changes that were made over there. That is also the reason for why there are so many file that were changed.
I think the shebang does add value as it make clear that it is compatible with Python 2 & 3 as well as being uniform. But I guess that's subjective. It doesn't do any harm however.
The coding line does add value as well, since it enforces the same behavior on Python 2 & 3. That is desirable since everything that makes these versions more alike in such manners is welcome for maintainability. It also makes it explicit.
|
|
||
| def __init__(self, function, error_code, arguments): | ||
| super(IscanError, self).__init__() | ||
| #: Status code |
There was a problem hiding this comment.
Why was this changed? See Sphinx documentation.
There was a problem hiding this comment.
Sorry, I misunderstood that. I will revert that.
can/interfaces/pcan/pcan.py
Outdated
| self.channel_info = channel | ||
|
|
||
| bitrate = kwargs.get('bitrate', 500000) | ||
| bitrate = kwargs.get('bitrate', can.rc.get('bitrate', 500000)) |
There was a problem hiding this comment.
This should be solved on a higher level instead so that it works for all interfaces.
There was a problem hiding this comment.
You are right. I removed it.
|
@christiansandberg Hey, I think there was a misunderstanding. I wanted to ask if you could comment on my design question above concerning the writers in the |
|
If you could rebase on top of develop it would be easier to find the actual changes. |
|
Holy f*ck! That made it worse. I simply rebased on top of |
|
Oh, you can see the actual changes here! I hope that helps ... |
can/io/asc.py
Outdated
| channel = msg.channel if isinstance(msg.channel, int) else self.channel | ||
| line = self.LOG_STRING.format(time=timestamp, | ||
|
|
||
| line = self.LOG_STRING.format(time=msg.timestamp, |
There was a problem hiding this comment.
Here is an example of an ASC file. There is not much info on this format but my interpretation is that Begin Triggerblock Mon Mar 7 01:21:51 pm 2005 defines the start of measurement and the timestamps are relative to this time.
|
I'm thinking it would be better to wait with writing the header until the first message is received and use that timestamp instead of the current time. This would make the result more reproducible and allow for converting formats in the future. I still think a |
|
I now changed
Shall I merge it like that? The tests all run successfully and the changes can be viewed here. |
can/io/asc.py
Outdated
| if not self.header_written: | ||
| self.last_timestamp = (timestamp or 0.0) | ||
| self.started = self.last_timestamp | ||
| self.log_file.write("Begin Triggerblock %s\n" % self.last_timestamp) |
There was a problem hiding this comment.
Timestamp should be converted to the format "%a %b %m %I:%M:%S %p %Y".
|
I also added |
won't change; was discussed in another PR
| in the resulting file. The locations as selected randomly | ||
| but deterministically, which makes the test reproducible. | ||
| :param bool round_timestamps: if True, rounds timestamps using :meth:`~builtin.round` | ||
| before comparing the read messages/events |
There was a problem hiding this comment.
Maybe it's better to specify number of decimal places to match against? ASC files only store 4 decimals currently.
There was a problem hiding this comment.
Well, I think the problem comes from the timestamps being relative to the start of the measurement, but that start timestamp is being serialized without sub-second precision. One failing test case failed because of something like 1000000.197 != 1000000.2. So I guess rounding is more appropriate in the case of ASC files.
There was a problem hiding this comment.
@christiansandberg Do you agree with that? Shall we merge it?
There was a problem hiding this comment.
Right, didn’t think of that. Go ahead and merge.
Closes #217 (cherry picked from commit 61f27e1)
Replaces #247