Change can.Message

[felixdivo]

Fixes a number of problems with the message object and the file IO:

• make __eq__() compare by identity only
• added channel, bitrate_switch and error_state_indicator to __str__()
• less verbose __repr__()
• __init__() is now properly documented
• added a check param to __init__() that then checks the input, and only then
• Writing and then reading a message does not reproduce the same message again; this will be surprising to new users. This is rather a problem of the IO module and the only partial CAN FD support. ⇒ added warning to docs
• add a deprecation notice to Message.id_type (in the docs and prints a warning when used).
• add __bytes__() support
• use __slots__()
  • allows faster attribute access & space savings in memory
  • using dynamic attributes is still supported but deprecated, and a warning is printed stating that support will be removed in the next major version
  • weak references to messages are still supported
• corrected __hash__() to include all members
• adjusted/extended logformat_test.py accordingly
• add __copy__() and __deepcopy__() (was required by some tests)
• added equals() methods for comparing two messages by value
• get the existing unit tests to run (no new ones are added since nothing seemed too important to test)

To discuss:

• Should the timestamp really be excluded from __eq__() and __hash__()? ⇒ Solved: compare only by identity
• Should we do anything about the inconsistency between the argument name extended_id in the constructor and the name is_extended_id as the attribute? Maybe, but this is not discussed in this PR. See Change can.Message #413.
• What to test in _check()? Is the current stuff enough?

Fixes #350.

[felixdivo]

I only published this unfinished PR to test the Codecov threshold that I complained about here.

[codecov]

Codecov Report

Merging #413 into develop will increase coverage by 0.16%.
The diff coverage is 49.33%.

@@             Coverage Diff             @@
##           develop     #413      +/-   ##
===========================================
+ Coverage    62.33%   62.49%   +0.16%     
===========================================
  Files           55       55              
  Lines         4614     4658      +44     
===========================================
+ Hits          2876     2911      +35     
- Misses        1738     1747       +9

[felixdivo]

The Kvaser interface sets some flags on a message object over here. That is not supported when using __slots__(). Is it really required?

[christiansandberg]

I don't mind changing the equality check, but it is not always trivial what can be considered equal messages. Is a gatewayed message between two CAN buses the same or different? Is a message sent multiple times the same or different? Should a message depend on the current state of the interface? If timestamps should be equal, how much are they allowed to differ? It should be high enough to handle log file resolutions but low enough to be able to separate messages sent in burst mode.

It won't be possible to avoid surprises. I can't imagine a use case where one would create two separate messages and compare them for equality, so maybe it's best to just skip __eq__ and __hash__ altogether and only check for identity. That should result in least surprises.

The custom attributes in the Kvaser interface are not documented so they are probably OK to remove. Hopefully nobody is attaching custom attributes in their applications...

[felixdivo]

Thanks for your thoughts!

It should be high enough to handle log file resolutions but low enough to be able to separate messages sent in burst mode.

That's a good point. For messages sent in a burst, we'd have to have at least 0.1 ms resolution, probably a bit finer, right? And all current log formats are currently fine with delta=1e-6 (in seconds). That means, they seem to preserve the timestamp within that error. But 1e-7 will probably cause problems if I remember correctly. So we can achieve ~0.01 ms resolution with the logging formats. That should barely be enough, I guess?

Checking simply for identity sounds interesting ...

Hopefully nobody is attaching custom attributes in their applications ...

Me too. We could print a warning if one tries to, for a version or two.

[felixdivo]

What do you think about adding a utility function for comparing messages? That should solve the problems with the failing unit tests.

[hardbyte]

If you want to add is_extended_id as an optional (extra) arg to the signature of Message we can start the process of deprecating extended_id.

But this PR is complete without that change - let's tidy this up and merge it in. I'll open an issue for the extended_id and if you want to take it on for this release it will be a small stand alone PR.

[felixdivo]

Doing the extened_id-stuff in another PR sound good.

[felixdivo]

Note: A Pull Request is now allowed to drop the coverage by up to 1.0%, to allow small uncovered or only partially covered changes to be applied.

[felixdivo]

I'm done with this. I would like at least one approving review before this gets merged or someone else merges it after having a look at it. Sorry for the PR being so long, but it was hard to draw a line on where to split this into multiple ones.

[hardbyte]

It might be worth adding a test for the newly supported deepcopy