-
Notifications
You must be signed in to change notification settings - Fork 597
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.
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
implemented FD for Pcan #537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test receiving messages? It seems like the timestamp format is different for FD
Codecov Report
@@ Coverage Diff @@
## develop #537 +/- ##
===========================================
- Coverage 64.47% 64.05% -0.42%
===========================================
Files 63 63
Lines 5655 5695 +40
===========================================
+ Hits 3646 3648 +2
- Misses 2009 2047 +38 |
@markuspi good catch, thanks. |
@markuspi fixed. Thanks again. |
This might be unexpected:
Maybe just raise a |
A ValueError would be incorrect. I have seen multiple cases where this is intended behavior. |
@christiansandberg What do you think? I'd be okay with that silently padding, as it might indeed make some applications quite a bit easier. Padding with zeros to the next bigger size sounds like something people would expect then, right? |
Padding is completely normal. I guess it could make sense to allow specification of other value than 0 for CRC purposes but not necessary. |
e328127
to
6fe56a1
Compare
After looking through some code on other interfaces i noticed that i can use the len2dlc and dlc2len functions instead of implementing them myself. Also i think can FD message length should be validated the either by _check in Message(as of now only checks that the length is less then 64 for FD) or the send in the Interface. |
You are right, the _check method should validate that as well! Maybe all interfaces should pass But then, why don't we always check it? Performance? |
If so then maybe check should default to True?
I don't think performance is an issue. I can think of reasons one would like to send a malformed message and see it rejected by the other devices. |
Why? We can simply call it and ignore the result if everything is fine. |
i meant that the check parameter should default to true. |
Ah, yes. I think that's a good idea. But I think we have already discussed it at some point ... @christiansandberg @hardbyte What do you think? |
It could be a performance issue on e.g. a RapsberryPi on a high traffic bus. |
We could make it opt-out in the RC/config parameters. |
As it seems that the use of check is a larger issue, |
@christiansandberg Could you also check if this PR shall be merged? |
I agree, I'm not familiar with can-FD though so I'll defer to @christiansandberg |
Sure, just document the new arguments. Otherwise people have to study the source code to figure out what config is accepted. |
Maybe we should also standardize the various timing parameters between interfaces, but that can be done in a separate issue. |
I documented the new arguments.
I think this should extended to a discussion of standardizing interface parameters in general. For example the handling of the state parameter is different between interfaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
I have implemented FD support for PCAN (solving #507).
This code has been tested manually on PCAN-FD hardware.
Points to be discussed:
Should this be switched to be a single string? (this is what the Pcan API accepts).
The benefit to switching would be a simpler interface.
Should we allow for an option to configure the padding?
What should the default padding be?