-
Notifications
You must be signed in to change notification settings - Fork 39
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
Adds flexible data rate CAN payload to account for 64 byte CANFD and > 64 byte ethernet payloads #21
Open
jshiv
wants to merge
38
commits into
einride:master
Choose a base branch
from
jshiv:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Adds flexible data rate CAN payload to account for 64 byte CANFD and > 64 byte ethernet payloads #21
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
4e93a88
Support canfd (#1)
jshiv 047b82f
Assign byte array directly to big.Int for big and little endian signa…
jshiv 9986b66
Removed payload.Length property and calculate length with len(payload…
jshiv b87d69e
Added Decode and DecodePayload signal methods for decoding without sh…
592beb4
Added unit tests for Decode() to signal_test.go
fd0c28c
Added unit tests to test Decode() and DecodePayload()
f845a4a
Added unit tests for UnmarshalPhysical() and UnmarshalPhysicalPayload()
51c1b50
Merge pull request #4 from jshiv/fix/do_not_shift_ranges_signal_decode
naveenv92 36ce676
Added message.Decode method
cdee1a2
Updated message.Decode to use signal.Decode internally
6e5dbc8
Merge pull request #5 from jshiv/feature/message_decode
naveenv92 cddec59
Updated README with example of decoding and dbc file with >8 byte mes…
naveenv92 ec1debd
Merge remote-tracking branch 'upstream/master'
a2d4186
Merged with upstream/master
ef9ceb3
Removed example_payload.dbc and added a string dbc to the example in …
0248a67
Shortened DBC in the README and fixed linting issues
f697a62
Removed global for linter
16b04f8
Fixed linting issues - passes all golint tests locally
0ec95e8
go mod tidy
b67ca94
Updated generated go code from new Message struct format to avoid mal…
623b114
Merge branch 'master' of github.com:jshiv/can-go
194a970
init
pvora27 eb9bd02
add long name metadata
pvora27 2514e7c
Merge pull request #7 from jshiv/support_long_signal_names
naveenv92 70928fe
Added multiplexing
ba99d84
Updated README
24b187a
Updated README
f5e2511
Fixed reverse function
2376779
Merge remote-tracking branch 'upstream/master'
d599b93
Removed copy from reverse function
f5d4371
Cleaned up code
2a78a25
Reverted to len(data) from len(reversedArray)
68bf611
Changed reverse to append logic and updated unit test
615c5a9
Merge pull request #9 from jshiv/fix/change_reverse
naveenv92 a76019b
Merge branch 'master' of github.com:jshiv/can-go
4d6ffd9
Merge pull request #8 from jshiv/feature/add_multiplex_decoding
naveenv92 7487488
Merge branch 'einride:master' into master
jshiv 566b73c
Fixed broken UnmarshalValueDescriptionPayload (#10)
jshiv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In CAN FD the DLC field may be 16 bits but in our case we're only counting the number of bytes in a frame. The
Frame
struct is supposed to parsecandump
output, which shows the length of the frame in bytes (maximum of 64 bytes for CAN FD) and as such there is no need to modify theLength
field touint16
.Something that needs to be changed instead is the
Data
type, defined indata.go
. There it is hard coded as a slice of length 8, but should be increased to 64 instead.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.
We use an ethernet protocol that can actually send messages >255 bytes in length so the
uint8
wasn't sufficient to hold the message length, which is why we changed this - open to suggestions on how you'd want to deal with thisThere 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.
Your ethernet protocol must be some kind of higher-layer protocol which utilizes CAN in the link layer. In that case, I think the best option would be that you implement a new library for your higher-layer protocol which depends on our library. There, you can wrap the
Frame
type in your own structs to fit your use case. As I see it this is a pure CAN library, thus we should limit the scope to the link-layer aspects and DBC file handling.I assume you're using the descriptor library to parse signals within your protocol, meaning your signals can be more than 255 bits wide as well. This is something we can implement in this library, since signals don't really follow a standard size structure the same way the CAN and CAN FD protocols do. That will be a breaking change but would make sense to do if we're implementing CAN FD support.
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.
Correct, we actually may be able to extract the message length from the header in our ethernet protocol so we can potentially revert this back to a
uint8
- the main issue with > 8 bytes (CAN-FD or arbitrary length) is that we're no longer able to useuint64
to pack the bits so we've resorted to usingmath.big
to handle this which by nature would allows lengths of any size. We can refactor some of our decoding code to deal with a 64 byte payload, it just seems like a more limiting constraint. We see value in keeping the currentcan.Data
struct and associated methods since it's very efficient for messages <= 8 bytes, and we currently use both.Would your team be open to a meeting on Zoom, etc. to talk through some of our design choices? This may be much simpler than going back-and-forth on the comments here. @jshiv and I are both in PST which is 9 hours behind Sweden but we can work something out if that sounds good on your side.
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.
I think that's a great idea to talk "face-to-face" 😄
I sent you a request on LinkedIn, let's exchange contact info there.